Report iframe & frame resource timing #7531

noamr · 2022-01-25T08:13:21Z

At least two implementers are interested (and none opposed):
- Already implemented, as of Clarify RT entry creation when iframe changes src w3c/resource-timing#316 all implementers are interested in aligning.
Tests are written and can be reviewed and commented upon at:
Implementation bugs are filed:
- Chrome: 1290721
- Safari: 235561, 238246
- Firefox: Not necessary so far

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

/browsing-the-web.html ( diff )
/iframe-embed-object.html ( diff )

domenic

We should avoid introducing more instances of "source browsing context", per #1130. At least ones outside of the synchronous initial computation.

Can you instead compute a boolean synchronously, something like "initiator is container", and then consult that at "report frame timing" time? You can stuff it into navigation params, so that you don't need to pass it around manually.

source

domenic · 2022-02-17T19:03:01Z

source

@@ -87498,6 +87500,7 @@ interface <dfn interface>Location</dfn> { // but see also <a href="#the-location

   <dt><dfn data-x="navigation-params-has-cross-origin-redirects">has cross-origin
   redirects</dfn></dt>
+   <dt><dfn data-x="navigation-params-is-container-initiated">is container initiated</dfn></dt>


I'd prefer these not share a dd; they can each have a separate one just saying "a boolean".

domenic · 2022-02-17T19:03:29Z

source

-  <dfn data-x="navigation-navigationid"><var>navigationId</var></dfn> (default null):</p>
+  "<code data-x="">other</code>"), an optional <span data-x="navigation-id">navigation id</span>
+  <dfn data-x="navigation-navigationid"><var>navigationId</var></dfn> (default null), and an
+  optional boolean <dfn data-x="navigation-iscontainerinitiated">isContainerInitiated</dfn>


If we keep this, put <var> inside the <dfn>. But per the below I think we should calculate it early in the algorithm, instead.

domenic · 2022-02-17T19:04:24Z

source

+  "<code data-x="">other</code>"), an optional <span data-x="navigation-id">navigation id</span>
+  <dfn data-x="navigation-navigationid"><var>navigationId</var></dfn> (default null), and an
+  optional boolean <dfn data-x="navigation-iscontainerinitiated">isContainerInitiated</dfn>
+  (default false):</p>


I think it's better to calculate this using the source browsing context vs. the target browsing context, instead of passing it in as a parameter. As long as we calculate it synchronously.

That would also get you embed and object for free, kinda? At least in the cases where they're behaving like nested browsing contexts. (Which embed doesn't in the spec, but does in practice; see #513 .)

I kind of changed my mind here; see #7554 (review) . We don't want embed/object to trigger this since you're doing it separately over in #7554.

However in that case I think we should rename the parameter. Something like "needs to report resource timing" maybe? Because for embed and object, "is container-initiated" could be true; it's just that we don't want to report resource timing for them since we're doing it separately.

domenic · 2022-02-17T19:05:28Z

source

-       false.</p></li>
+       false, and
+       <span data-x="navigation-params-is-container-initiated">is container initiated</span> is
+       <var>isContainerInitiated</var>.</p></li>


Also need to set it for the javascript: URL case a few lines below this.

domenic · 2022-02-17T19:06:46Z

source

+   <span data-x="navigation-params-is-container-initiated">is container initiated</span> is false,
+   then return.</p></li>
+
+   <li><p><span>Queue an element task</span> on the <span>DOM manipulation task source</span> given


Should it be the DOM manipulation or the networking task source? I feel like most of the others are done from the networking task source.

Not sure it matters much in practice...

domenic · 2022-02-17T19:07:54Z

source

+  given <var>navigationParams</var>, and <span>queue a global task</span> on the
+  <span>networking task source</span> given the newly-created <code>Document</code>'s
+  <span>relevant global object</span> to have the parser process the implied EOF character, which
+  eventually causes a <code data-x="event-load">load</code> event to be fired.</p>


So, do we not report frame timing when navigating to XHTML/img/video/text/etc. documents? I think all those sections will probably need updating too.

noamr · 2022-03-14T17:33:15Z

@domenic, apparently this no longer depends on a change in Fetch, the existing finalize algorithm should suffice as Fetch already performs TAO checks for iframes based on the top-level origin at a different stage.

domenic · 2022-03-14T19:38:16Z

source

-   <span>source browsing context</span> set to <var>element</var>'s <span>node document</span>'s
-   <span data-x="concept-document-bc">browsing context</span>.</p></li>
+   data-x="navigation-hh">historyHandling</var> set to <var>historyHandling</var>,
+   <span data-x="navigation-report-rt-to-container">shouldReportResourceTimingToContainer</span> set


domenic · 2022-03-14T19:40:36Z

source

