1338563 - Crash in nsXPCComponents_Utils::GetObjectPrincipal

Status: RESOLVED DUPLICATE of bug 1322273

(Core :: XPConnect, defect, P1)

Description

[Julian Seward [:jseward]]

This bug was filed from the Socorro interface and is 
report bp-b1d25448-a7d1-47f7-9b15-b6b0b2170210.
=============================================================

This is ranked #12 in the Windows build for Aurora 20170209004018.

Comment 1

[Boris Zbarsky [:bzbarsky]]

It'd be so nice to have a JS stack here....

Anyway, it's a null deref.  Presumably CheckedUnwrap returned null.

Bobby, do you know whether that can happen for something like a DeadObjectProxy?  At first glance that's not a Wrapper, so I'd guess no... which means we somehow managed to get this called with an actual wrapper we can't unwrap somehow?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1)
> It'd be so nice to have a JS stack here....
> 
> Anyway, it's a null deref.  Presumably CheckedUnwrap returned null.
> 
> Bobby, do you know whether that can happen for something like a
> DeadObjectProxy?  At first glance that's not a Wrapper, so I'd guess no...
> which means we somehow managed to get this called with an actual wrapper we
> can't unwrap somehow?

The only time CheckedUnwrap returns null is when it encounters a wrapper handler with a security policy. That used to never happen in chrome scopes, which is why there's no null check here.

But then bug 1273251 changed that. I think that was a mistake and I shouldn't have allowed it. There are too many places that assume that CheckedUnwrap never returns null for system code.

Comment 3

[Bobby Holley (:bholley)]

[Tracking Requested - why for this release]:
New crash introduced in 53.

Comment 4

[Bobby Holley (:bholley)]

I filed bug 1338647 for the whole "searchfox can't find all the callers of CheckedUnwrap" business.

Comment 5

[Kris Maglione [:kmag]]

Would fixing bug 1322273 fix this problem? It seems like it should.

Also, it would be nice to know what's causing this. It should only be possible when we're creating new wrappers for a compartment after it's been nuked, which probably means a bug.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #5)
> Would fixing bug 1322273 fix this problem? It seems like it should.

Yes, it would.

> Also, it would be nice to know what's causing this. It should only be
> possible when we're creating new wrappers for a compartment after it's been
> nuked, which probably means a bug.

Comment 7

[Kris Maglione [:kmag]]

OK, I'll bump up my priority for that, then.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #5)
> Also, it would be nice to know what's causing this. It should only be
> possible when we're creating new wrappers for a compartment after it's been
> nuked, which probably means a bug.

The only useful information from the stack is that this is coming from a JS event dispatched to the main event loop, with only JS between it and the getObjectPrincipal call. My best guess is that this is coming from the devtools promise debugging code, which dispatches promises-settled events via Services.mainThread.dispatch. Those events, I suspect, cause a .grip() call, which checks the safety of the object by calling getObjectPrincipal.

That code already checks for dead wrappers, so just fixing bug 1322273 should probably be sufficient.

Comment 9

[Kris Maglione [:kmag]]

Hm. No, that actually shouldn't be possible. The promise in that case would already be a DeadObjectProxy, so it wouldn't be possible to create a new wrapper from it. The object would actually have to come from an XPConnect getter. Which means that this is probably coming from the devtools docshell tracking code, for a WebExtension window that's being destroyed (made more likely by the fact that the crash reports have temporary extensions installed).

So there probably is a bug to fix there.

Comment 10

[Marcia Knous [:marcia]]

Tracking 53/54+ for this crash regression.

Comment 11

[Andrew Overholt [:overholt]]

Do you want to dupe this to bug 1322273, Kris? Sorry if I'm overstepping assigning this to you but I'm assuming you're going to fix this given earlier comments.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Andrew Overholt [:overholt] from comment #11)
> Do you want to dupe this to bug 1322273, Kris? Sorry if I'm overstepping
> assigning this to you but I'm assuming you're going to fix this given
> earlier comments.

Maybe. I'm still worried that we have code that's triggering this, though, per comment 9. I want to look into that even though bug 1322273 should fix the immediate issue.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Any progress on these?  we're getting near beta for 53.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Randell Jesup [:jesup] from comment #13)
> Any progress on these?  we're getting near beta for 53.

Yes, it's being worked on in bug 1322273.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Needinfo'd for uplift there

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Regression has been resolved by fixing bug 1322273.  marking.  See comment 9 for possible remaining issues.  Kris, you decide what to do with this bug (close, dup, rename/revector, etc).

Also, this is marked as affecting ESR45 - true?  is it something we'd land there, or is it wontfix?

Comment 17

[Kris Maglione [:kmag]]

Sorry, I have no idea why this was marked as affecting esr45. It definitely doesn't.

Comment 18

[Kris Maglione [:kmag]]

I'm going to mark this as a dupe after all. I'd still like to find out why the devtools was triggering this issue in the first place, but I won't have time any time soon, so I don't think there's much point in keeping this open.