1322292 - Performance.now() is broken on workers

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: part 1

We are not following what the spec says. The first patch moves Now() to the Performance.cpp.
The second patch uses creationTime instead of nowBase in workers.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 2

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Well, I don't think the Performance spec acknowledged the existence of workers when we first implemented this.  Still, good to conform if there is a spec now.

Are their wpt tests?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I think bz should review this, since, IIRC, he was dealing with .now() issues in the spec.

Comment 4

[Andrea Marchesini [:baku]]

Bz, can you please take a look at these 2 patches?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8817063 [details] [diff] [review]
part 1

>+  virtual DOMHighResTimeStamp Now() const;

Doesn't need to be virtual anymore, right?

r=me

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8817065 [details] [diff] [review]
part 2

It's not clear to me that the console behavior is right.  Why is it right?  That is, where do these TimeDurations actually get used?  Note that a page and its workers share a console, and showing timestamps with different 0 times in there is a bit confusing.  But that's already a problem to some extent for subframes, I guess?

The comment in Event::TimeStamp no longer matches its implementation.  Which one matches the spec?  r- for this part at the very least, because _something_ in here needs changing.

It's not clear to me that the PerformanceWorker::GetPerformanceTimingFromString change is correct either.  What does the spec say there?

Comment 7

[Andrea Marchesini [:baku]]

> That is, where do these TimeDurations actually get used?

It's used for: console.time('foo'); something(); console.timeEnd('foo'); and you see how long it took to run something().

> and its workers share a console, and showing timestamps with different 0

This is not actually true. console is not shared between workers and windows (or iframes). Each context has its own console object and these time/timeEnd are per context.

> The comment in Event::TimeStamp no longer matches its implementation.  Which
> one matches the spec?  r- for this part at the very least, because
> _something_ in here needs changing.

The spec talks about returning the epoch time (from 1970/01/01). And this is what happens in release. But in our nightly we return Now() - navigationStart (on main-thread). It seems correct to me to return now() - creationTime on workers.
Note that creationTime and nowBaseTime are equal only for the top-level worker. NowBaseTime value for a subWorker is equal to the creationTime of the parent worker.

> It's not clear to me that the
> PerformanceWorker::GetPerformanceTimingFromString change is correct either. 
> What does the spec say there?

For workers navigationStart should be equal to fetchStart, and that is when the WorkerPrivate is created (in theory when the fetch operation starts, but it's almost the same value of the WorkerPrivate creation), so yes, this seems correct. Definitely better than what we had before: we return the value of the creationTime of the parent worker.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> It's used for: console.time('foo'); something(); console.timeEnd('foo');

So is the actual value in "monotonicTimer" never exposed?  Only differences between two values?  That is, it doesn't matter what timestamp we actually subtract as long as we're consistent?  Or is the actual value exposed somewhere?

> console is not shared between workers and windows (or iframes).

I mean the actual console the web developer observes, not the Console object.  Again, if we never show the "monotonicTimer" values in there this doesn't matter.

> The spec talks about returning the epoch time (from 1970/01/01).

Sort of.  It then has a "this part of the spec is a lie, see the issue" box pointing at <https://github.com/whatwg/dom/issues/23>.

I guess following the links from there lands us at "use the same time base as performance.now()" in <https://lists.w3.org/Archives/Public/public-whatwg-archive/2014May/0046.html>, so the code change is correct...  The comment needs fixing.

I wonder whether we can just call through to some method on the worker's Performance object to avoid the code duplication here and centralize the concept of zero time for the worker a bit more...  Maybe worth a followup.

> Note that creationTime and nowBaseTime are equal only for the top-level worker.

I'm not sure what this is trying to say.  Is this a comment on today's code?  In today's code for a top-level dedicated worker nowBaseTime is the same as for its parent page, which is generally different from the creationTime of the worker.

> For workers navigationStart should be equal to fetchStart

Why?  What spec says this and where?  I tried sorting through the rat's nest of specs involved but they have tons of broken links and I can't find anything actually defining this.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> What spec says this and where?

Fwiw, I think the answer is "none; there is absolutely no spec that actually defines the behavior of this API in workers in any implementable way".  See also https://lists.w3.org/Archives/Public/public-web-perf/2016Dec/0007.html

Comment 10

[Andrea Marchesini [:baku]]

> So is the actual value in "monotonicTimer" never exposed?  Only differences
> between two values?  That is, it doesn't matter what timestamp we actually
> subtract as long as we're consistent?  Or is the actual value exposed
> somewhere?

In theory, it's exposed to devtools. But they don't use it. They just use the duration attribute from console.timeEnd('foo').
I'll file a follow up for get rid of 'started' in ConsoleTimerStart.

> > For workers navigationStart should be equal to fetchStart

Yeah, only in w3.org/TR/navigation-timing there is that definition.

https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart
"This attribute must return the time immediately after the user agent finishes prompting to unload the previous document. If there is no previous document, this attribute must return the same value as fetchStart."

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 2

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 3

WorkerPrivate::TimeStampToDOMHighRes()

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Yeah, only in w3.org/TR/navigation-timing there is that definition.

That spec is explicitly not applicable to workers.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8818841 [details] [diff] [review]
part 2

>+++ b/dom/performance/PerformanceWorker.cpp
> PerformanceWorker::GetPerformanceTimingFromString(const nsAString& aProperty)
>   if (aProperty.EqualsLiteral("navigationStart")) {

OK.  So per the spec discussion starting at https://lists.w3.org/Archives/Public/public-web-perf/2016Dec/0007.html this is all wrong.  IsPerformanceTimingAttribute() should just return null for workers, and we shouldn't have this code be reachable at all, presumably.  Please file a followup bug on this, though we should check what other UAs do.

r=me with that bug and the console bug filed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8818860 [details] [diff] [review]
part 3

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/291a68ca4825
Some fixes for the Performance API in workers - part 1 - Centralized Performance.now(), r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/301231f4165a
Some fixes for the Performance API in workers - part 2 - Get rid of NowBaseTimeStamp(), r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b827e4d0dc73
Some fixes for the Performance API in workers - part 3 - TimeStampToDOMHighRes() in workerPrivate, r=bz

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/291a68ca4825
https://hg.mozilla.org/mozilla-central/rev/301231f4165a
https://hg.mozilla.org/mozilla-central/rev/b827e4d0dc73