Closed Bug 1191943 Opened 9 years ago Closed 7 years ago

Add workerStart field to ResourceTiming objects

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: nhughes, Assigned: bkelly)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 5 obsolete files)

3.11 KB, patch
asuth
: review+
Details | Diff | Splinter Review
3.61 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.65 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
11.63 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
      No description provided.
I need to implement this to get some tests passing.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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
Keywords: dev-doc-needed
Depends on: 1405739
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 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.
Attachment #8915325 - Flags: review?(bugmail)
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.
Attachment #8915326 - Flags: review?(bugmail)
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
(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 on attachment 8915327 [details] [diff] [review]
P3 Update resource-timing.https.html and performance-timeline.https.html WPT test expectations. r=asuth

Moar tests.
Attachment #8915327 - Flags: review?(bugmail)
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.
Attachment #8915325 - Flags: review?(bugmail)
Attachment #8915327 - Flags: review?(bugmail)
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.
Attachment #8915325 - Attachment is obsolete: true
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
Attachment #8915327 - Attachment is obsolete: true
Attachment #8915641 - Flags: review?(bugmail)
Comment on attachment 8915638 [details] [diff] [review]
P1 Implement PerformanceResourceTiming.workerStart. r=asuth

Please see comment 10 and comment 16.
Attachment #8915638 - Flags: review?(bugmail)
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.
Attachment #8915641 - Attachment is obsolete: true
Attachment #8915641 - Flags: review?(bugmail)
Attachment #8915703 - Flags: review?(bugmail)
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?
Attachment #8915638 - Flags: review?(bugmail) → review+
Attachment #8915326 - Flags: review?(bugmail) → review+
Attachment #8915703 - Flags: review?(bugmail) → review+
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.
Attachment #8916030 - Flags: review+
Attachment #8915638 - Attachment is obsolete: true
Attachment #8916033 - Flags: review+
Pushed by bkelly@mozilla.com:
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
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
Flags: needinfo?(bkelly)
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."
Flags: needinfo?(bkelly) → needinfo?(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.
Flags: needinfo?(cmills)
Depends on: 1415630
You need to log in before you can comment on or make changes to this bug.