911303 - Remove manual destroy routine from CycleCollectedJSRuntime

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Remove runtime callbacks in XPCJSRuntime destructor. v1

We also null out the deleted maps to make this kind of bug easier to spot. This
is part (1) from above.

Comment 2

[Bobby Holley (:bholley)]

Two version:

https://tbpl.mozilla.org/?tree=Try&rev=b316eba396e1
https://tbpl.mozilla.org/?tree=Try&rev=1b435856c535

Comment 3

[Bobby Holley (:bholley)]

Looks like version 1 is green. Let's try all platforms:

https://tbpl.mozilla.org/?tree=Try&rev=c81c18140ea1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Remove manual Destroy() routine from CycleCollectedJSRuntime. v1

Some infra rockiness in the try run, but otherwise this looks good. Flagging
for review.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50dcaa9d8d1a

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50dcaa9d8d1a