Closed
Bug 1293690
Opened 8 years ago
Closed 8 years ago
many WorkerHolder classes expect to get Terminated status, but HoldWorker() does not fail if worker is already Terminated
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(3 files)
2.89 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
13.66 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
843 bytes,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
While investigating an intermittent failure in bug 1290116 I discovered that we have a fairly prevalent race in a lot of code using WorkerHolder. Current WorkerHolder::HoldWorker() will only fail if the worker has a status of Canceling or greater: https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#5266 We have a lot of code now that wants to hold a worker alive until Terminating instead. Pretty much all our service worker code does this. This is a problem because these Terminating WorkerHolder instances will never clean up if they are added to a WorkerPrivate that is already Terminated. There are two possible solutions here: 1) Allow WorkerHolder classes to specify what status level they care about in HoldWorker(). 2) Just call Notify on WorkerHolder objects immediately if they are added to a Worker that is not in the Running state.
Assignee | ||
Comment 1•8 years ago
|
||
I think we have to do option (1) since there is no clean way to do (2) without dispatching a runnable back to the current thread. I don't think we want to do any runnable dispatch because then we could potentially race with another Notify() that happens for a greater status value.
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07e30db63ada
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8779472 [details] [diff] [review] P1 Allow WorkerHolder::HoldWorker() callers to specify the status value they consider to be a failure. r=baku This patch adds an additional argument to WorkerHolder::HoldWorker() that lets the caller indicate what worker Status level they consider "non-operational". For now I default it to the previous hard coded value of Canceling. A later patch will remove the default.
Attachment #8779472 -
Flags: review?(amarchesini)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8779473 [details] [diff] [review] P2 Set explicit status levels to fail at when calling WorkerHolder::HoldWorker. r=baku This patch updates each use of WorkerHolder::HoldWorker() to specify an exact failure Status level. Most of these were obvious due to when the WorkerHolder implementation shut things down in its Notify() method. For the cases where Notify() never shuts down or if it was otherwise ambiguous I simply specified Canceling to maintain current behavior.
Attachment #8779473 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8779474 [details] [diff] [review] P3 Force all callers of HoldWorker to provide an explicit status code that triggers failure. r=baku Remove the default Status value from WorkerHolder::HoldWorker(). Authors need to think about what value they want there when they write new code. We should not provide a default as it would lead to more instances of this bug.
Attachment #8779474 -
Flags: review?(amarchesini)
Updated•8 years ago
|
Attachment #8779472 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Attachment #8779473 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Attachment #8779474 -
Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f62ade9508 P1 Allow WorkerHolder::HoldWorker() callers to specify the status value they consider to be a failure. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/d84b0768f0ce P2 Set explicit status levels to fail at when calling WorkerHolder::HoldWorker. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/0342d98afa10 P3 Force all callers of HoldWorker to provide an explicit status code that triggers failure. r=baku
Comment 9•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/252ff19cd746 for https://treeherder.mozilla.org/logviewer.html#?job_id=34070669&repo=mozilla-inbound
Comment 10•8 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f60ef91bdbde P1 Allow WorkerHolder::HoldWorker() callers to specify the status value they consider to be a failure. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/37b2a40ca3f7 P2 Set explicit status levels to fail at when calling WorkerHolder::HoldWorker. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/493fdcb41ed5 P3 Force all callers of HoldWorker to provide an explicit status code that triggers failure. r=baku
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f60ef91bdbde https://hg.mozilla.org/mozilla-central/rev/37b2a40ca3f7 https://hg.mozilla.org/mozilla-central/rev/493fdcb41ed5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•