Closed Bug 1453986 Opened 4 years ago Closed 4 years ago

mozilla::dom::TraceBlackJS can call GetService recursively


(Core :: DOM: Core & HTML, enhancement, P2)




Tracking Status
firefox61 --- fixed


(Reporter: jonco, Assigned: jonco)



(1 file, 1 obsolete file)

When testing with GC zeal enabled I see errors where GetService is called recursively.  This happens because TraceBlackJS() calls nsFrameMessageManager::GetChildProcessManager() which will attempt to create the child process message manager service if it doesn't already exist.  Doing so can trigger another GC.

I've only seen this with GC zeal enabled, but in theory this could happen due to (very?) low memory conditions during startup.
Attached patch dont-gc-in-trace-black-js (obsolete) — Splinter Review
Here's a patch I've been using to work around this.  I don't know if this is the right fix.
Olli, is that patch a sensible approach?  I'm not sure whether this problem will occur in normal operation but I want to make it easier to test the browser with GC zeal enabled.
Flags: needinfo?(bugs)
Comment on attachment 8967761 [details] [diff] [review]

>+ProcessGlobal::WasCreated() {
{ goes to its own line

> }
> private:
>   bool mInitialized;
>+  static bool mWasCreated;
This should be called sWasCreated.
s-prefix for static variables

>-  if (nsFrameMessageManager::GetChildProcessManager()) {
>+  if (nsFrameMessageManager::GetChildProcessManager() && ProcessGlobal::WasCreated()) {
Don't you want to check ProcessGlobal::WasCreated() first?

But yeah, looks reasonable.
Flags: needinfo?(bugs)
Cheers for the feedback.  Here's an updated patch with comments applied.
Assignee: nobody → jcoppeard
Attachment #8967761 - Attachment is obsolete: true
Attachment #8970916 - Flags: review?(bugs)
Attachment #8970916 - Flags: review?(bugs) → review+
Priority: -- → P2
Pushed by
Remove possiblity of recursive GetService call when tracing r=smaug
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.