858642 - Null-check the XBL scope

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

XBL scope creation is lazy, and involves allocating an entire compartment, which can certainly OOM. Some of the crashes in bug 854604 comment 6 appear to be related to the XBL scope being null.

It's tempting to just use the global of XBL scope creation fails, but I don't think we should do that. If we decide we ought to be using an XBL scope, we should just fail if we can't make that happen.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> which can certainly OOM

But should it be able to do so non-fatally?

Comment 2

[Bobby Holley (:bholley)]

Attachment: Null-check the XBL scope. v1

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> But should it be able to do so non-fatally?

JS heap allocations are fallible, right? I mean, we could just MOZ_CRASH if that happens, but that doesn't solve the crashes here. Or maybe sandbox creation is failing for some other reason?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I mean, we could just MOZ_CRASH if that happens

That's what I meant, but good point about people hitting that....

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> > I mean, we could just MOZ_CRASH if that happens
> 
> That's what I meant, but good point about people hitting that....

Yeah. There's a chance that this is really some other issue in disguise, and that the fix here will just end up being wallpaper. The alternative is to make the sandbox scope allocation infallible and then see if we start seeing these crashes turn into allocation failure crashes, but that's a two-step process, which may involve backing out the first step. So more of a pain.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 733936 [details] [diff] [review]
Null-check the XBL scope. v1

r=me

Comment 7

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f51945a2b7

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b7f51945a2b7

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 733936 [details] [diff] [review]
Null-check the XBL scope. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 821850. That bug is now on beta, but it sounds like this may (or may not) fix the crashes in bug 858648. So I think we should land this on aurora now and see if it affects crash volume.
User impact if declined: Crashes, potentially.
Testing completed (on m-c, etc.): Baked on m-c.
Risk to taking this patch (and alternatives if risky): Not risky. 
String or IDL/UUID changes made by this patch: None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8cc6a4733c83