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

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement)

Description

[Andrew Sutherland [:asuth] (he/him)]

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?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Andrea Marchesini [:baku]]

> 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.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: runnable.patch

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

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! :)

Comment 5

[Pulsebot]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34b63eb52b4d