Closed Bug 911303 Opened 6 years ago Closed 6 years ago

Remove manual destroy routine from CycleCollectedJSRuntime

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

In bug 905926, we had to expose a mechanism for CycleCollectedJSRuntime to destroy its JSRuntime earlier than its destructor, since destroying the runtime invokes a rambo GC which can invoke various finalizers, which can call back into XPConnect after the XPCJSRuntime destructor runs.

This is problematic for two reasons. First, if we still have various JSRuntime callbacks registered, we're going to have a bad time. This is fixable with a simple patch, which I'll attach. The other problem is that things like XPCWrappedJS::Release currently need to acquire the map lock, which gets destroyed in ~XPCJSRuntime.

Once those map locks go away in bug 770535, we can take another crack at removing this.
We also null out the deleted maps to make this kind of bug easier to spot. This
is part (1) from above.
Looks like version 1 is green. Let's try all platforms:

https://tbpl.mozilla.org/?tree=Try&rev=c81c18140ea1
Some infra rockiness in the try run, but otherwise this looks good. Flagging
for review.
Attachment #798077 - Attachment is obsolete: true
Attachment #8338947 - Flags: review?(wmccloskey)
Comment on attachment 8338947 [details] [diff] [review]
Remove manual Destroy() routine from CycleCollectedJSRuntime. v1

Review of attachment 8338947 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. I'm glad this works now.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1517,5 @@
>  }
>  
>  XPCJSRuntime::~XPCJSRuntime()
>  {
> +    // This (inherited) destructor runs before ~CycleCollectedJSRuntime, which

Could you take out "(inherited)"? At first I read it as "The inherited destructor runs before ..." which is the opposite of what you mean.
Attachment #8338947 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/50dcaa9d8d1a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.