@@ -87683,6 +87702,7 @@ interface <dfn interface>Location</dfn> { // but see also <a href="#the-location

   <dt><dfn data-x="navigation-params-has-cross-origin-redirects">has cross-origin
   redirects</dfn></dt>
+   <dt><dfn data-x="navigation-params-report-rt-to-container">is container initiated</dfn></dt>
   <dd>a boolean</dd>


Let's stick with keeping dd/dt pairs 1:1 for navigation params, so insert another <dd>a boolean</dd>. Or even better, <dd>a boolean indicating ...</dd> explaining its purpose.

domenic · 2022-03-14T19:40:47Z

source

+  "<code data-x="">other</code>"), an optional <span data-x="navigation-id">navigation id</span>
+  <dfn data-x="navigation-navigationid"><var>navigationId</var></dfn> (default null), and an
+  optional boolean
+  <dfn data-x="navigation-report-rt-to-container">shouldReportResourceTimingToContainer</dfn>


<var> inside the <dfn>

domenic · 2022-03-14T19:44:46Z

source

+
+    <p class="note">This ensures that resource timing is reported to the frame before any parsing
+    and regardless of the type of resource.</p>
+  </li>


Involving the streams machinery here just to get access to the point in time before the final byte seems bad, since it doesn't match implementations. And, I don't think it will work? Once a chunk passes through the transform, it will probably be parsed, and that happens before the flush algorithm.

I'm not really clear how this is supposed to work, in general. We have three events:

A. The resource timing entry is sent
B. The browser parses the bytes, e.g. to do preload scanning or tree construction (which might run scripts)
C. The last byte is read from the network

You seem to be claiming that A must be sent before B, and also that A must occur after C. But B occurs way before C.

Involving the streams machinery here just to get access to the point in time before the final byte seems bad, since it doesn't match implementations. And, I don't think it will work? Once a chunk passes through the transform, it will probably be parsed, and that happens before the flush algorithm.

I'm not really clear how this is supposed to work, in general. We have three events:

A. The resource timing entry is sent

B. The browser parses the bytes, e.g. to do preload scanning or tree construction (which might run scripts)

C. The last byte is read from the network

You seem to be claiming that A must be sent before B, and also that A must occur after C. But B occurs way before C.

Fair enough, will have another think about this...

I think the stream machinery does the right thing, but the comment is inaccurate - the reporting might be done after parsing, but before handling the EOF.

I can think of 3 alternatives:

Keep the current stream foo and update the comment

Add a processResponseEndOfBody callback to the navigation fetch and report iframe there. It does the streams foo internally in fetch. (this somewhat matches implementations)

Add separate wording for each resource type (HTML, XML, etc.)

processResponseEndOfBody sounds best to me!

OK, refactored in this spirit and the PR is significantly smaller :)

Anything still missing @domenic?

source

domenic · 2022-03-22T21:11:29Z

source

