1191943 - Add workerStart field to ResourceTiming objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[(no longer active)]

Link to the new spec: <https://w3c.github.io/navigation-timing/>

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I need to implement this to get some tests passing.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

There is dev-doc already here:

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/workerStart

But it is wrong.  It says the workerStart value should be shown for worker contexts.  Its really meant to show when the ServiceWorker FetchEvent occurs.

https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-workerstart

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement PerformanceResourceTiming.workerStart. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Copy service worker timing information across redirects. r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update resource-timing.https.html and performance-timeline.https.html WPT test expectations. r=asuth

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a55dcfb0d5823b286c528c97e532c692385aaf1c

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I think we should also check responseEnd and duration in this test.  Right now its possible to pass this test while reporting a 0 duration for everything.  Thats not very helpful.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8915325 [details] [diff] [review]
P1 Implement PerformanceResourceTiming.workerStart. r=asuth

Andrew, this implements the PerformanceResourceTiming.workerStart property.  See:

https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-workerstart

Note, we have to do a few fixups to handle the oddities of our channels changing during internal redirects, etc:

1. The spec wants workerStart to be <= fetchStart.  We map startTime to the current channel AsyncOpen, but we determine if we want to fire the service worker event after that point.  So we basically have to force workerStart to the earlier AsyncOpen time when this happens.
2. Of course, if we ResetInterception, then our workerStart time may be from an earlier channel (after P2 patch).  The spec wants startTime <= workerStart.  So we also have to adjust our startTime method as well.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8915326 [details] [diff] [review]
P2 Copy service worker timing information across redirects. r=asuth

This patch makes us propagate service worker times across redirects.  This is basically what the spec wants since it doesn't consider redirects to be separate entries.  If we end up intercepting multiple times (for a navigation, etc) then we will simply overwrite the previous values.

This won't mess up our telemetry since we fire that explicitly from the nsIInterceptedChannel::SaveTimestamps() call which is triggered explicitly from the service worker code.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

BTW, I think the spec is unclear what to for a few cases with `workerStart`.  I wrote a spec issue:

https://github.com/w3c/resource-timing/issues/118

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> I think we should also check responseEnd and duration in this test.  Right
> now its possible to pass this test while reporting a 0 duration for
> everything.  Thats not very helpful.

I'm going to do this in bug 1351521.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8915327 [details] [diff] [review]
P3 Update resource-timing.https.html and performance-timeline.https.html WPT test expectations. r=asuth

Moar tests.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8915325 [details] [diff] [review]
P1 Implement PerformanceResourceTiming.workerStart. r=asuth

I'm seeing some output from chrome which makes me think the test is being overly strict.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement PerformanceResourceTiming.workerStart. r=asuth

I reworked this a bit to:

1. Clamp FetchStart to the later of WorkerStart/AsyncOpen.
2. Make StartTime the earliest of AsyncOpen/RedirectStart/WorkerStart.

This way sites can see the delay between StartTime and WorkerStart, but still guarantees FetchStart >= WorkerStart.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update resource-timing.https.html and performance-timeline.https.html WPT tests and expectations. r=asuth

I updated the test to verify the responseStart/responseEnd/duration values as well.  I left the workerStart <= FetchStart for now.

Note, I wrote this spec issue to clarify some of this in the future:

https://github.com/w3c/resource-timing/issues/119

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef29c2484cc7d438965dd95482bda31c016cf76c

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8915638 [details] [diff] [review]
P1 Implement PerformanceResourceTiming.workerStart. r=asuth

Please see comment 10 and comment 16.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update resource-timing.https.html and performance-timeline.https.html WPT tests and expectations. r=asuth

Sorry, one more tweak to make sure that respondEnd is actually after fetchStart.  Without this the test still passes when we are lying about the respondEnd value.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8915638 [details] [diff] [review]
P1 Implement PerformanceResourceTiming.workerStart. r=asuth

Review of attachment 8915638 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/performance/PerformanceTiming.cpp
@@ +91,5 @@
> +    // have a strict relation with each other. The truth, however, is the browser
> +    // engages in a number of speculative activities that sometimes mean connections
> +    // and lookups begin at different times. Workaround that here by clamping
> +    // these values to what we expect FetchStart to be.  This means the later of
> +    // AsnycOpen or WorkerStart times.

typo nit: s/Asnyc/Async/

@@ +97,5 @@
> +      // We want to clamp to the expected FetchStart value.  This is later of
> +      // the AsyncOpen and WorkerStart values.
> +      const TimeStamp* clampTime = &mAsyncOpen;
> +      if (!mWorkerStart.IsNull() && mWorkerStart > mAsyncOpen) {
> +        clampTime = & mWorkerStart;

style guide nit that will be mooted by clang-format: should the space between & and mWorkerStart be there?

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make webconsole.js compare messages against navigationStart when determining if service worker messages should be shown. r=bgrins

Moving this patch from bug 1391693 to here.  I'm worried that adjusting the timing values will trigger the same test failures we saw over there without this fix.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement PerformanceResourceTiming.workerStart. r=asuth

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement PerformanceResourceTiming.workerStart. r=asuth

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/980fc3946922
P1 Implement PerformanceResourceTiming.workerStart. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad52be9ea0ae
P2 Copy service worker timing information across redirects. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d492063a52c
P3 Update resource-timing.https.html and performance-timeline.https.html WPT tests and expectations. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5ab89f1a0c
P4 Make webconsole.js compare messages against navigationStart when determining if service worker messages should be shown. r=bgrins

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/980fc3946922
https://hg.mozilla.org/mozilla-central/rev/ad52be9ea0ae
https://hg.mozilla.org/mozilla-central/rev/3d492063a52c
https://hg.mozilla.org/mozilla-central/rev/1a5ab89f1a0c

Comment 26

[Chris Mills [:cmills]]

I've had a go at fixing the wording in the docs - is this ok?

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/workerStart

I also added a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#DOM

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Thanks, but I still find this a bit oddly worded:

"The workerStart read-only property of the PerformanceResourceTiming interface returns a timestamp immediately before the worker that fetches the resource is started — but only when fetched by a Service Worker FetchEvent."

How about:

"The workerStart read-only property of the PerformanceResourceTiming interface returns a timestamp:

  1. Immediately before dispatching the FetchEvent if the Service Worker thread is already running.
  2. Or immediately before starting the Service Worker thread if it is not already running.

If the resource is not intercepted by a Service Worker the workerStart property will always be 0."

Comment 28

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #27)
> Thanks, but I still find this a bit oddly worded:
> 
> "The workerStart read-only property of the PerformanceResourceTiming
> interface returns a timestamp immediately before the worker that fetches the
> resource is started — but only when fetched by a Service Worker FetchEvent."
> 
> How about:
> 
> "The workerStart read-only property of the PerformanceResourceTiming
> interface returns a timestamp:
> 
>   1. Immediately before dispatching the FetchEvent if the Service Worker
> thread is already running.
>   2. Or immediately before starting the Service Worker thread if it is not
> already running.
> 
> If the resource is not intercepted by a Service Worker the workerStart
> property will always be 0."

Cheers Ben - I've rewritten it accordingly to your wording.