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

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

Adding Mac version of crash to the signature list.

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.

Pushed by bkelly@mozilla.com: 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

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

This is still triggering.

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 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().

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/80eb58e9df78 P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku

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

(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 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 on attachment 8974404 [details] [diff] [review] P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku See comment 16.

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.

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