1458971 - Crash in mozilla::MozPromise<T>::ThenValueBase::AssertIsDead

Status: VERIFIED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-cbc33ab6-070d-4bab-a77a-efe530180503.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::MozPromise<bool, bool, 0>::ThenValueBase::AssertIsDead xpcom/threads/MozPromise.h:447
1 xul.dll mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0>::AssertIsDead xpcom/threads/MozPromise.h:1016
2 xul.dll mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0>::~MozPromise<mozilla::dom::ClientOpResult, nsresult, 0> xpcom/threads/MozPromise.h:1061
3 xul.dll mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0>::Private::`scalar deleting destructor' 
4 xul.dll mozilla::MozPromiseRefcountable::Release xpcom/threads/MozPromise.h:146
5 xul.dll void mozilla::Maybe<<lambda_2e74b72bd1fcce7f9c41a99219bb9bc8> >::reset mfbt/Maybe.h:536
6 xul.dll mozilla::dom::DOMMozPromiseRequestHolder<mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0> >::DisconnectFromOwner dom/base/DOMMozPromiseRequestHolder.h:80
7 xul.dll void std::_Func_impl_no_alloc<<lambda_3ceb91d4765c7763deeecc61e0110841>, void, mozilla::DOMEventTargetHelper*, bool*>::_Do_call vs2017_15.6.6/VC/include/functional:16707566
8 xul.dll nsIGlobalObject::ForEachEventTargetObject dom/base/nsIGlobalObject.cpp:165
9 kernelbase.dll PostQueuedCompletionStatus 

=============================================================

There are 6 crashes (from 5 installations) in nightly 61 starting with buildid 20180502220059. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1457157.

[1] https://hg.mozilla.org/mozilla-central/rev?node=3f9dd911845c

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I think this is due to these kung fu grip Thenables I am using.  I'll replace them with something less clever.

https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/dom/clients/manager/ClientHandle.cpp#50

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make operation actors explicitly hold references to the initiating object instead of using an extra MozPromise::Then(). r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e717acb2de47a131e2ba08a1ece1cef68897559f

Comment 3

[Marcia Knous [:marcia]]

Adding Mac version of crash to the signature list.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8973022 [details] [diff] [review]
Make operation actors explicitly hold references to the initiating object instead of using an extra MozPromise::Then(). r=baku

Andrea, I believe this crash is due to the use Then() for the kung-fu grip in ClientManager.  That thenable does not have a global to disconnect it.

The goal of this thenable was to keep the ClientManager alive until the operation completes.  This patch fixes the problem by removing the thenable and instead just having the operation actor hold the reference to the ClientManager directly.

The ClientHandle/op code has the same issue.  I used to have a kung-fu grip thenable there, but removed it recently.  I see I forgot to keep the ref alive for the entire operation, though.  So this patch also fixes ClientHandleOpChild to also reference the ClientHandle for similar reasons.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b652659732
Make operation actors explicitly hold references to the initiating object instead of using an extra MozPromise::Then(). r=baku

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Let's see if this works on trunk before uplifting to beta.

Comment 7

[Andrei Ciure[:aciure]]

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

This is still triggering.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I need to do a P2 patch here to deal with the MozPromise values created in ClientSource and Then'd in ClientSourceOpChild.  I thought these were handled because I have a MozPromiseHolder in ClientSourceOpChild that disconnects when the actor dies.

Looking at the crash, though, it seems that disconnecting the outer promise in ClientSource makes us destroy the promise before the actor can shutdown and disconnect on its side.

A simple solution would be to simply make the actor hold the promise alive.  I'm not sure I can cleanly remove the promise since I'm using the disconnect feature to deal with the actor going away before the ClientSource.  I don't want to build a similar mechanism.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5dde3c76bd6403141bbe0de6627977d9b264c60

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8974404 [details] [diff] [review]
P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku

Andrea, now that we are using DOMMozPromiseRequestHolder to disconnect the Then() in ClientSource::Claim() its possible for the promise returned to the ClientSourceOpChild to be destroyed before the actor shutdown starts.  Since we disconnect the Then() in ClientSourceOpChild::ActorDestroy() this means that we hit the AssertIsDead() crash.

This patch simply holds the promise returned from the ClientSource alive until we either get a reaction or actor shutdown disconnects the op Then().

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80eb58e9df78
P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/80eb58e9df78

Comment 14

[[:philipp]]

thanks, this seems to have fixed things on nightly. could you request uplift to 61.0b?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to [:philipp] from comment #14)
> thanks, this seems to have fixed things on nightly. could you request uplift
> to 61.0b?

Sure, although the diagnostic assertions only really impact nightly and dev-edition.  They won't trigger crashes in beta/release.  And in this case I don't think anything bad will happen in beta/release without the assertion triggering.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8973022 [details] [diff] [review]
Make operation actors explicitly hold references to the initiating object instead of using an extra MozPromise::Then(). r=baku

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1457157
[User impact if declined]: Diagnostic assertions causing crashes in nightly and dev-edition.
[Is this code covered by automated tests?]: Yes, but the particular crash only happens when a race condition occurs, so we don't see it show up in automation.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Small risk
[Why is the change risky/not risky?]: The changes are moderate in size, but its been verified on nightly.
[String changes made/needed]: None

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8974404 [details] [diff] [review]
P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku

See comment 16.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8973022 [details] [diff] [review]
Make operation actors explicitly hold references to the initiating object instead of using an extra MozPromise::Then(). r=baku

Fixes a topcrash. Approved for 61.0b4.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/19a17750a099
https://hg.mozilla.org/releases/mozilla-beta/rev/1304c1c433d3