Pass in response status to mark resource timing #1468

abinpaul1 · 2022-07-09T14:21:12Z

These changes are to support addition of http response status code to Perfomance resource timing. Further details are available at w3c/resource-timing#335

At least two implementers are interested (and none opposed):
- Chromium : https://chromestatus.com/feature/5163838794629120
- Firefox : HTTP response status code in Resource Timing mozilla/standards-positions#665
Tests are written and can be reviewed and commented upon at:
- https://chromium-review.googlesource.com/c/chromium/src/+/3754923
- https://chromium-review.googlesource.com/c/chromium/src/+/3958184
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1343293
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1796785
- Safari: https://bugs.webkit.org/show_bug.cgi?id=246857
- Deno (not for CORS changes): …

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

fetch.bs

yoavweiss

Non-authoritative LGTM

yoavweiss · 2022-07-11T12:36:23Z

fetch.bs

@@ -5536,6 +5539,9 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:

       <li><p>Let <var>status</var> be the HTTP response's status code.


@annevk - Would it make sense to also explicitly set the response's status to |status| here? (maybe as a separate PR)

It might be worth having a follow-up issue on how to better integrate with HTTP or indicate that's what's happening. We basically want to map an HTTP response to a Fetch response. There has been some discussion about this also with the HTTP WG, but I don't think we'll get anything formal out of that.

yoavweiss · 2022-07-11T12:38:01Z

Also, we'd want tests and implementation issues before this lands.

noamr · 2022-07-27T14:06:27Z

fetch.bs

@@ -301,6 +301,9 @@ following <a for=struct>items</a>: [[RESOURCE-TIMING]] [[NAVIGATION-TIMING]]
 <dt><dfn export for="response body info" id="fetch-timing-info-decoded-body-size">decoded
 size</dfn> (default 0)
 <dd>A number.
+ <dt><dfn export for="response body info" id="fetch-timing-info-response-status">response


This is not needed. You have response's status when you set the report timing steps in fetch response handover. I would add it as another parameter to Mark resource timing.

Also, as we talked on the Chromium CL, I don't think we should automatically make this TAO-protected rather than CORS-protected. It's not exactly timing information.

Okay, just to be sure I understand. We would pass in the status as a new parameter to Mark resource time which would in turn pass it to Setup the resource timing entry where we would use it to set the entry's responseStatus.

Also, as we talked on the Chromium CL, I don't think we should automatically make this TAO-protected rather than CORS-protected. It's not exactly timing information.

Yeah, we don't want to. I am a bit unsure as to where the exact place to specify this is. Does it make sense to add this to fetch response handover as well - If the CORS check fails pass in 0 as the status to Mark resource timing

Okay, just to be sure I understand. We would pass in the status as a new parameter to Mark resource time which would in turn pass it to Setup the resource timing entry where we would use it to set the entry's responseStatus.

Exactly

Also, as we talked on the Chromium CL, I don't think we should automatically make this TAO-protected rather than CORS-protected. It's not exactly timing information.

Yeah, we don't want to. I am a bit unsure as to where the exact place to specify this is. Does it make sense to add this to fetch response handover as well - If the CORS check fails pass in 0 as the status to Mark resource timing

Actually I don't think you need to do anything. If this is a no-cors request, response would already be an opaque filtered response with a status of 0.

I think there might be an existing problem here in that an opaque response wouldn't expose body info.

We set the body info to an empty body info if TAO fails.

annevk · 2022-07-28T15:46:28Z

fetch.bs

@@ -5536,6 +5539,9 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:

       <li><p>Let <var>status</var> be the HTTP response's status code.


It might be worth having a follow-up issue on how to better integrate with HTTP or indicate that's what's happening. We basically want to map an HTTP response to a Fetch response. There has been some discussion about this also with the HTTP WG, but I don't think we'll get anything formal out of that.

annevk · 2022-07-28T15:47:50Z

fetch.bs

@@ -301,6 +301,9 @@ following <a for=struct>items</a>: [[RESOURCE-TIMING]] [[NAVIGATION-TIMING]]
 <dt><dfn export for="response body info" id="fetch-timing-info-decoded-body-size">decoded
 size</dfn> (default 0)
 <dd>A number.
