Fix potential race in RegisterJSRuntime

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

This method uses a static bool to detect if the CC has been registered as a memory reporter yet. We now have CC on workers in addition to the main thread, so potentially multiple threads are accessing this single variable. I think it isn't a problem in practice because the main thread's CC runtime should be completely started up before we start any worker CC runtime, but it still feels questionable. I also am not sure why this boolean is needed, as RegisterJSRuntime should only be called once per CC.
Comment on attachment 8790780 [details]
Bug 1301796 - Remove racy code in nsCycleCollector::RegisterJSRuntime.

https://reviewboard.mozilla.org/r/78446/#review76968

And we don't want worker CCRuntimes to be registered or something?
(but that would be a different bug)
Attachment #8790780 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #3)
> And we don't want worker CCRuntimes to be registered or something?
> (but that would be a different bug)

I'm not entirely sure how memory reporter registration workers for non-mainthread. Possibly it just works. Anyways, I already have bug 1283251 on file.
Blocks: 1283251
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f7f9fa31c4a
Remove racy code in nsCycleCollector::RegisterJSRuntime. r=smaug
https://hg.mozilla.org/mozilla-central/rev/0f7f9fa31c4a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.