Prepare for FETCH integration #261

noamr · 2021-03-17T13:39:34Z

Remove duplicates that are moved to FETCH
Add an exported algorithm to mark resource timing

See whatwg/fetch#1185 and #252

Preview | Diff

noamr · 2021-03-17T13:40:01Z

@yoavweiss @annevk: this is how the FETCH integration would look like from the RT side.

- Remove duplicates that are moved to FETCH - Add an exported algorithm to mark resource timing

noamr · 2021-03-17T14:08:20Z

Some explanation:

RT receives a response and a global object, and is responsible for queuing, dealing with the performance buffers, and coarsening/adjusting the response timestamps to the global time origin. The timestamps themselves are defined in FETCH.
FETCH is responsible for TAO - RT calls an algorithm retrieve timing info for response, which sanitizes the timing info returned to account for TAO.
Everything is done in getters to an associated fetch timing info - this allows for cases like navigation timing (or future cases of ResourceTiming) where the timestamps for an entry might be updated after the entry is created.

When response is a network error or body is fully read, finalize and report the response. Depends on whatwg/fetch#1185 See: w3c/resource-timing#261 and w3c/resource-timing#252

annevk

I really like how this becomes an API just returning values from a model rather than a bunch of prose you have to mentally parse into a model.

One thing I realized is that with our current approach to timers we leave the raw time vulnerable to Spectre attacks. Implementations can probably invert it, but hmm.

annevk · 2021-03-18T09:18:54Z

index.html

-<li>If the fetched resource results in an HTTP redirect or
-<a data-cite="HTML#or-equivalent" data-lt=
-'HTTP response codes equivalence'>equivalent</a>, then
+<li><a>Queue a global task</a> on the <a>networking task source</a> with


Why do we need a task here? Aren't we already on a task so this can be done synchronously?

Not necessarily. Someone needs to queue a task on the global object. We can have FETCH queue this task on the taskDestination instead, but there is no guarantee that it's the global object.
Since we anyway need a global object for the time origin, I thought making this re-entrant and queue it here would be right, but open to do this in a different way.

yoavweiss

One thing I realized is that with our current approach to timers we leave the raw time vulnerable to Spectre attacks. Implementations can probably invert it, but hmm.

Do you mean that we leave the raw timers in the renderer's memory, and a Spectre attacker can use low resolution timers to steal high-resolution timers and amplify the attacks?

index.html

yoavweiss · 2021-03-18T09:59:04Z

index.html

+response end time</a> minus <a>this</a>'s
+<p data-dfn-for="PerformanceResourceTiming">On getting, the <dfn>workerStart</dfn> attribute
+returns the result of calling <a>convert fetch timestamp</a> for <a>this</a>'s
+<a data-for="PerformanceResourceTiming">timing info</a>'s <a for="fetch timing info">


Might be interesting to abstract the boilerplate away from all these getters, but I'm not sure if/how we can do that. @annevk - thoughts?

The modern style would be:

The X getter steps are to return the result of calling ...

For the latter part, we'd have to do the conversion earlier somehow.

index.html

mikewest · 2021-03-18T15:12:47Z

One thing I realized is that with our current approach to timers we leave the raw time vulnerable to Spectre attacks.

This seems true, but I'm not sure I see the risk. If the attacker has a timer that's good enough to use Spectre-like attacks to access the raw timer, then they don't need the raw timer in the first place.

annevk · 2021-03-18T15:50:51Z

In theory raw timers could be used for cross-process attacks. In practice this is probably infeasible, but it's always nice if the model is the best it can be. There might also be some fingerprinting risks. If I help out figuring out how we can do this, would you be okay with it? (It seems to me that fetch has access to the cross-origin isolated capability through the client so we could use that to obtain a time in the granularity we need, although this will need a new hook.)

Note that this would also solve the boilerplate issue @yoavweiss mentioned above.

mikewest · 2021-03-18T15:58:38Z

I'm not sure it's essential, but if there's a reasonable way to spec things such that we don't need to store the information in a renderer, wonderful! I'd certainly support that, and as you say, it does seem possible for resource timing.

noamr · 2021-03-18T16:14:00Z

In theory raw timers could be used for cross-process attacks. In practice this is probably infeasible, but it's always nice if the model is the best it can be. There might also be some fingerprinting risks. If I help out figuring out how we can do this, would you be okay with it? (It seems to me that fetch has access to the cross-origin isolated capability through the client so we could use that to obtain a time in the granularity we need, although this will need a new hook.)

You mean, coarsen the timestamps earlier and not when they're accessed through the getter?

annevk · 2021-03-18T16:21:53Z

Yeah, exactly.

