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

RESOLVED FIXED in Firefox 59

Status

()

Core
DOM: Service Workers
--
critical
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: calixte, Assigned: bkelly)

Tracking

(Blocks: 1 bug, {crash, regression})

59 Branch
mozilla59
Unspecified
Windows 10
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 fixed)

Details

(Whiteboard: [clouseau], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a month ago
This bug was filed from the Socorro interface and is
report bp-00cd36ca-df05-44a8-a14e-b6c020171215.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::MozPromise<unsigned int, mozilla::MediaTrackDemuxer::SkipFailureHolder, 1>::ThenValueBase::AssertIsDead xpcom/threads/MozPromise.h:448
1 xul.dll mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0>::AssertIsDead xpcom/threads/MozPromise.h:1017
2 xul.dll mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0>::~MozPromise<mozilla::dom::ClientOpResult, nsresult, 0> xpcom/threads/MozPromise.h:1062
3 xul.dll mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0>::`scalar deleting destructor' 
4 xul.dll mozilla::net::BoolWrapper::Release netwerk/protocol/http/nsHttpConnectionMgr.cpp:218
5 xul.dll mozilla::dom::StartClientManagerOp<RefPtr<mozilla::MozPromise<mozilla::dom::ClientOpResult, enum nsresult, 0> > , mozilla::dom::ClientMatchAllArgs, <lambda_c27725e7889e7721ac30c52d4c28166b>, <lambda_f80d0b7fbb2bc1ba18e2a1f0bcf7ae61> > dom/clients/api/ClientDOMUtil.h:49
6 xul.dll mozilla::dom::Clients::MatchAll dom/clients/api/Clients.cpp:168
7 xul.dll mozilla::dom::ClientsBinding::matchAll dom/bindings/ClientsBinding.cpp:297
8 xul.dll mozilla::dom::ClientsBinding::matchAll_promiseWrapper dom/bindings/ClientsBinding.cpp:316
9 xul.dll mozilla::dom::GenericPromiseReturningBindingMethod dom/bindings/BindingUtils.cpp:3088

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

There are 29 crashes (from 4 installs) in nightly 59 starting with buildid 20171213220121. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1293277.

[1] https://hg.mozilla.org/mozilla-central/rev/f7b0cd514d46
Flags: needinfo?(bkelly)
(Assignee)

Comment 1

a month ago
I believe this is occurring when Clients.matchAll() is called on a service worker thread that has started shutting down.  We simply don't execute the ClientManager::MatchAll() IPC operation thanks to this check:

https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientThing.h#54

We should instead reject the MozPromise if the operation has one. Right now the MozPromise is just a lambda capture that ClientThing doesn't know about.  I'll have to think about the best way to handle this.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
(Assignee)

Comment 2

a month ago
Created attachment 8937469 [details] [diff] [review]
P1 Make ClientThing::MaybeExecute() call an optional failure callback. r=asuth
(Assignee)

Comment 3

a month ago
Created attachment 8937470 [details] [diff] [review]
P2 Make ClientManager and ClientHandle operation promises reject if called on a worker thread already shutting down. r=asuth
(Assignee)

Comment 5

a month ago
Created attachment 8937471 [details] [diff] [review]
P1 Make ClientThing::MaybeExecute() call an optional failure callback. r=asuth
Attachment #8937469 - Attachment is obsolete: true
(Assignee)

Comment 6

a month ago
Comment on attachment 8937471 [details] [diff] [review]
P1 Make ClientThing::MaybeExecute() call an optional failure callback. r=asuth

Review of attachment 8937471 [details] [diff] [review]:
-----------------------------------------------------------------

Andrew, this patch adds an optional failure callback to ClientThing::MaybeExecute().  This method is called whenever ClientSource, ClientHandle, or ClientManager want to send something across an IPC actor.  The ClientHandle and ClientManager cases are usually holding a MozPromise over these IPC operations.  If we never start the IPC call we need to tell ClientHandle and ClientManager so they can reject the promise.
Attachment #8937471 - Flags: review?(bugmail)
(Assignee)

Comment 7

a month ago
Comment on attachment 8937470 [details] [diff] [review]
P2 Make ClientManager and ClientHandle operation promises reject if called on a worker thread already shutting down. r=asuth

Review of attachment 8937470 [details] [diff] [review]:
-----------------------------------------------------------------

This patch makes us actually use the optional failure callback in MaybeExecute() to reject our ClientHandle/ClientManager promises.
Attachment #8937470 - Flags: review?(bugmail)
Attachment #8937471 - Flags: review?(bugmail) → review+
Attachment #8937470 - Flags: review?(bugmail) → review+

Comment 8

a month ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/926123cda347
P1 Make ClientThing::MaybeExecute() call an optional failure callback. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/10b24604cf6b
P2 Make ClientManager and ClientHandle operation promises reject if called on a worker thread already shutting down. r=asuth

Comment 9

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/926123cda347
https://hg.mozilla.org/mozilla-central/rev/10b24604cf6b
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.