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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow WorkerHolder::HoldWorker() callers to specify the status value they consider to be a failure. r=baku

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Set explicit status levels to fail at when calling WorkerHolder::HoldWorker. r=baku

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f60ef91bdbde
https://hg.mozilla.org/mozilla-central/rev/37b2a40ca3f7
https://hg.mozilla.org/mozilla-central/rev/493fdcb41ed5