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?
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.
> 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.
Created attachment 8873320 [details] [diff] [review] runnable.patch
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! :)
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/34b63eb52b4d Renaming WorkerProxyToMainThreadRunnable::RunBackOnWorkerThread to RunBackOnWorkerThreadForCleanup and improving the description for this method, r=asuth