many WorkerHolder classes expect to get Terminated status, but HoldWorker() does not fail if worker is already Terminated

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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

2 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

2 years ago
Created attachment 8779472 [details] [diff] [review]
P1 Allow WorkerHolder::HoldWorker() callers to specify the status value they consider to be a failure. r=baku
(Assignee)

Comment 3

2 years ago
Created attachment 8779473 [details] [diff] [review]
P2 Set explicit status levels to fail at when calling WorkerHolder::HoldWorker. r=baku
(Assignee)

Comment 4

2 years ago
Created attachment 8779474 [details] [diff] [review]
P3 Force all callers of HoldWorker to provide an explicit status code that triggers failure. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07e30db63ada
(Assignee)

Comment 5

2 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

2 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

2 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)
Attachment #8779472 - Flags: review?(amarchesini) → review+
Attachment #8779473 - Flags: review?(amarchesini) → review+
Attachment #8779474 - Flags: review?(amarchesini) → review+

Comment 8

2 years ago
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 10

2 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

2 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
Last Resolved: 2 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.