Fetch needs to obtain time values from in parallel and would like to avoid having to store raw time values and hand these over to "attacker" agent clusters. I also changed some unordered lists to ordered lists as they are sequences of steps. (As discussed in w3c/resource-timing#261.)

annevk · 2021-03-24T15:47:25Z

index.html

+<a>global object</a> |global|, do the following:
+<ol>
+<li>If |ts| is zero, return zero.
+<li>Otherwise, return the <a>relative high resolution time</a> given |ts| and |global|.


This now coarsens |ts| twice. I'm not sure if that's a problem?

It's not. But I think it doesn't need to be re-coarsened. Let me check if it can be improved

Yeah, coarsening twice is not ideal. Maybe we can expose the diff algorithm from the timestamps to the timeorigin as a separate algorithm?

Why not. once w3c/hr-time#114 is in I'll amend this one.

annevk · 2021-03-25T10:43:54Z

nextHopProtocol should also become a ByteString.

index.html

yoavweiss · 2021-03-25T12:04:28Z

index.html

+<a>global object</a> |global|, do the following:
+<ol>
+<li>If |ts| is zero, return zero.
+<li>Otherwise, return the <a>relative high resolution time</a> given |ts| and |global|.


Yeah, coarsening twice is not ideal. Maybe we can expose the diff algorithm from the timestamps to the timeorigin as a separate algorithm?

This defines the foundation for the PerformanceResourceTiming API and its properties. Note that the names used here are not a 1:1 match, e.g., post-redirect start time is fetchStart in the API. Note that Fetch doesn't decide when to create and enqueue PerformanceResourceTiming objects, but rather creates a data structure that holds the different time values, and it's up to the caller to invoke the newly-added finalize and report timing. Fetch also has a new callback, processResponseDone, for when the response errors or EOFs, which lets callers like the fetch() API invoke finalize and report timing at the right time. This change also makes fetch() adopt that. It's the first phase for addressing w3c/resource-timing#252, which contains a link to the other required steps to address Resource Timing/Navigation Timing/HTML/Fetch integration. Corresponding Resource Timing PR: w3c/resource-timing#261. (Once that lands there'll be a subsequent change to Fetch to resolve the remaining TODO.) Co-authored-by: Anne van Kesteren <[email protected]>

For resource-timing, we pre-coarsen the timestamps before they can be converted to relative timestamps, so the inermediate algorithm to calculate the relative timestamp from a pre-coarsened absolulte timestamp is needed. See w3c/resource-timing#261

index.html

yoavweiss · 2021-03-29T09:02:20Z

index.html

-  <ol>
-    <li>If the resource's <a>Request</a>'s
-    <a data-cite="Fetch#concept-request-destination">destination</a> equals to
-    "document", set <a>workerStart</a> to zero.


Here I believe the Fetch integration resulted in setting workerStart to zero for all destinations. I think it's cleaner, but it's a behavior change so we want to make sure implementations align with that.

We should file bugs and add tests on that front.

/cc @wanderview in case he has opinions given past discussion

Can you open a Resource Timing issue on the fact that we need better tests here to verify the new behavior? (as well as reporting that as an implementation issues)

index.html

yoavweiss · 2021-03-29T14:03:56Z

index.html

+ section.</p>
+<section id="sec-timing-allow-origin"> 
+ <h4><code>Timing-Allow-Origin</code> Response Header</h4>
+<p class="note">This section is non-normative.</p>


This section should be normative.

yoavweiss · 2021-03-29T14:05:22Z

index.html

-separated by a comma.</p>
-<p>The <dfn>timing allow check</dfn> algorithm, which checks
+ <h3>Cross-origin Resources</h3>
+ <p class="note" data-dfn-for="PerformanceResourceTiming">As detailed in [=fetch=],


Nit: s/fetch/Fetch/ ?

yoavweiss · 2021-03-29T14:05:59Z

index.html

+ cross-origin resources are included as <a>PerformanceResourceTiming</a> objects in the
+ <a data-cite="PERFORMANCE-TIMELINE-2#performance-timeline">Performance
+ Timeline</a>. If the <a href="FETCH#concept-tao-check">timing allow check</a> algorithm
+ fails for a resource, these attributes of its <a>PerformanceResourceTiming</a>


Nit: s/these/the following/

index.html

noamr · 2021-03-29T15:02:45Z

Opened HTML issue whatwg/html#6542 to track specific elements that should/shouldn't be integrated.

npm1

Looks great! Pointing out some nits

npm1 · 2021-03-29T20:19:35Z

index.html

@@ -254,7 +259,7 @@ <h3>Resources Included in the <a>PerformanceResourceTiming</a>
 <a data-cite=
 "PERFORMANCE-TIMELINE-2#performance-timeline">Performance
 Timeline</a>, unless excluded from the timeline as part of the <a
