1178271 - crash in mozilla::dom::Promise::Settle(JS::Handle<T>, mozilla::dom::Promise::PromiseState)

Status: RESOLVED DUPLICATE of bug 1182197

(Core :: DOM: Core & HTML, defect)

Description

[Jesper Hansen]

This bug was filed from the Socorro interface and is 
report bp-5de3859e-a203-4092-af6d-4fc282150629.
=============================================================

This is one of the odd crashes that appear to happen when using Firefox after restoring Windows from Standby/Hibernation.
Direct cause is not known.

More crashes: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Adom%3A%3APromise%3A%3ASettle%28JS%3A%3AHandle%3CT%3E%2C+mozilla%3A%3Adom%3A%3APromise%3A%3APromiseState%29#tab-reports

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hrm.  These all seem to be null-deref crashes on mGlobal->IsDying().  Further, the crash address (0x4) is consistent with reading the mIsDying of a null global but not with reading the mGlobal of a null Promise, afaict).

But I don't see how we could have a null mGlobal here unless we have been unlinked... in which case how is script managing to call into us?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Is it guaranteed that mGlobal is always set to non-null value? MOZ_ASSERT is there only for debug builds.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I guess it is, since Promise::Create should fail if global is null.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I guess it is, since Promise::Create should fail if global is null.

Exactly...

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I'd assume we store somewhere a promise object, and end up calling unlink on it, but
not unlink the owner->promise edge and then somehow pass promise object back to JS.
Trying to find where that might happen...

Comment 6

[Boris Zbarsky [:bzbarsky]]

If you look at the stacks, they all (well, the 5-6 I looked at) come in via Promise::JSCallback.  That's only called as a JSNative by a JSFunction created by Promise::CreateFunction, which is only called by Promise::CallInitFunction.  CallInitFunction is only called from Promise::Constructor and AbortablePromise::Constructor.

In other words the only possible flow I see here is something like this, from JS:

  var res, rej:
  var p = new Promise(function(resolve, reject) {
    res = resolve;
    rej = reject;
  });

  // sometime later
  res(value); // but promise is cced here?

I do seem to see a bunch of chrome JS higher up the stack (in some cases calling the resolve/reject handler of the Promise directly), but that may just be because our chrome uses Promises a lot and not indicative of a CC bug involving cross-compartment wrappers.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Here's a question.  Do we need to mark black when we store the promise object in the reserved slot of the function?  Can we even be non-black there?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Why do we store anything in the reserved slot here?
And if something is gray, CC traverses through such JS and if cycle is then detected without
incoming references (which would keep the cycle alive), C++ objects in the cycle are unlinked.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Why do we store anything in the reserved slot here?

Because the caller just has the function and calls it.  But the function needs to then be able to resolve the promise.  So it needs to be able to get at the promise.  So we store the promise in a reserved slot as an ObjectValue.

I understand how the gray thing works, I think.  It seems like a bug to have JS run that has access to objects that were part of such a gray cycle, though....

Comment 10

[Boris Zbarsky [:bzbarsky]]

Kyle, do you think you have the cycles to take a look at once of the minidumps in Visual Studio and see whether the analysis above about what's null is correct?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Next week, perhaps.

Comment 12

[(Away)]

I can confirm that mGlobal is null based on the disassembly and registers. But I can't speak to the analysis regarding Unlink, etc.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK.  Well, mGlobal is set in the Promise constructor, and known non-null at that point because immediately after that we call CreateWrapper which will fail out if mGlobal is null, so the promise will never get handed out to anyone.  And the only thing that sets mGlobal to null on a Promise is Unlink.

So it really does sound like some sort of CC problem here.  :(

Comment 14

[Andrew McCreight [:mccr8]]

We've had a few "use after unlink" bugs, but unfortunately without any more information or steps to reproduce it isn't really possible to figure out what the cause is. Maybe I can take a look at Promise CC and hope that something jumps out at me.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracking for FF41 as this was mentioned in the channel meeting today. We are hitting this crash at 1.4% as of 41.0b3.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Boris, Andrew: There is a recent spike in this crash. On Beta41 (b3 + b4 + b5), I see ~1800 crashes. Could you please help find an owner who can investigate the crash dumps to determine a speculative fix?

Note: 41 already has fixes for bug 1189122 and 1074416.

Comment 18

[Boris Zbarsky [:bzbarsky]]

By speculative fix do we mean fixes to CC, or a null-check in Settle?  The latter may or may not fix the crash, but will definitely lead to bogus behavior...

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Boris Zbarsky [:bz] from comment #18)
> By speculative fix do we mean fixes to CC, or a null-check in Settle?  The
> latter may or may not fix the crash, but will definitely lead to bogus
> behavior...

yes a null check/easy fix is what i meant but not at the cost of bogus behavior :)

Comment 20

[Robert Kaiser]

Right now, it feels to me like we will ship this undefined Promise crash to 41 release, which doesn't sound good. We both need to find a way to fix this and also get information about later crashes with Promises. How can we get to instrumentation that can help us to get those in check?

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK, this is a bit ridiculous.  Could we please pick one of this bug and bug 1182197 and keep all the discussion there?  At this point most of the substantive stuff seems to be over there...

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Untracked because of comment 22. bug 1182197 is FF41+.