Improve WorkerHolder use in Runnables

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

49 Branch
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8765362 [details] [diff] [review]
holder.patch

There are a couple of runnables (in Console and in BindingUtils) that use WorkerHolder just to keep the worker alive.
With this patch I want to unify these runnables implementing a common base class and use ModifyBusyCount instead using WorkerHolder.

This is a good first step to make WorkerHolder a thread-only class so that we can add assertions in the CTOR and in the DTOR.
Attachment #8765362 - Flags: review?(khuey)
Comment on attachment 8765362 [details] [diff] [review]
holder.patch

Review of attachment 8765362 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/ActorsChild.cpp
@@ +927,5 @@
> +    if (NS_WARN_IF(!mWorkerPrivate->ModifyBusyCountFromWorker(true))) {
> +      return false;
> +    }
> +
> +    if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {

here too

::: dom/workers/WorkerRunnable.cpp
@@ +721,5 @@
> +    RunBackOnWorkerThread();
> +    return false;
> +  }
> +
> +  if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {

NS_DispatchToMainThread can't fail until after xpcom-shutdown-threads, at which point the worker has to be gone.  So you can MOZ_ALWAYS_SUCCEEDS this.

::: dom/workers/WorkerRunnable.h
@@ +410,5 @@
>  
> +// This runnable is an helper class for dispatching something from a worker
> +// thread to the main-thread and back to the worker-thread. During this
> +// operation, this class will keep the worker alive.
> +class WorkerToMainThreadRunnable : public Runnable

I have to admit that I don't care for this name, but I can't think of anything much better. WorkerProxyToMainThreadRunnable? WorkerPingPongToMainThreadRunnable? :P

I think the name needs to indicate that it comes back ...
Attachment #8765362 - Flags: review?(khuey) → review+

Comment 3

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc14db264521
Improve WorkerHolder use in Runnables, r=khuey

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc14db264521
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1286487
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1280168
You need to log in before you can comment on or make changes to this bug.