1197421 - Incorrect cross thread promise handling in worker thread registration.unregister and WorkerFetchResolver

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Cătălin Badea (:catalinb)]

I found two sites where we don't handle promise passing between main thread and worker thread correctly while working on another patch.

https://treeherder.mozilla.org/logviewer.html#?job_id=10640647&repo=try

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

It would be even better if we could simplify this across the board, which is hard, but worth thinking about if someone has bandwidth.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

Bug 1197421 - Fix promise release in worker unregister and fetch. r?catalinb

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/17381/#review15465

Catalin, could you push to try again with whatever tests you were running? Thank you.

Comment 4

[Cătălin Badea (:catalinb)]

https://reviewboard.mozilla.org/r/17381/#review15465

The fix looks good to me, but unregister still crashes in one of the wpt tests.
https://treeherder.mozilla.org/logviewer.html#?job_id=10928794&repo=try

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8653150 [details]
MozReview Request: Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1197421 - Fetch resolver uses PromiseWorkerProxy. r?catalinb

Bug 1197421 - Fetch resolver uses PromiseWorkerProxy. r?catalinb

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20115f62b0bf

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8653150 [details]
MozReview Request: Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

Get rid of having users dispatch control runnables. It was error prone and
required too much reasoning. It was also possible to end up in a state where
callers would dispatch a WorkerRunnable, which would succeed, so they would not
dispatch a WorkerControlRunnable.  Then the worker would stop running,
canceling the runnable, leaving the proxy in an unclean state. Instead, we
AddRef() and add the feature and remove the feature and Release() on Notify().
If callers successfully run a WorkerRunnable they clean the proxy. If not, the
proxy stays alive until the worker switches to Canceling state.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8656259 [details]
MozReview Request: Bug 1197421 - Fetch resolver uses PromiseWorkerProxy. r?catalinb

Bug 1197421 - Fetch resolver uses PromiseWorkerProxy. r?catalinb

The logic was very similar and PromiseWorkerProxy is well tested.  We defy
convention a bit by calling CleanUp() in another runnable later in time after
resolving the Promise, but it does not violate any invariants.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8653150 [details]
MozReview Request: Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

Get rid of having users dispatch control runnables. It was error prone and
required too much reasoning. It was also possible to end up in a state where
callers would dispatch a WorkerRunnable, which would succeed, so they would not
dispatch a WorkerControlRunnable.  Then the worker would stop running,
canceling the runnable, leaving the proxy in an unclean state. Instead, we
AddRef() and add the feature and remove the feature and Release() on Notify().
If callers successfully run a WorkerRunnable they clean the proxy. If not, the
proxy stays alive until the worker switches to Canceling state.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8656259 [details]
MozReview Request: Bug 1197421 - Fetch resolver uses PromiseWorkerProxy. r?catalinb

Bug 1197421 - Fetch resolver uses PromiseWorkerProxy. r?catalinb

The logic was very similar and PromiseWorkerProxy is well tested.  We defy
convention a bit by calling CleanUp() in another runnable later in time after
resolving the Promise, but it does not violate any invariants.

Comment 12

[Cătălin Badea (:catalinb)]

Comment on attachment 8653150 [details]
MozReview Request: Bug 1197421 - Fix promise worker proxy cleanup and update callers. r?catalinb

https://reviewboard.mozilla.org/r/17383/#review16235

lgtm

::: dom/workers/ServiceWorkerClients.cpp:165
(Diff revision 4)
>    // operation is happening on the main thread.

Please remove the comment.

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Had to move ReleaseObject() into CleanUp() since the Mutex can't be locked when it is destroyed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eda57eac9b12

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Another run just to be sure
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c0691661342

Comment 15

[Cătălin Badea (:catalinb)]

Comment on attachment 8656259 [details]
MozReview Request: Bug 1197421 - Fetch resolver uses PromiseWorkerProxy. r?catalinb

https://reviewboard.mozilla.org/r/18127/#review16379

thanks for fixing this. \o/

::: dom/fetch/Fetch.cpp:51
(Diff revision 3)
> -class WorkerFetchResolver final : public FetchDriverObserver,
> +class WorkerFetchResolver final : public FetchDriverObserver

I think we can relax the threadsafe ref-counting of FetchDriverObserver.

The steps for worker fetch requests would be:
1. send a proxy to the main thread
2. create a WorkerFetchResolver
3. OnResponse(End) dispatch a runnable with the proxy back to the worker thread.

This way, WorkerFetchResolver will live only on the main thread.

::: dom/fetch/Fetch.cpp:61
(Diff revision 3)
> -    : mWorkerPrivate(aWorkerPrivate)
> +  Create(workers::WorkerPrivate* aWorkerPrivate, Promise* aPromise)

Based on the comment above, a normal constructor that asserts main thread would suffice.

::: dom/fetch/Fetch.cpp:301
(Diff revision 3)
> -  explicit WorkerFetchResponseEndRunnable(WorkerFetchResolver* aResolver)
> +  explicit WorkerFetchResponseEndRunnable(WorkerPrivate* aWorkerPrivate,

Nit: This doesn't need to be explicit anymore.

Pass only the promise proxy back to the worker thread.

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Cătălin Badea (:catalinb) from comment #15)
> Comment on attachment 8656259 [details]
> MozReview Request: Bug 1197421 - Fetch resolver uses PromiseWorkerProxy.
> r?catalinb
> 
> https://reviewboard.mozilla.org/r/18127/#review16379
> 
> thanks for fixing this. \o/
> 
> ::: dom/fetch/Fetch.cpp:51
> (Diff revision 3)
> > -class WorkerFetchResolver final : public FetchDriverObserver,
> > +class WorkerFetchResolver final : public FetchDriverObserver
> 
> I think we can relax the threadsafe ref-counting of FetchDriverObserver.
> 
> The steps for worker fetch requests would be:
> 1. send a proxy to the main thread
> 2. create a WorkerFetchResolver
> 3. OnResponse(End) dispatch a runnable with the proxy back to the worker
> thread.
> 
> This way, WorkerFetchResolver will live only on the main thread.
> 
> ::: dom/fetch/Fetch.cpp:61
> (Diff revision 3)
> > -    : mWorkerPrivate(aWorkerPrivate)
> > +  Create(workers::WorkerPrivate* aWorkerPrivate, Promise* aPromise)
> 
> Based on the comment above, a normal constructor that asserts main thread
> would suffice.
> 
> ::: dom/fetch/Fetch.cpp:301
> (Diff revision 3)
> > -  explicit WorkerFetchResponseEndRunnable(WorkerFetchResolver* aResolver)
> > +  explicit WorkerFetchResponseEndRunnable(WorkerPrivate* aWorkerPrivate,
> 
> Nit: This doesn't need to be explicit anymore.
> 
> Pass only the promise proxy back to the worker thread.

Filed a followup for making WorkerFetchResolver main thread only. Its not urgent.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c920d96f600
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c126018a48

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c920d96f600
https://hg.mozilla.org/mozilla-central/rev/34c126018a48