+ <dt><dfn export for="response body info" id="fetch-timing-info-response-status">response
+ status</dfn> (default 0)


One minor thing here that I notice is incorrect for some other fields as well is that fetch.bs shouldn't wrap inside phrasing elements such as <dfn> so you can more easily search for the terms inside of them.

annevk · 2022-07-28T15:49:30Z

fetch.bs

@@ -301,6 +301,9 @@ following <a for=struct>items</a>: [[RESOURCE-TIMING]] [[NAVIGATION-TIMING]]
 <dt><dfn export for="response body info" id="fetch-timing-info-decoded-body-size">decoded
 size</dfn> (default 0)
 <dd>A number.
+ <dt><dfn export for="response body info" id="fetch-timing-info-response-status">response


I think there might be an existing problem here in that an opaque response wouldn't expose body info.

noamr

Non-authoritative LGTM

fetch.bs

This CL introduces a responseStatus field to Performance Resource Timing object. This field is behind a Runtime Enabled Flag. Resource Timing PR : w3c/resource-timing#335 Fetch PR : whatwg/fetch#1468 Bug:1343293 Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b

This CL introduces a responseStatus field to Performance Resource Timing object. This field is behind a Runtime Enabled Flag. Resource Timing PR : w3c/resource-timing#335 Fetch PR : whatwg/fetch#1468 Bug: 1343293 Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923 Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org> Commit-Queue: Abin Paul <abin.paul1@gmail.com> Reviewed-by: Yoav Weiss <yoavweiss@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/main@{#1040902}

This CL introduces a responseStatus field to Performance Resource Timing object. This field is behind a Runtime Enabled Flag. Resource Timing PR : w3c/resource-timing#335 Fetch PR : whatwg/fetch#1468 Bug: 1343293 Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923 Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org> Commit-Queue: Abin Paul <abin.paul1@gmail.com> Reviewed-by: Yoav Weiss <yoavweiss@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/main@{#1040902} Co-authored-by: Abin K Paul <abin.paul1@gmail.com>

