Closed Bug 1338563 Opened 3 years ago Closed 3 years ago

Crash in nsXPCComponents_Utils::GetObjectPrincipal

Categories

(Core :: XPConnect, defect, P1, critical)

x86
Windows 8
defect

Tracking

()

RESOLVED DUPLICATE of bug 1322273
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 + fixed
firefox54 + fixed

People

(Reporter: jseward, Assigned: kmag)

References

Details

(Keywords: crash, regression)

Crash Data

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.
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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley) → needinfo?(kmaglione+bmo)
Blocks: 1273251
Keywords: regression
[Tracking Requested - why for this release]:
New crash introduced in 53.
I filed bug 1338647 for the whole "searchfox can't find all the callers of CheckedUnwrap" business.
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.
Flags: needinfo?(kmaglione+bmo)
(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.
OK, I'll bump up my priority for that, then.
Depends on: 1322273
(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.
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.
Tracking 53/54+ for this crash regression.
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.
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P1
(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.
Flags: needinfo?(kmaglione+bmo)
Any progress on these?  we're getting near beta for 53.
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
Needinfo'd for uplift there
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?
Flags: needinfo?(kmaglione+bmo)
Sorry, I have no idea why this was marked as affecting esr45. It definitely doesn't.
Flags: needinfo?(kmaglione+bmo)
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1322273
You need to log in before you can comment on or make changes to this bug.