Closed
Bug 1350688
Opened 8 years ago
Closed 8 years ago
SpinEvent ctor implicitly depends on telemetry eagerly loading osfile.jsm on Windows
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(1 file)
SpinEvent::SpinEvent() appears to be called off the main thread, and to figure out if there is more than one CPU it calls do_GetService(NS_SYSTEMINFO_CONTRACTID), which is implemented by nsSystemInfo. nsSystemInfo::Init can't be called off the main thread (at least on Windows, in whatever conditions cause GetProfileHDDInfo() to return false) because it tries to add an observer, and the observer service can't be called off the main thread. So, this code implicitly depends on nsSystemInfo having been initialized earlier.
Right now, telemetry eagerly loads osfile.jsm early in startup on the main thread, which calls InitOSFileConstants() which calls do_GetService("@mozilla.org/system-info;1") which calls nsSystemInfo::Init(), so it works out.
However, my patch in bug 1349389 makes telemetry load osfile.jsm lazily, so I get Windows crashes in a11y and e10s bc7: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba4d60c46077c53acd03eaadbe17d7dd8b83e87d
Some possible fixes:
- Find some other way for SpinEvent to figure out if there is more than one CPU.
- Force the nsSystemInfo to always start up early on the main thread somehow.
- Dispatch a runnable to the main thread to deal with the observer service.
As a side note, as far as I can tell, the observer service setup in nsSystemInfo is not quite threadsafe, despite using threadsafe refcounting, because the hashtable for the property bag is not guarded by locks. It is possible for the main thread to get the profile-do-change topic and mutate the hashtable while another thread is reading from the hashtable. (Otherwise, the hashtable is only written during nsSystemInfo::Init so it should be okay.)
Assignee | ||
Comment 1•8 years ago
|
||
It looks like in bug 1311834, you were originally going to do some SYSTEM_INFO thing. Can I just use that here? There's also some PR_GetNumberOfProcessors() method I could use.
Flags: needinfo?(aklotz)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → continuation
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e62a0c71982c2e4e163aba90e62e43d45accc7f9
Flags: needinfo?(aklotz)
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8851365 [details]
Bug 1350688 - Use a thread safe way to get the CPU count in the SpinEvent ctor.
https://reviewboard.mozilla.org/r/123672/#review126394
lgtm, thanks!
Attachment #8851365 -
Flags: review?(aklotz) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c31849e2a06
Use a thread safe way to get the CPU count in the SpinEvent ctor. r=aklotz
Comment 6•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•