-href="#processing-model">processing model</a>.
+href="FETCH#concept-fetch">fetching process</a>.


This link does not seem to work in the Preview. Is this a Preview bug or does it need to be fixed?

npm1 · 2021-03-29T20:24:42Z

index.html

-"FETCH#concept-fetch">fetch</a> redirected to a different URL.</dd>
+<dt><dfn>name</dfn>
+<dd>The <a>name</a> getter steps are to return the
+<a data-for="PerformanceResourceTiming">requested URL</a>


The requested URL of who? I think you need to use this. Also, nit: no period at end of sentence.

npm1 · 2021-03-29T20:25:38Z

index.html

-Otherwise, this attribute MUST return the same value as
-<a>fetchStart</a>.</dd>
+<dd><p data-dfn-for="PerformanceResourceTiming">The <a>startTime</a> getter
+steps are to <a>convert fetch timestamp</a> for <a>this</a>'


nit: sadly it would be this's, here and throughout.

npm1 · 2021-03-29T20:28:03Z

index.html

-respectively.</dd>
+<dd>
+<p data-dfn-for="PerformanceResourceTiming">The <a>duration</a> getter
+steps are to <a>convert fetch timestamp</a> for <a>this</a>'


convert fetch timestamp receives two parameters, so you need to add the relevant global in both uses here. Alternatively, maybe it always uses the context object's global object so you could get rid of the second parameter there?

npm1 · 2021-03-29T20:29:59Z

index.html

-"HTML#resolve-a-url">resolved URL</a> of the requested resource.
-This attribute MUST NOT change even if the <a data-cite=
-"FETCH#concept-fetch">fetch</a> redirected to a different URL.</dd>
+<dt><dfn>name</dfn>


It seems more logical to me to place this after the IDL and after announcing the associated values to the object (some of them used in here). So basically I suggest moving this to right before JSON

npm1 · 2021-03-29T20:31:18Z

index.html

+is called, run [[WEBIDL]]'s <a data-cite=
+"WEBIDL#default-tojson-operation">default toJSON operation</a>.</p>
+<p data-dfn-for="PerformanceResourceTiming">
+<dfn>initiatorType</dfn> getter steps would be to return the


nit: "The initiatorType getter steps are to return", for consistency with the rest?

npm1 · 2021-03-29T20:32:46Z

index.html

+info</a>'s [=fetch timing info/final connection timing info=]'s
+[=connection timing info/domain lookup start time=] and the <a>relevant global object</a> for
+<a>this</a>.
+See <a data-cite="FETCH#concept-recording-connection-timing-info">Recording connection timing


I guess this link will only work after landing the FETCH part? Not currently linking to anything

npm1 · 2021-03-29T20:42:11Z

index.html

 explicitly allow all timing information to be collected for a
-resource by adding the <a>Timing-Allow-Origin</a> HTTP response
+resource by adding the [=request/timing allow failed flag|Timing Allow Origin=] HTTP response


This is the wrong link. You're talking about a header but linking to a flag.

npm1 · 2021-03-29T20:43:06Z

index.html

 header, which specifies the domains that are allowed to access the
 timing information.</p>
 <p>Statistical fingerprinting is a privacy concern where a
 malicious web site may determine whether a user has visited a
 third-party web site by measuring the timing of cache hits and
 misses of resources in the third-party web site. Though the
 <a>PerformanceResourceTiming</a> interface gives timing information
-for resources in a document, the <a href=
-"#sec-cross-origin-resources">cross-origin restrictions</a> prevent
+for resources in a document, the <a data-cite="FETCH#">cross-origin restrictions</a> prevent


ditto does this link work?

npm1

Looks good to me

npm1 · 2021-03-30T15:49:21Z

index.html

+ <p class="note" data-dfn-for="PerformanceResourceTiming">As detailed in [=Fetch=],
+ cross-origin resources are included as <a>PerformanceResourceTiming</a> objects in the
+ <a data-cite="PERFORMANCE-TIMELINE-2#performance-timeline">Performance
+ Timeline</a>. If the <a href="FETCH#concept-tao-check">timing allow check</a> algorithm


Link doesn't seem to work

npm1 · 2021-03-30T15:55:20Z

index.html

-       "FETCH#concept-response-timing-allow-passed">
-       timing allow passed flag</a>.</li>
- </ol>
+ <p>The <a>Timing-Allow-Origin</a> check is performed as part of


TAO is a header, not a check, so you may want to word this more precisely (the headers are processed in fetch to compute the attributes accordingly?)

yoavweiss

LGTM % nits and comments

yoavweiss · 2021-04-01T15:26:00Z

