1245674 - 45.0 beta crash in mozilla::dom::Promise::AppendCallbacks at address 0x8

Status: RESOLVED FIXED

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

Description

[Robert Kaiser]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-32ae2ee8-016c-4531-a745-576842160203.
=============================================================

Top Stack Frames:
0 	xul.dll 	mozilla::dom::Promise::AppendCallbacks(mozilla::dom::PromiseCallback*, mozilla::dom::PromiseCallback*) 	dom/promise/Promise.cpp
1 	xul.dll 	mozilla::dom::Promise::Then(JSContext*, JS::Handle<JSObject*>, mozilla::dom::AnyCallback*, mozilla::dom::AnyCallback*, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) 	dom/promise/Promise.cpp
2 	xul.dll 	mozilla::dom::PromiseBinding::then 	obj-firefox/dom/bindings/PromiseBinding.cpp
3 	xul.dll 	js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
4 	xul.dll 	js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/CrossCompartmentWrapper.cpp
5 	xul.dll 	js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
6 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp
[...]

This happened very rarely with 44 and earlier, but started to rise when 45 did hit the beta channel, the crashes all have an address of 0x8 and most happen after some longer uptime of Firefox, 95% after more than 15min uptime, 73% after more than 1h, all of the crashes are on 32bit.

Most of the URLs are Facebook, but there are some YouTube, accounts.google.com and other sites in there as well.

This signature is 1.3% of all 45.0b1 data, 1.7% if I only look at non-e10s crashes.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This seems to be in places shutdown:

mozilla::places::DatabaseShutdown::BlockShutdown(nsIAsyncShutdownClient*)

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Important crash, tracking.

Comment 3

[Andrew Overholt [:overholt]]

(In reply to Ben Kelly [:bkelly] from comment #1)
> This seems to be in places shutdown:
> 
> mozilla::places::DatabaseShutdown::BlockShutdown(nsIAsyncShutdownClient*)

Nothing has really changed there recently. David, any thoughts?

Jan, any thoughts from an ?

Comment 4

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

Well, bug 1043863 wasn't that long ago. Something may have just made this happen more often.

But the actual crash is a null + offset, Promise::mGlobal.
This reminds me of bug 1182197. bz, you were looking the issue there.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

If I interpret this stack correctly, this takes place in one of the clients blocking Places shutdown. In Firefox vanilla, I have the impression that the only client that ever does this is sanitize.js, which has been tweaked recently, and is known to be very unreliable at the moment. However, it uses Promise.jsm rather than DOM Promise, so it's probably not the immediate caller.

After that, I lose the trail.

Generally, when sanitize.js causes a problem, it's because code is executed too late during shutdown. My intuition, based on smaug's comment 4, is that the crash is somehow related to both that and to the comment in Promise::Settle "We really should have a global here. Except we sometimes don't in the wild for some odd reason"

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, much like bug 1182197.

I think if !mGlobal we should treat it just like if it's dying.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Null-check mGlobal before dereferencing it in one more place in Promise code

Comment 8

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

Comment on attachment 8716328 [details] [diff] [review]
Null-check mGlobal before dereferencing it in one more place in Promise code

Yeah, I was thinking the same.
The promise callbacks won't be called, but I don't know what else could be done here.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Well, if this blocks shutdown, we might end up with AsyncShutdown timeouts, which will give us a clearer idea of what is blocking what and when.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1431df7bb3

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8716328 [details] [diff] [review]
Null-check mGlobal before dereferencing it in one more place in Promise code

Approval Request Comment
[Feature/regressing bug #]: Bug 1058695.
[User impact if declined]: Crashes in some cases.
[Describe test coverage new/current, TreeHerder]: This clearly wasn't being hit
   in our automation, and we're not sure how to trigger this condition.
[Risks and why]: Might convert a crash on shutdown to a hang on shutdown.  Not
   clear.  Other than that, probably no risk.
[String/UUID change made/needed]: None.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e1431df7bb3

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8716328 [details] [diff] [review]
Null-check mGlobal before dereferencing it in one more place in Promise code

Fix a crash, taking it.
Should be in 45 beta 4

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e66b2afa9628

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f542c0715e92