Closed Bug 1350688 Opened 3 years ago Closed 3 years ago

SpinEvent ctor implicitly depends on telemetry eagerly loading osfile.jsm on Windows

Categories

(Core :: IPC, defect)

defect
Not set

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.)
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: nobody → continuation
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
https://hg.mozilla.org/mozilla-central/rev/8c31849e2a06
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.