…ing, a=testonly Automatic update from web-platform-tests Add response status code to Resource Timing (#34828) This CL introduces a responseStatus field to Performance Resource Timing object. This field is behind a Runtime Enabled Flag. Resource Timing PR : w3c/resource-timing#335 Fetch PR : whatwg/fetch#1468 Bug: 1343293 Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923 Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org> Commit-Queue: Abin Paul <abin.paul1@gmail.com> Reviewed-by: Yoav Weiss <yoavweiss@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/main@{#1040902} Co-authored-by: Abin K Paul <abin.paul1@gmail.com> -- wpt-commits: 9e8b0dbba979679efc9230834ea89cb3b0f06685 wpt-pr: 34828

annevk

If you rebase this I think Build will succeed again. There was a temporary issue that is now resolved.

fetch.bs

annevk · 2022-09-26T11:53:44Z

fetch.bs

+     <li><p>Let <var>responseStatus</var> be 0 if <var>fetchParams</var>'s
+     <a for="fetch params">request</a>'s <a for=request>mode</a> is "<code>navigate</code>" and
+     <var>response</var>'s <a for=response>URL</a>'s <a for=url>origin</a> is not
+     <a>same origin</a> with <var>request</var>'s <a for=request>origin</a>, and
+     <var>response</var>'s <a for=response>status</a> otherwise.


I don't think the same origin check here is good. This doesn't take redirects into account.

Also, is this the internal response or not?

To take redirects into acount, would introducing a new flag to request be a good idea - is navigation same origin. As HTTP fetch step 6.6 we would check if request’s mode is "navigate" and request’s current URL’s origin is same origin with request’s origin. If at any time this is not the case we set is navigation same origin to false.

This flag would then be used instead of the current check. Would this work?

Can we use https://fetch.spec.whatwg.org/#concept-request-tainted-origin? I think request's URL list is still being build up by HTML's navigate algorithm so that should be fine. cc @noamr

Okay, so just to be clear we let responseStatus be 0 if request’s mode is "navigate" and request has a redirect-tainted origin?

Also, is this the internal response or not?

I don't think it is the internal response. We want the filtered response's status, so that status would be 0 if its say a no-cors request as well.

I think so, but now I wonder why this is different from the other measurements. If this is specifically for navigation timing it should probably follow the current model for those. cc @noamr

This is not about navigation timing but about iframe navigations being reported to their parent. Since they cannot be CORS fetches, we don't want to report their status if they are cross origin.

Right, but don't we have existing infrastructure for that which needs to meet the same constraints?

I don't believe we do, but I could be wrong. Are you aware of such infrastructure?

Wait, we have https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigation-params-has-cross-origin-redirects for NavTiming. I can see if/how it fits here.

yoavweiss · 2022-10-04T12:07:48Z

A rough sketch of using existing infrastructure:

navigate an iframe defines reportFrameTiming. It needs to also take in a "has cross origin redirects" boolean and pass it to Fetch's report timing steps
- We need to fix up the frame reporting so that it passes an algorithm that works with the current report timing steps
The passed algorithm would need to nullify the status code in case of a non-same-origin iframe or one that has cross-origin redirects.
In order for that to happen, we'd need to pass "has cross origin redirects" to process response end of body

@abinpaul1 @annevk - does this make sense?

annevk · 2022-10-07T09:52:29Z

Kinda, but then "has cross-origin redirects" needs to be state that Fetch maintains, I suspect. I'm also confused by "navigate an iframe" as I suspect some of those steps also need to apply to embed and object and processing the final response doesn't just result in a report but also needs to actually render something and such.

abinpaul1 · 2022-10-11T12:35:59Z

The information regarding if there were any cross origin redirects can be determined if needed in fetch, but currently its stored in the has cross-origin redirects boolean on navigation params struct in HTML.
I could see this boolean is used mainly in two places ([1], [2]). In both these places we also have access to the response as navigation param's response.
Is it possible to consider moving the has cross-origin redirects boolean as a field to the response? Its value could be determined in the Fetch spec and it could be used as needed in the above two places as well and for our use case as well. This would maybe help us to easily reuse the boolean without having to pass it along to different algorithms.

@annevk @yoavweiss

This CL introduces a responseStatus field to Performance Resource Timing object. This field is behind a Runtime Enabled Flag. Resource Timing PR : w3c/resource-timing#335 Fetch PR : whatwg/fetch#1468 Bug: 1343293 Change-Id: I60d1b6ba00f055481ca526a490144d88ddaf881b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3754923 Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org> Commit-Queue: Abin Paul <abin.paul1@gmail.com> Reviewed-by: Yoav Weiss <yoavweiss@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/main@{#1040902} NOKEYCHECK=True GitOrigin-RevId: fe7385a80df9cb4f7b7bc07609f3ac8228281541

annevk · 2022-10-17T08:33:29Z

Yeah that seems reasonable. And since we copy request's URL list over to the eventual response we might be able to reuse part of https://fetch.spec.whatwg.org/#concept-request-tainted-origin somehow.

(Apologies for taking so long. In the future please bug me on Matrix if I'm not responsive. Or ask the design question there as others might be able to answer as well.)

abinpaul1 · 2022-10-18T15:05:20Z

Have added the has-cross-origin-redirects boolean to the response. I'll also modify html spec to reuse this in place of has cross-origin redirects boolean on navigation params. Building html spec seems to be failing since it can't reference this new boolean.

annevk · 2022-10-19T15:00:32Z

In HTML you have to explicitly add the reference (search for data-x-href). If you cannot figure it out please PR what you do have and ask for help. Should not be too hard to get it all working. (Also, if it was not HTML it would be somewhat expected that CI fails and we just have to land things carefully with some delay so there's time for indexing the new terms.)

annevk

This looks perfect, let me know when there's an HTML PR up.

abinpaul1 · 2022-10-19T18:41:19Z

@annevk Thanks for the pointer. I've opened the HTML PR with the changes. whatwg/html#8405

annevk · 2022-10-21T12:57:27Z

@abinpaul1 can you file bugs on Gecko and WebKit? That's the last thing remaining here.

abinpaul1 · 2022-10-21T14:52:54Z

Filed the bugs

annevk · 2022-10-21T16:28:40Z

fetch.bs

+     <li><p>Let <var>responseStatus</var> be 0 if <var>fetchParams</var>'s
+     <a for="fetch params">request</a>'s <a for=request>mode</a> is "<code>navigate</code>" and
+     <var>response</var>'s <a for=response>has-cross-origin-redirects</a> is true; otherwise
+     <var>response</var>'s <a for=response>status</a>.


I was about to merge this, but I don't understand how this protects the response status when mode isn't "navigate". Shouldn't we be using the timing allow passed flag in that case?

As noted in #1468 (comment), wouldn't the response be a filtered response here thereby protecting the response status?

Fetch PR: whatwg/fetch#1468.

abinpaul1 mentioned this pull request Jul 9, 2022

Add response status code to PerfomanceResourceTiming w3c/resource-timing#335

Merged

yoavweiss reviewed Jul 11, 2022

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

abinpaul1 force-pushed the resource-response-status branch from a8a2650 to 28e9a7c Compare July 11, 2022 11:53

yoavweiss approved these changes Jul 11, 2022

View reviewed changes

noamr reviewed Jul 27, 2022

View reviewed changes

annevk reviewed Jul 28, 2022

View reviewed changes

Pass in response status to mark resource timing

03610e8

abinpaul1 force-pushed the resource-response-status branch from 28e9a7c to 03610e8 Compare July 29, 2022 08:01

abinpaul1 changed the title ~~Add response status to response body info~~ Pass in response status to mark resource timing Jul 29, 2022

noamr approved these changes Jul 29, 2022

View reviewed changes

noamr reviewed Aug 4, 2022

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

Report status for iframes based on request mode and same origin

1ecd43d

chromium-wpt-export-bot mentioned this pull request Aug 30, 2022

Add response status code to Resource Timing web-platform-tests/wpt#34828

Merged

Use response'url to check origin

a298369

abinpaul1 force-pushed the resource-response-status branch from ab74d22 to a298369 Compare September 20, 2022 14:34

annevk reviewed Sep 26, 2022

View reviewed changes

Merge branch 'whatwg:main' into resource-response-status

4a15271

Add has-cross-origin-redirects boolean to response

08d1251

annevk approved these changes Oct 19, 2022

View reviewed changes

annevk added the do not merge yet Pull request must not be merged per rationale in comment label Oct 19, 2022

abinpaul1 mentioned this pull request Oct 19, 2022

Reuse response's has-cross-origin-redirects whatwg/html#8405

Merged

annevk reviewed Oct 21, 2022

View reviewed changes

annevk merged commit 7c30987 into whatwg:main Oct 22, 2022

annevk pushed a commit to whatwg/html that referenced this pull request Oct 22, 2022

Editorial: use response's has-cross-origin-redirects

478e131

Fetch PR: whatwg/fetch#1468.

abinpaul1 deleted the resource-response-status branch October 22, 2022 07:17

annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Mar 2, 2023

		@@ -5536,6 +5539,9 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:

		<li><p>Let <var>status</var> be the HTTP response's status code.

Pass in response status to mark resource timing #1468

Pass in response status to mark resource timing #1468

Conversation

abinpaul1 commented Jul 9, 2022 • edited by pr-preview bot Loading

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss commented Jul 11, 2022

Choose a reason for hiding this comment

abinpaul1 Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abinpaul1 Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss commented Oct 4, 2022

annevk commented Oct 7, 2022

abinpaul1 commented Oct 11, 2022 • edited Loading

annevk commented Oct 17, 2022

abinpaul1 commented Oct 18, 2022 • edited Loading

annevk commented Oct 19, 2022

annevk left a comment

Choose a reason for hiding this comment

abinpaul1 commented Oct 19, 2022

annevk commented Oct 21, 2022

abinpaul1 commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abinpaul1 commented Jul 9, 2022 •

edited by pr-preview bot

Loading

abinpaul1 Jul 27, 2022 •

edited

Loading

abinpaul1 Sep 27, 2022 •

edited

Loading

abinpaul1 commented Oct 11, 2022 •

edited

Loading

abinpaul1 commented Oct 18, 2022 •

edited

Loading