WorkerProxyToMainThreadRunnable uses a control runnable for the back-to-worker step

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Workers
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: asuth, Assigned: baku)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
WorkerProxyToMainThreadRunnable's ReleaseRunnable helper class subclasses MainThreadWorkerControlRunnable.  This is not documented in WorkerRunnable.h and is a little surprising.

There are 2 current uses of this class, BindingUtils.cpp's DeprecationWarningRunnable and Console.cpp's ConsoleRunnable and its subclasses.  Both of these only use RunBackOnWorkerThread for cleanup purposes.  The deprecation warning just for the side effect of the WorkerHolder being released and ConsoleRunnable also for explicitly releasing resources.

It seems like a win for these to release their WorkerHolder and releases sooner rather than later.  Should we just improve the comments on WorkerProxyToMainThreadRunnable?  Or is it better to avoid unexpected reordering for future consumers and the class should be converted to use the non-control runnable variant?
Flags: needinfo?(amarchesini)

Comment 1

6 months ago
If we try a non-control runnable, we need to fallback to a control runnable if the first Dispatch() fails.  Pretty sure these can be triggered while we're trying to shutdown which causes Dispatch() to fail.
(Assignee)

Comment 2

6 months ago
> If we try a non-control runnable, we need to fallback to a control runnable
> if the first Dispatch() fails.  Pretty sure these can be triggered while
> we're trying to shutdown which causes Dispatch() to fail.

I think we should improve the comment, specifying that RunBackOnWorkerThread() is meant to be used for cleanup purpose and nothing more. It's _kind of_ guaranteed to be executed because a control runnable is involved, and it should not be used for anything else but cleanup things.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 3

6 months ago
Created attachment 8873320 [details] [diff] [review]
runnable.patch
Assignee: nobody → amarchesini
Attachment #8873320 - Flags: review?(bugmail)
(Reporter)

Comment 4

6 months ago
Comment on attachment 8873320 [details] [diff] [review]
runnable.patch

Thanks!  The RunBackOnWorkerThreadForCleanup change is a great example of how to make it easier to understand what's going on in a complicated system with more than just comments.  (The comment is also great too! :)
Attachment #8873320 - Flags: review?(bugmail) → review+

Comment 5

6 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34b63eb52b4d
Renaming WorkerProxyToMainThreadRunnable::RunBackOnWorkerThread to RunBackOnWorkerThreadForCleanup and improving the description for this method, r=asuth

Comment 6

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34b63eb52b4d
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.