index.html


+<p>A <a>PerformanceResourceTiming</a> has an associated DOMstring


s/DOMstring/DOMString/

yoavweiss · 2021-04-01T22:12:04Z

index.html

-the header's value may come from the revalidation response, or if not present
-there, from the original cached resource.</p>
-</section>
-<section id="sec-iana-considerations">


Can you bring back the IANA considerations section?

yoavweiss · 2021-04-01T22:15:11Z

index.html

-  <ol>
-    <li>If the resource's <a>Request</a>'s
-    <a data-cite="Fetch#concept-request-destination">destination</a> equals to
-    "document", set <a>workerStart</a> to zero.


Can you open a Resource Timing issue on the fact that we need better tests here to verify the new behavior? (as well as reporting that as an implementation issues)

When response is a network error or body is fully read, finalize and report the response. Depends on whatwg/fetch#1185 See: w3c/resource-timing#261 and w3c/resource-timing#252

When response is a network error or body is fully read, finalize and report the response. Depends on whatwg/fetch#1185. Also see w3c/resource-timing#261 and w3c/resource-timing#252.

WIP: Prepare for FETCH integration

f29bcf9

- Remove duplicates that are moved to FETCH - Add an exported algorithm to mark resource timing

noamr force-pushed the fetch-integ branch from cebc5ec to f29bcf9 Compare March 17, 2021 13:58

noamr mentioned this pull request Mar 17, 2021

Resource Timing integration whatwg/xhr#319

Merged

Added links into FETCH for info

29e3e4f

annevk reviewed Mar 18, 2021

View reviewed changes

yoavweiss reviewed Mar 18, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

yoavweiss reviewed Mar 18, 2021

View reviewed changes

PR fixes

9f31905

annevk mentioned this pull request Mar 19, 2021

Add coarsened shared current time w3c/hr-time#112

Merged

Remove task queueing bit

7eef4e7

noamr changed the title ~~WIP: Prepare for FETCH integration~~ Prepare for FETCH integration Mar 24, 2021

Apply relevant FETCH changes

f06f880

annevk reviewed Mar 24, 2021

View reviewed changes

yoavweiss reviewed Mar 25, 2021

View reviewed changes

annevk mentioned this pull request Mar 25, 2021

Current definition of startTime exposes total redirect time even when TAO fails #260

Closed

noamr mentioned this pull request Mar 25, 2021

Expose "relative high resolution coarse time" w3c/hr-time#114

Merged

Use cites until respec is up to date

561f6ef

Resolve refs

4e5067e

noamr mentioned this pull request Mar 29, 2021

4.6 Integrate the processing model with Fetch #252

Closed

16 tasks

yoavweiss reviewed Mar 29, 2021

View reviewed changes

noamr added 5 commits March 29, 2021 13:08

Address PR comments

0748cca

Reinstate TAO

da27272

Reinstate TAO

8b58f1b

Make paragraphs into notes

4deeb20

Make paragraphs into notes

46e3f8f

yoavweiss reviewed Mar 29, 2021

View reviewed changes

nits

bc47c39

npm1 reviewed Mar 29, 2021

View reviewed changes

noamr added 2 commits March 30, 2021 08:39

Fix nits

9575d8f

Fix nits

8059f4b

npm1 approved these changes Mar 30, 2021

View reviewed changes

Fix broken links and wording

937c5c0

yoavweiss approved these changes Apr 1, 2021

View reviewed changes

noamr added 2 commits April 2, 2021 09:11

Bring back IANA

f416759

indent

ce74048

noamr merged commit 837ca19 into gh-pages Apr 2, 2021

noamr deleted the fetch-integ branch April 2, 2021 06:16

This was referenced Jun 22, 2021

Initiator is not well defined #69

Closed

Definition of initiatorType removed #276

Closed


		<p>A <a>PerformanceResourceTiming</a> has an associated DOMstring

Prepare for FETCH integration #261

Prepare for FETCH integration #261

Conversation

noamr commented Mar 17, 2021 • edited by pr-preview bot Loading

noamr commented Mar 17, 2021

noamr commented Mar 17, 2021

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

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

mikewest commented Mar 18, 2021

annevk commented Mar 18, 2021 • edited Loading

mikewest commented Mar 18, 2021

noamr commented Mar 18, 2021

annevk commented Mar 18, 2021

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

annevk commented Mar 25, 2021

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

Choose a reason for hiding this comment

noamr commented Mar 29, 2021

npm1 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

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

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

Choose a reason for hiding this comment

npm1 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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

noamr commented Mar 17, 2021 •

edited by pr-preview bot

Loading

annevk commented Mar 18, 2021 •

edited

Loading