1044205 - XPCWrappedNativeScope::TraceSelf isn't invoked for Sandboxes

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

TraceSelf traces the various global objects we have hanging off the XPCWrappedNativeScope - the global object, the XBL scopes, and various addon scopes. And since bug 973780 (FF30), we've started also using it to trace the JS::WeakMapPtr for Xray expandos.

Each WN's trace hook invokes TraceSelf() on its scope, so WN globals are safe. Additionally, CreateGlobalOptions<nsGlobalWindow>::TraceGlobal invokes it, so DOM Windows are safe (on new or old bindings). But the global trace hook for Sandboxes just invokes TraceXBLGlobal, which doesn't invoke TraceSelf.

So if we have a Sandbox that doesn't have a single WN in its scope _and_ places Xray expandos, we're in trouble. I bet this is what's causing bug 1039547.

The fix here should actually be relatively simple. Let me try writing it up.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Detach the XPCWrappedNativeScope from the CompartmentPrivate during forcible shutdown. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Invoke XPCWrappedNativeScope::TraceSelf from TraceXPCGlobal. v1

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e93657a42f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87d020110245
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e251fca362f

Comment 5

[Andrew McCreight [:mccr8]]

Does this affect all versions back to 30?  If so, as a sec-crit bug, it should have gotten sec-approval before landing.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Does this affect all versions back to 30?  If so, as a sec-crit bug, it
> should have gotten sec-approval before landing.

Doh - sorry for the slip-up. :-(

For what it's worth, I'm pretty sure landing it right now would have been the eventual conclusion. We need to get this on trunk, see if it fixes the very conspicuous (and public) stability issues in bug 1039547, and get it backported.

Comment 7

[Bobby Holley (:bholley)]

Kairo, when this hits central, can you check whether it fixes the crashes in bug 1039547?

Comment 8

[Andrew McCreight [:mccr8]]

> see if it fixes the very conspicuous (and public) stability issues in bug 1039547, and get it backported.
Ah, yeah, that makes sense.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a7e93657a42f
https://hg.mozilla.org/mozilla-central/rev/87d020110245
https://hg.mozilla.org/mozilla-central/rev/5e251fca362f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment 5 says this is 30+, but esr24 is marked as affected?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> Comment 5 says this is 30+, but esr24 is marked as affected?

The underlying issue still exists there, but the factors that make it more dangerous (tracing non-globals + GGC) are more recent.

Backporting this to esr24 is probably more risk than it's worth though - I think we should just wontfix it there and embargo for 10 weeks.

Comment 12

[Andrew McCreight [:mccr8]]

I don't see any of these crashes after the 7/28 build, so it looks like this worked.  Please request aurora and beta approval.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8463601 [details] [diff] [review]
Part 2 - Invoke XPCWrappedNativeScope::TraceSelf from TraceXPCGlobal. v1

This approval request applies to all the patches in this bug that landed on m-c (including the one that's not on bugzilla).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding, but exacerbated by bug 973780
User impact if declined: Crashes. This is the top stability issue on 33 (see the dependent bug) 
Testing completed: Landed on m-c, eliminated the crashes. 
Risk to taking this patch (and alternatives if risky): Medium-low risk - this patch changes GC behavior, but does so in a pretty safe way. No alternatives.
String or UUID changes made by this patch: None

Comment 14

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #13)
> User impact if declined: Crashes. This is the top stability issue on 33 (see
> the dependent bug) 

Er, crashes _and_ sec-crits.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8463601 [details] [diff] [review]
Part 2 - Invoke XPCWrappedNativeScope::TraceSelf from TraceXPCGlobal. v1

Bobby, what about ESR 31?

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8463601 [details] [diff] [review]
Part 2 - Invoke XPCWrappedNativeScope::TraceSelf from TraceXPCGlobal. v1

Sorry, meant to flag that as well.

Comment 17

[Bobby Holley (:bholley)]

Ryan, is there a reason you cancelled the b2g approval requests?

Comment 18

[Andrew McCreight [:mccr8]]

32 is beta right now, so anything on beta gets merged to b2g32.  (I don't know about the b2g30 part of that.)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment 18 and sec-crits have auto-approval for b2g30. Bobby, can you jump on IRC and ping me quick?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/59824d8a19aa
https://hg.mozilla.org/releases/mozilla-aurora/rev/29467b1808ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/450f2f3dc306

https://hg.mozilla.org/releases/mozilla-beta/rev/a979f156f8d1
https://hg.mozilla.org/releases/mozilla-beta/rev/7fb3c0ed4417
https://hg.mozilla.org/releases/mozilla-beta/rev/72c66ced503e

Needs rebasing for 30/31.

Comment 21

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/bd707e8210db
remote:   https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/bab00ee58d07
remote:   https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/60d3725443a5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a979f156f8d1
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/7fb3c0ed4417
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/72c66ced503e

Comment 23

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-esr31/rev/a0d2de65ad9f
remote:   https://hg.mozilla.org/releases/mozilla-esr31/rev/143ab340f7a2
remote:   https://hg.mozilla.org/releases/mozilla-esr31/rev/36c0b5bf5bd0

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking this so the QA team will track it for verification in the current cycle.

Comment 25

[Tracy Walker [:tracy]]

There are no longer reports of these crash signatures on builds since this landed, at least on desktop.

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting flags based on Tracy's comment above.