Closed Bug 1458971 Opened Last year Closed Last year

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

Categories

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

x86
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + fixed
firefox62 + verified

People

(Reporter: calixte, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

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
Flags: needinfo?(bkelly)
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
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
Priority: -- → P2
Adding Mac version of crash to the signature list.
Crash Signature: [@ mozilla::MozPromise<T>::ThenValueBase::AssertIsDead] → [@ mozilla::MozPromise<T>::ThenValueBase::AssertIsDead] [@ mozilla::MozPromise<T>::AssertIsDead]
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.
Attachment #8973022 - Flags: review?(amarchesini)
Attachment #8973022 - Flags: review?(amarchesini) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/e0b652659732
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
This is still triggering.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
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().
Attachment #8974404 - Flags: review?(amarchesini)
Attachment #8974404 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80eb58e9df78
P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku
https://hg.mozilla.org/mozilla-central/rev/80eb58e9df78
Status: ASSIGNED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
thanks, this seems to have fixed things on nightly. could you request uplift to 61.0b?
Flags: needinfo?(bkelly)
(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.
Flags: needinfo?(bkelly)
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
Attachment #8973022 - Flags: approval-mozilla-beta?
Comment on attachment 8974404 [details] [diff] [review]
P2 Make ClientSourceOpPromise hold the operation promise alive. r=baku

See comment 16.
Attachment #8974404 - Flags: approval-mozilla-beta?
Status: RESOLVED → VERIFIED
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.
Attachment #8973022 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8974404 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.