@@ -88251,7 +88261,8 @@ interface <dfn interface>Location</dfn> { // but see also <a href="#the-location

      <ol>
       <li><p>If <var>response</var> is null, <!--FETCH--><span
-       data-x="concept-fetch">fetch</span> <var>request</var>.</p></li>
+       data-x="concept-fetch">fetch</span> <var>request</var> with <i>processResponseEndOfBody</i>


This <i> needs to be a link, i.e. <i data-x="...">.

domenic · 2022-03-22T21:13:10Z

source

@@ -88251,7 +88261,8 @@ interface <dfn interface>Location</dfn> { // but see also <a href="#the-location

      <ol>
       <li><p>If <var>response</var> is null, <!--FETCH--><span
-       data-x="concept-fetch">fetch</span> <var>request</var>.</p></li>
+       data-x="concept-fetch">fetch</span> <var>request</var> with <i>processResponseEndOfBody</i>
+       set to <var>processResponseEndOfBody</var>.</p></li>


This variable is not defined inside "process a navigate fetch". It needs to be threaded through from at least one caller, possibly multiple.

Also, this will only call processResponseEndOfBody for the very first fetch, when response is null. Any redirect fetches afterward will be ignored. Is that the desired timing?

Hmm it's not the desired thing. But not sure how else to use processResponseEndOfBody - we will need to make the same "is this a valid redirect" checks inside processEndOfBody.

I'm thinking that perhaps my previous approach of using a transform stream (a previous revision) is the correct way to do this. It's really a replication of a similar stream in https://fetch.spec.whatwg.org/#fetch-finale that we need to do here because we replicate the redirect mechanics.

noamr

Refactored to account for redirects. Not sure if this is simpler/more readable than using a TransformStream, but it would be as correct. WDYT?

source

…ue RT entry See whatwg/html#7531

…ue RT entry (#33317) * Iframes with missing redirect location should fire load event and queue RT entry See whatwg/html#7531 * Rename and use loader

…ue RT entry (web-platform-tests#33317) * Iframes with missing redirect location should fire load event and queue RT entry See whatwg/html#7531 * Rename and use loader

…hould fire load event and queue RT entry, a=testonly Automatic update from web-platform-tests Iframes with missing redirect location should fire load event and queue RT entry (#33317) * Iframes with missing redirect location should fire load event and queue RT entry See whatwg/html#7531 * Rename and use loader -- wpt-commits: 7eb2cb2120e6cd172ad10edb7f38518e765daf49 wpt-pr: 33317

domenic · 2022-03-28T16:39:49Z

source

@@ -88250,12 +88266,32 @@ interface <dfn interface>Location</dfn> { // but see also <a href="#the-location
      <p>Otherwise:</p>

      <ol>
+       <li><p>Let <var>finalResponseReached</var> be false.</p></li>


I find it weird that we're tracking final response-ness both here and in the rest of the loop. For example, that leads to problems where e.g. the censoring done in step 13.5.10 is not reflected; you can use navigation timing to figure out how long responses blocked by CORP take to read their full body. Maybe that is already handled through additional checks in the navigation timing spec? But it seems very duplicative.

Similarly, I'm unsure whether navigation timing will give you results for 204s, 205s, Content-Disposition: attachment downloads, X-Frame-Options failures, or CSP failures. All of those are handled in "process a navigate response", far disconnected from this algorithm.

The cleanest layering I could imagine would be to handle navigation timing around the same time load events are fired, e.g. in https://html.spec.whatwg.org/#navigate-html. But that would preclude firing navigation timing for some error cases, so I don't know if that would be correct. I'd need additional clarity on which error cases you want to include, to give better advice.

How is this related to navigation timing? This PR is about resource timing for the iframe. It should represent the timing used to fetch the iframe resource, and nothing else. The rest of these concerns are indeed about navigation and not related to this...

Also the implementations are in that spirit - an EOF handler on the iframe fetch that reports to the container if the navigation was container initiated.

I admit I misunderstood/misspoke. However, it's a resource being used for navigation, so I think some of those concerns might apply, especially the security concerns about not wanting to leak data if it is blocked.

Here is another angle of approach. I am pretty sure that in implementations, if you fail a CSP check or X-Frame-Options check or CORP check while doing the initial load of an iframe, we don't proceed to download the whole request body just so we can give you accurate resource timing. Whereas your current PR implies that would be done. It isn't meshing with other parts of the spec which indicate such errors by, e.g., overwriting the response variable with a network error.

I'm less sure about Content-Disposition: attachment cases (maybe we do report the amount of time it takes to download the file?) or 204s/205s where the server gives a body anyway despite that being disallowed by HTTP (maybe Fetch will queue processResponseEndOfBody immediately, ignoring the body)? But at least in cases where the current HTML spec ignores the body, I am doubtful implementations read the whole thing just for resource timing.

I see what you mean, indeed X-Frame-Options and CSP navigate failure should terminate the fetch, and report an opaque entry, as if it was a network error, to resource timing. This is indeed covered with WPT: https://wpt.fyi/results/resource-timing/iframe-failed-commit.html

I also added a WPT for 204/205: web-platform-tests/wpt#33402.

I'll amend the spec. I believe we need to be explicit about navigation failure terminating the fetch, which would solve the issue of fetch continuing all the way through, and we can add reporting there.

Actually there is another handled case, an unknown mime-type, which would cause a download. An RT entry should not be reported in that case. Added a test.

This convinces me that the reporting should be done in the different scenarios such as navigate-html navigate-xml etc, like I did in one of the first PRs.

source

…hould fire load event and queue RT entry, a=testonly Automatic update from web-platform-tests Iframes with missing redirect location should fire load event and queue RT entry (#33317) * Iframes with missing redirect location should fire load event and queue RT entry See whatwg/html#7531 * Rename and use loader -- wpt-commits: 7eb2cb2120e6cd172ad10edb7f38518e765daf49 wpt-pr: 33317

noamr · 2022-03-29T12:38:31Z

OK I refactored again to use report inside the mime-type specific handlers. This makes more sense because for non-supported mime-types there is no reporting, and it also takes care of X-Frame-Options and CSP.

One issue that's not properly handled though is TAO checks.
@domenic do you want to see if where I went with this reporting-wise is OK and then I'll add the TAO checks, which is kind of a separate issue, or should I try to add it to the PR first?

Required for whatwg#1221 Blocks whatwg/html#7531 Closes whatwg#1421

This reverts commit b348a9d.

noamr · 2022-03-29T14:14:03Z

OK I refactored again to use report inside the mime-type specific handlers. This makes more sense because for non-supported mime-types there is no reporting, and it also takes care of X-Frame-Options and CSP.

One issue that's not properly handled though is TAO checks. @domenic do you want to see if where I went with this reporting-wise is OK and then I'll add the TAO checks, which is kind of a separate issue, or should I try to add it to the PR first?

OK it's resolved in whatwg/fetch#1422 without need to change anything in the HTML PR.

domenic

Looks good, just editorial stuff at this point

source

noamr

Fixed editorial notes

source

This was referenced Jan 25, 2022

ResourceTiming: Add some scenarios to iframe RT test web-platform-tests/wpt#32527

Merged

Create resource timing entries for HTML resources #6542

Closed

noamr changed the title ~~Report iframe & frame resource timing~~ WIP Report iframe & frame resource timing Jan 25, 2022

noamr force-pushed the report-iframe-resource-timing branch from 3fc29a3 to d803310 Compare January 30, 2022 10:16

noamr mentioned this pull request Jan 30, 2022

Allow timing reporting with origin (for iframes) whatwg/fetch#1388

Closed

noamr changed the title ~~WIP Report iframe & frame resource timing~~ Report iframe & frame resource timing Jan 30, 2022

domenic reviewed Feb 12, 2022

View reviewed changes

source Outdated Show resolved Hide resolved

domenic added the topic: navigation label Feb 12, 2022

noamr force-pushed the report-iframe-resource-timing branch from d803310 to 822607a Compare February 13, 2022 11:15

domenic reviewed Feb 17, 2022

View reviewed changes

noamr mentioned this pull request Feb 17, 2022

Report ResourceTiming for object and embed #7554

Merged

3 tasks

noamr force-pushed the report-iframe-resource-timing branch from 822607a to 9d2b771 Compare February 21, 2022 11:41

noamr force-pushed the report-iframe-resource-timing branch from 9d2b771 to 7464cce Compare March 14, 2022 17:20

domenic requested changes Mar 14, 2022

View reviewed changes

noamr force-pushed the report-iframe-resource-timing branch from 044e827 to ce68ed2 Compare March 16, 2022 06:07

domenic requested changes Mar 22, 2022

View reviewed changes

noamr commented Mar 23, 2022

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

noamr added a commit to web-platform-tests/wpt that referenced this pull request Mar 23, 2022

Iframes with missing redirect location should fire load event and que…

388beb0

…ue RT entry See whatwg/html#7531

domenic reviewed Mar 28, 2022

View reviewed changes

noamr and others added 4 commits March 29, 2022 13:52

Report iframe & frame resource timing

08ceaa4

Refactor out of source-browsing-context

fb33f7c

Refactor EOF for iframe reporting

3767b13

No need for special finalize

a5616dd

noamr added 7 commits March 29, 2022 13:52

nits

9461720

Refactor to use endOfBody

7c68dca

Fixups

ce5e76a

Inline things in IFRAME DONAV

a35bcf3

Fix build

935d765

Refactor to account for redirects correctly

de594fb

Report iframe at end

f3f3a80

noamr force-pushed the report-iframe-resource-timing branch from c64a9af to f3f3a80 Compare March 29, 2022 12:36

nit

b196aa4

noamr added a commit to noamr/fetch that referenced this pull request Mar 29, 2022

Expose TAO-origin check

090a930

Required for whatwg#1221 Blocks whatwg/html#7531 Closes whatwg#1421

noamr mentioned this pull request Mar 29, 2022

Perform TAO check for nested navigations whatwg/fetch#1422

Merged

3 tasks

noamr added 2 commits March 29, 2022 16:59

Incorporate TAO

b348a9d

Revert "Incorporate TAO"

7c6eef0

This reverts commit b348a9d.

domenic reviewed Mar 30, 2022

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

Editorial fixes

4087e4f

noamr commented Mar 30, 2022

View reviewed changes

domenic reviewed Mar 30, 2022

View reviewed changes

source Show resolved Hide resolved

noamr and others added 2 commits March 30, 2022 19:12

Editorial fixes

4751a40

Rewrap

6faa037

domenic approved these changes Mar 30, 2022

View reviewed changes

domenic merged commit 89f9bcb into whatwg:main Mar 30, 2022

This was referenced Mar 30, 2022

Clarify RT entry creation when iframe changes src w3c/resource-timing#316

Closed

TAO check and child frame navigations whatwg/fetch#1221

Closed

Report iframe & frame resource timing #7531

Report iframe & frame resource timing #7531

Uh oh!

Conversation

noamr commented Jan 25, 2022 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr commented Mar 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

noamr commented Mar 29, 2022

Uh oh!

noamr commented Mar 29, 2022

Uh oh!

noamr commented Jan 25, 2022 •

edited by pr-preview bot

Loading

noamr Mar 15, 2022 •

edited

Loading

noamr Mar 29, 2022 •

edited

Loading