Crash in performance.timeOrigin

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
a year ago
13 days ago

People

(Reporter: tjr, Assigned: baku)

Tracking

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox58+ fixed, firefox59 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Accessing performance.timeOrigin in a worker crashes in some cases. It's hard to reproduce - I was unable to do it on Linux, but running locally on a Mac I can get the following test (from Bug 1424341) to reliably crash.

I can't get a nice callstack anymore, but here it is manually:

GECKO(28216) | Assertion failure: NS_IsMainThread(), at /Users/tritter/Documents/moz/mozilla-unified/obj-x86_64-apple-darwin17.2.0/dist/include/mozilla/ClearOnShutdown.h:102

https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/xpcom/base/ClearOnShutdown.h#102
https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/dom/performance/PerformanceService.cpp#26
https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/dom/performance/Performance.cpp#135

It seems like a relatively straightforward error, but I'm not sure what the correct resolution is.
(Nothing to do with DOM Events.)
Component: DOM: Events → DOM
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

a year ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8937464 - Flags: review?(bkelly)
Comment on attachment 8937464 [details] [diff] [review]
preferenceService initialized in RuntimeService

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

Looks ok.  It might be slightly nicer to make a PerformanceService::Init() called from our process startup code and then just a PerformanceService::Get() instead of GetOrCreate().  Or even a PerformanceService::Init() and a static method for TimeOrigin().
Attachment #8937464 - Flags: review?(bkelly) → review+

Comment 4

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59658470f635
Initialize PerformanceService in RuntimeService, r=bkelly
(Reporter)

Comment 5

a year ago
FWIW, I can confirm this fixed my problem :)

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59658470f635
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Reporter)

Comment 7

a year ago
[Tracking Requested - why for this release]: This is required for Bug 1424341 to land in 58

Existing patch applies cleanly to -beta for me.
status-firefox58: --- → affected
tracking-firefox58: --- → ?
Flags: needinfo?(rkothari)
Flags: needinfo?(rkothari) → needinfo?(gchang)
Track 58+ as this is required by bug 1424341.
tracking-firefox58: ? → +
Flags: needinfo?(gchang)
Hi Tom, since this is required by bug 1424341, please nominate beta uplift request.
Flags: needinfo?(tom)
(Reporter)

Comment 10

a year ago
Comment on attachment 8937464 [details] [diff] [review]
preferenceService initialized in RuntimeService

Approval Request Comment
[Feature/Bug causing the regression]: We would like the ability to easily tune timer precision on beta for the next release cycle to respond to Spectre. The pref is turned off by default right now.

[User impact if declined]: We will have to do dot releases instead of doing a system addon to tweak a pref. Also, the dot releases will be more work (much more work if we don't uplift this in one of them) to cover all timers - previously our chemspill was only to address performance.now()

[Is this code covered by automated tests?]: Yes (in fact it fixes a test and stops it from crashing.)

[Has the fix been verified in Nightly?]: No

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: This is a dependency for 1424341

[Is the change risky?]: No

[Why is the change risky/not risky?]: This is a small patch that initializes an object (if i is not already initialized) on WorkerThreads.

[String changes made/needed]: None
Flags: needinfo?(tom)
Attachment #8937464 - Flags: approval-mozilla-beta?
(Reporter)

Comment 11

a year ago
Sorry this has been verified in Nightly actually.
Comment on attachment 8937464 [details] [diff] [review]
preferenceService initialized in RuntimeService

Required by bug 1424341. Beta58+.
Attachment #8937464 - Flags: approval-mozilla-release+
Attachment #8937464 - Flags: approval-mozilla-beta?
Attachment #8937464 - Flags: approval-mozilla-beta+
(Reporter)

Updated

a year ago
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.