console.timeEnd() always claims 0ms in web workers

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: simon.lindholm10, Assigned: baku)

Tracking

({regression})

unspecified
mozilla32
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30+ fixed, firefox31+ fixed, firefox32+ fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [qa-] )

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Testcase:

data:text/html,<script>w=new Worker("data:,console.time('a'); t=Date.now()+1000; while (Date.now() < t); console.timeEnd('a')")</script>

The web console shows:
a: timer started
a: 0ms

Regressed in Firefox 30 - bisection points to https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58eca03214a6&tochange=8abc76dedec2 and thus bug 965860.
Er, yes.  time() and timeEnd() only set mMonotonicTimer in the mWindow case, which is false on workers.

The old code seems like it was just taking the timings on the main thread, which also seems pretty useless.

Ideally we'd just have something like performance.now() in workers, but failing that, can we just record the TimeStamp when the worker starts and then have time() and timeEnd() send over millisecond durations from that TimeStamp?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

5 years ago
Posted patch timer.patch (obsolete) — Splinter Review
Attachment #8416397 - Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
(Assignee)

Updated

5 years ago
Assignee: nobody → amarchesini
Comment on attachment 8416397 [details] [diff] [review]
timer.patch

r=me, but add a test?
Attachment #8416397 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

5 years ago
Posted patch timer.patchSplinter Review
Test included. It's on try: https://tbpl.mozilla.org/?tree=Try&rev=774f24ed45bd
Attachment #8416397 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8e82e8257789
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Please nominate for uplift so we can get this into this week's Beta.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 8

5 years ago
Comment on attachment 8417878 [details] [diff] [review]
timer.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 965860
User impact if declined: Console.time in workers is broken
Testing completed (on m-c, etc.): mochitest included in the patch.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8417878 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
Andrea, you just nominated it for aurora and not beta. Is that on purpose?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 10

5 years ago
Comment on attachment 8417878 [details] [diff] [review]
timer.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 965860
User impact if declined: Console.time in workers is broken
Testing completed (on m-c, etc.): mochitest included in the patch.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Flags: needinfo?(amarchesini)
Attachment #8417878 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8417878 [details] [diff] [review]
timer.patch

Thanks
We also want it for aurora ;)
Attachment #8417878 - Flags: approval-mozilla-beta?
Attachment #8417878 - Flags: approval-mozilla-beta+
Attachment #8417878 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.