Closed Bug 1044205 Opened 5 years ago Closed 5 years ago

XPCWrappedNativeScope::TraceSelf isn't invoked for Sandboxes

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox31 --- wontfix
firefox32 + verified
firefox33 + verified
firefox34 + verified
firefox-esr24 + wontfix
firefox-esr31 + fixed
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: sec-critical, Whiteboard: [embargo until esr24 eol][adv-main32-][adv-esr31.1-])

Attachments

(2 files)

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.
Attachment #8463600 - Flags: review?(wmccloskey) → review+
Attachment #8463601 - Flags: review?(wmccloskey) → review+
Does this affect all versions back to 30?  If so, as a sec-crit bug, it should have gotten sec-approval before landing.
(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.
Kairo, when this hits central, can you check whether it fixes the crashes in bug 1039547?
Flags: needinfo?(kairo)
> see if it fixes the very conspicuous (and public) stability issues in bug 1039547, and get it backported.
Ah, yeah, that makes sense.
Comment 5 says this is 30+, but esr24 is marked as affected?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Whiteboard: [embargo until esr24 eol]
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.
Flags: needinfo?(kairo)
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
Attachment #8463601 - Flags: approval-mozilla-beta?
Attachment #8463601 - Flags: approval-mozilla-b2g32?
Attachment #8463601 - Flags: approval-mozilla-b2g30?
Attachment #8463601 - Flags: approval-mozilla-aurora?
(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 on attachment 8463601 [details] [diff] [review]
Part 2 - Invoke XPCWrappedNativeScope::TraceSelf from TraceXPCGlobal. v1

Bobby, what about ESR 31?
Attachment #8463601 - Flags: approval-mozilla-beta?
Attachment #8463601 - Flags: approval-mozilla-beta+
Attachment #8463601 - Flags: approval-mozilla-aurora?
Attachment #8463601 - Flags: approval-mozilla-aurora+
Flags: needinfo?(bobbyholley)
Comment on attachment 8463601 [details] [diff] [review]
Part 2 - Invoke XPCWrappedNativeScope::TraceSelf from TraceXPCGlobal. v1

Sorry, meant to flag that as well.
Attachment #8463601 - Flags: approval-mozilla-esr31?
Flags: needinfo?(bobbyholley)
Attachment #8463601 - Flags: approval-mozilla-b2g32?
Attachment #8463601 - Flags: approval-mozilla-b2g30?
Attachment #8463601 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Ryan, is there a reason you cancelled the b2g approval requests?
Flags: needinfo?(ryanvm)
32 is beta right now, so anything on beta gets merged to b2g32.  (I don't know about the b2g30 part of that.)
Comment 18 and sec-crits have auto-approval for b2g30. Bobby, can you jump on IRC and ping me quick?
Flags: needinfo?(ryanvm)
Marking this so the QA team will track it for verification in the current cycle.
Keywords: verifyme
There are no longer reports of these crash signatures on builds since this landed, at least on desktop.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Setting flags based on Tracy's comment above.
Whiteboard: [embargo until esr24 eol] → [embargo until esr24 eol][adv-main32-][adv-esr31.1-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.