mozilla::dom::TraceBlackJS can call GetService recursively

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted 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]
dont-gc-in-trace-black-js

>+bool
>+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 jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1e88230672
Remove possiblity of recursive GetService call when tracing r=smaug
https://hg.mozilla.org/mozilla-central/rev/cf1e88230672
Status: NEW → RESOLVED
Closed: Last year
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.