1436784 - Considering a different way to expose WorkerPrivate

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

This is taken from the WorkerRef proof of concept. Note that this approach is not meant to replace the current way completely. Or at least, not soon.

/*
 * If you want to play with a DOM Worker, you must know that it can go away
 * at any time if nothing prevents its shutting down. This documentation helps
 * to understand how to play with DOM Workers correctly.
 *
 * There are several reasons why a DOM Worker could go away. Here the complete
 * list:
 *
 * a. GC/CC - If the DOM Worker thread is idle and the Worker object is garbage
 *    collected, it goes away.
 * b. The worker script can call self.close()
 * c. The Worker object calls worker.terminate()
 * d. Firefox is shutting down.
 *
 * When a DOM Worker goes away, it does several steps. See more in
 * WorkerHolder.h. The interesting point is that, the Worker thread will
 * basically stop scheduling WorkerRunnables. and eventually
 * WorkerControlRunnables. But if there is something preventing the shutting
 * down, it will always possible to dispatch WorkerControlRunnables.
 * Of course, at some point, the worker _must_ be released, otherwise firefox
 * will leak it and the browser shutdown will hang. And this is bad.
 *
 * First contacts with a DOM Worker using WorkerRef
 * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 *
 * The starting point of dealing with a DOM Worker is a WorkerWeakRef. You can
 * obtain a WorkerWeakRef calling mozilla::dom::GetWorkerWeakRefFromContext() or
 * mozilla::dom::GetCurrentThreadWorkerWeakRef().
 *
 * WorkerWeakRef is a refcounted, NON thread-safe object.
 *
 * From this object, you can obtain a WorkerPrivate, calling
 * WorkerWeakRef::GetPrivate(). It return nullptr if the worker is shutting
 * down or if it is already gone away.
 *
 * If you want to know when a DOM Worker starts the shutting down procedure, use
 * SetCallback(). Your function will be called. Note that _after_ the callback,
 * WorkerWeakRef::GetPrivate() will return nullptr.
 *
 * How to keep a DOM Worker alive?
 * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 *
 * If you need to keep the worker alive, you must use WorkerStrongRef.
 * You can have this refcounted, NON thread-safe object, calling
 * yourWorkerWeakRef->CreateStrongRef().
 *
 * Until you have a WorkerStrongRef:
 * a. the DOM Worker is kept alive.
 * b. you can have access to the WorkerPrivate, calling: Private().
 * c. WorkerControlRunnable can be dispatched.
 *
 * Note that the DOM Worker shutdown can start at any time, but having a
 * WorkerStrongRef prevents the full shutdown. Also with WorkerStrongRef, you
 * can set a callback calling SetCallback().
 *
 * How to have a thread-safe DOM Worker reference?
 * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 *
 * Sometimes you need to play with threads and you need a thread-safe worker
 * reference. WorkerThreadSafeRef is what you want. You can have this
 * refcounted, thread-safe object, calling:
 * workerStrongRef->CreateThreadSafeRef().
 *
 * Just because this object can be sent to different threads, we don't allow the
 * setting of a callback. It would be confusing. But you can set a callback in
 * the workerStrongRef, or in the workerWeakRef...
 *
 * WorkerThreadSafeRef can be destroyed in any thread. Internally it keeps a
 * reference to its WorkerStrongRef creator and this ref will be dropped on the
 * correct thread when the WorkerThreadSafeRef is deleted.
 */

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

This is just a proof of concept. I don't know how many tests it breaks.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - dom/workers

Here I convert all the dom/workers use of GetWorkerPrivateFromContext() and GetCurrentThreadWorkerPrivate() to WorkerRef.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - dom/console

I just need something to test how it works in reality. I ported a couple of APIs to WorkerRef. Here the Console API.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - dom/broadcastChannel

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - BroadcastChannel/MessagePort

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 3 - WebSocket

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 4 - Connection

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 5 - FileReader

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I think the refs should be name a tad more clear.

WeakWorkerRef, StrongWorkerRef and also the thread safe variants should have Weak or Strong in the name.
(Currently WorkerHolder is quite clear)

But I'm not still sure about the naming. Since those aren't just refs, but observers, given the SetCallback.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950520 [details] [diff] [review]
part 1 - WorkerRef

I don't like the setup where one needs to have first weak ref in order to create strong ref.
And CreateStrongRef is even a bit odd since it crashes if called after the weakref has been already notified.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - BroadcastChannel/MessagePort

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950617 [details] [diff] [review]
part 1 - WorkerRef


>+/*
>+ * If you want to play with a DOM Worker, you must know that it can go away
>+ * at any time if nothing prevents its shutting down. This documentation helps
>+ * to understand how to play with DOM Workers correctly.
>+ *
>+ * There are several reasons why a DOM Worker could go away. Here the complete
>+ * list:
>+ *
>+ * a. GC/CC - If the DOM Worker thread is idle and the Worker object is garbage
>+ *    collected, it goes away.
>+ * b. The worker script can call self.close()
>+ * c. The Worker object calls worker.terminate()
>+ * d. Firefox is shutting down.
>+ *
>+ * When a DOM Worker goes away, it does several steps. See more in
>+ * WorkerHolder.h. The interesting point is that, the Worker thread will
>+ * basically stop scheduling WorkerRunnables. and eventually
>+ * WorkerControlRunnables.
should be , before and

>+ But if there is something preventing the shutting
>+ * down, it will always possible to dispatch WorkerControlRunnables.
it will always be possible ? (and even during FF shutdown?)


>+ * Of course, at some point, the worker _must_ be released, otherwise firefox
>+ * will leak it and the browser shutdown will hang. And this is bad.
Drop 'and this is bad.' not really useful.


>+ * First contacts with a DOM Worker using WorkerRef
First contacts?
Perhaps just drop the sentence

>+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>+ *
>+ * The starting point of dealing with a DOM Worker is a WeakWorkerRef. You can
>+ * obtain a WeakWorkerRef calling mozilla::dom::GetWeakWorkerRefFromContext() or
>+ * mozilla::dom::GetCurrentThreadWeakWorkerRef().
this all isn't quite right. There is the static method in StrongWorkerRef

>+ *
>+ * If you need to keep the worker alive, you must use StrongWorkerRef.
>+ * You can have this refcounted, NON thread-safe object, calling
>+ * yourWeakWorkerRef->CreateStrongRef().
similar here


>+ * Until you have a StrongWorkerRef:
>+ * a. the DOM Worker is kept alive.
>+ * b. you can have access to the WorkerPrivate, calling: Private().
>+ * c. WorkerControlRunnable can be dispatched.
But not WorkerRunnables?
It is unclear to me here, how this all is used for example with BroadcastChannel to keep the worker actually alive and working normally,
so that also WorkerRunnables can be dispatched.


>+
>+class WorkerRef
...
>+  WorkerStatus mStatus;
It is unclear what mStatus is.
It is set just in the constructor to value Killing, but never changed.
Yet it is used in WorkerRef::SetCallback

>+  std::function<void()> mCallback;
>+};
>+


>+class WeakWorkerRef final : public WorkerRef
>+{
>+public:
>+  static already_AddRefed<WeakWorkerRef>
>+  Create(WorkerPrivate* aWorkerPrivate);
>+
>+  already_AddRefed<StrongWorkerRef>
>+  CreateStrongRef(const char* aName, WorkerStatus aMaxStatus);
I still don't understand why you need CreateStrongRef in WeakWorkerRef


>+class StrongWorkerRef final : public WorkerRef
>+{
>+public:
>+  static already_AddRefed<StrongWorkerRef>
>+  Create(WorkerPrivate* aWorkerPrivate,
>+         const char* aName,
>+         WorkerStatus aMaxStatus);
>+
>+  already_AddRefed<ThreadSafeWorkerRef>
>+  CreateThreadSafeRef();
Why StrongWorkerRef has CreateThreadSafeRef?
This setup looks weird. I see that you just end up reusing StrongWorkerRef in ThreadSafeWorkerRef.
What if ThreadSafeWorkerRef had Create() method?


>+class ThreadSafeWorkerRef final
hmm, ok, I guess this can be without Strong in its name, since weak makes less sense in Threadsafe case.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950616 [details] [diff] [review]
part 2 - BroadcastChannel/MessagePort

I think I should get part 1 reviewed before looking these others

Comment 17

[Andrea Marchesini [:baku]]

Attachment: ptr_1.patch

Comments applied. About broadcastChannel, probably there is some WorkerRunnable to adjust.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950906 [details] [diff] [review]
part 1 - WorkerRef

>+// These functions return a WeakWorkerRef if this thread is a worker and if it's
>+// not shutting down. If you really need a WeakWorkerRef, why don't you have
>+// already one?
drop 
"If you really need a WeakWorkerRef, why don't you have already one?"

>+// These functions return a StrongWorkerRef if this thread is a worker and if
>+// it's not shutting down. If you really need a StrongWorkerRef, why don't you
>+// have already one?
drop
"If you really need a StrongWorkerRef, why don't you have already one?"

>+WorkerRef::~WorkerRef()
>+{
>+  NS_ASSERT_OWNINGTHREAD(WorkerRef);
>+
>+  if (mHolder) {
>+    // This will be release the worker.
>+    mHolder = nullptr;
>+  }
No need to explicitly release UniquePtr in dtor.


>+ * There are several reasons why a DOM Worker could go away. Here the complete
>+ * list:
Here is the...

>+ * When a DOM Worker goes away, it does several steps. See more in
>+ * WorkerHolder.h. The interesting point is that, the Worker thread will
drop
"The interesting point is that, "



>+ * The starting point of dealing with a DOM Worker is a WeakWorkerRef. You can
>+ * obtain a WeakWorkerRef calling mozilla::dom::CreateWeakWorkerRef().
this isn't true anymore with the latest version of the API. I guess this can be just dropped.

>+ *
>+ * WeakWorkerRef is a refcounted, NON thread-safe object.
>+ *
>+ * From this object, you can obtain a WorkerPrivate, calling
>+ * WeakWorkerRef::GetPrivate(). It return nullptr if the worker is shutting
returns

>+ *
>+ * Until you have a StrongWorkerRef:
If you have a ...
>+ * Just because this object can be sent to different threads, we don't allow the
>+ * setting of a callback. It would be confusing. But you can set a callback in
>+ * the workerStrongRef, or in the workerWeakRef...
drop
"But you can set a callback in the workerStrongRef, or in the workerWeakRef..."
That is redundant.


WeakWorkerRef behaves oddly. If one creates one when status is running, and status then becomes closing, GetPrivate() returns null.
But one can then create a new WeakWorkerRef and its GetPrivate() returns non-null.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950925 [details] [diff] [review]
part 1 - WorkerRef

>+// These functions return a StrongWorkerRef if this thread is a worker and if
>+// the shutdown step is < aMaxStatus.
< is wrong here.
The check is 
if (aMaxStatus < mWorkerPrivate->Status()) { return false; }
So, the functions return StrongWorkerRef if
shutdow step is <= aMaxStatus

>+ * When a DOM Worker goes away, it does several steps. See more in
>+ * WorkerHolder.h. The interesting point is that, the Worker thread will
drop "The interesting point is that,". It doesn't make the comment any easier to read.
I commented about it earlier too ;)

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 2 - BroadcastChannel/MessagePort

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 3 - WebSocket

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 4 - Connection

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 5 - FileReader

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950982 [details] [diff] [review]
part 2 - BroadcastChannel/MessagePort

>   } else {
>     JSContext* cx = aGlobal.Context();
>-    workerPrivate = GetWorkerPrivateFromContext(cx);
>-    MOZ_ASSERT(workerPrivate);
>+    RefPtr<StrongWorkerRef> workerRef =
>+      CreateStrongWorkerRef(cx, "BroadcastChannel", Closing,
Running, not Closing, as far as I see. The old code already did shutdown on Closing.

>   if (NS_IsMainThread()) {
>     globalObject = do_QueryInterface(mBC->GetParentObject());
>   } else {
>-    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
>-    MOZ_ASSERT(workerPrivate);
>-    globalObject = workerPrivate->GlobalScope();
>+    RefPtr<WeakWorkerRef> workerRef = CreateWeakWorkerRef();
>+    if (!workerRef) {
>+      return IPC_OK();
>+    }
>+
>+    globalObject = workerRef->GetPrivate()->GlobalScope();
this isn't nice. Creating WeakWorkerRef just to access the global.

>   if (!NS_IsMainThread()) {
>-    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
>-    MOZ_ASSERT(workerPrivate);
>-    MOZ_ASSERT(!mWorkerHolder);
>+    RefPtr<MessagePort> self = this;
> 
>-    nsAutoPtr<WorkerHolder> workerHolder(new MessagePortWorkerHolder(this));
>-    if (NS_WARN_IF(!workerHolder->HoldWorker(workerPrivate, Closing))) {
>+    // When the callback is executed, we cannot process messages anymore because
>+    // we cannot dispatch new runnables. Let's force a Close().
>+    RefPtr<StrongWorkerRef> strongWorkerRef =
>+      CreateStrongWorkerRef("MessagePort", Closing,
Running
The old code force closes ports when status > Running.
HoldWorker above needs Closing, since AddHolder uses >= check

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950925 [details] [diff] [review]
part 1 - WorkerRef

>+WorkerRef::SetCallback(const std::function<void()>& aCallback,
>+                       WorkerStatus aMaxStatus)
>+{
>+  MOZ_ASSERT(mHolder);
>+  NS_ASSERT_OWNINGTHREAD(WorkerRef);
>+
>+  if (!mWorkerPrivate) {
>+    return false;
>+  }
>+
>+  mWorkerPrivate->AssertIsOnWorkerThread();
>+
>+  if (aMaxStatus < mWorkerPrivate->Status()) {
>+    return false;
>+  }
Here, this succeeds if aMaxStatus == worker status

>+StrongWorkerRef::Create(WorkerPrivate* aWorkerPrivate,
>+                        const char* aName,
>+                        WorkerStatus aMaxStatus,
>+                        const std::function<void()>& aCallback)
>+{
>+  MOZ_ASSERT(aWorkerPrivate);
>+  MOZ_ASSERT(aName);
>+
>+  RefPtr<StrongWorkerRef> ref = new StrongWorkerRef(aWorkerPrivate);
>+
>+  if (aCallback && NS_WARN_IF(!ref->SetCallback(aCallback, aMaxStatus))) {
>+    return nullptr;
>+  }
>+
>+  // The worker is kept alive by this holder.
>+  UniquePtr<Holder> holder(new Holder(aName, ref,
>+                                      WorkerHolder::PreventIdleShutdownStart));
>+  if (NS_WARN_IF(!holder->HoldWorker(aWorkerPrivate, aMaxStatus))) {
But here SetCallback and HoldWorker both get aMaxStatus, even though HoldWorker fails
when aMaxStatus == worker status.

back to r-. Not sure what is the best way to sort this out. I guess statuses should be used the same way everywhere.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950991 [details] [diff] [review]
part 3 - WebSocket

>-  // Disconnect can be called from some control event (such as Notify() of
>-  // WorkerHolder). This will be schedulated before any other sync/async
>+  // Disconnect can be called from some control event (such as a callback from
>+  // StrongWorkerRef). This will be schedulated before any other sync/async
scheduled

>+WebSocketImpl::RegisterWorkerRef()
> {
>-  mWorkerPrivate->AssertIsOnWorkerThread();
>-  MOZ_ASSERT(!mWorkerHolder);
>-  mWorkerHolder = new WebSocketWorkerHolder(this);
>-
>-  if (NS_WARN_IF(!mWorkerHolder->HoldWorker(mWorkerPrivate, Canceling))) {
>-    mWorkerHolder = nullptr;
>+  RefPtr<WebSocketImpl> self = this;
>+
>+  // In workers we have to keep the worker alive using a strong reference in
>+  // order to dispatch messages correctly.
>+  RefPtr<StrongWorkerRef> workerRef =
>+    CreateStrongWorkerRef("WebSocketImpl", Canceling, [self]()
HoldWorker fails if status is Canceling, CreateStrongWorkerRef succeeds.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950992 [details] [diff] [review]
part 4 - Connection

>   Create(WorkerPrivate* aWorkerPrivate, ConnectionWorker* aConnection)
>   {
>-    RefPtr<ConnectionProxy> proxy =
>-      new ConnectionProxy(aWorkerPrivate, aConnection);
>-    if (!proxy->HoldWorker(aWorkerPrivate, Closing)) {
>-      proxy->mConnection = nullptr;
>+    RefPtr<ConnectionProxy> proxy = new ConnectionProxy(aConnection);
>+
>+    RefPtr<StrongWorkerRef> workerRef =
>+      StrongWorkerRef::Create(aWorkerPrivate, "ConnectionProxy", Closing,
>+                              [proxy]() { proxy->Shutdown(); });
Again, the old and new code have somewhat different meaning for status.

I think I should review these other patches only once the part 1 is clarified.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #28)
> >+WebSocketImpl::RegisterWorkerRef()
> > {
> >-  mWorkerPrivate->AssertIsOnWorkerThread();
> >-  MOZ_ASSERT(!mWorkerHolder);
> >-  mWorkerHolder = new WebSocketWorkerHolder(this);
> >-
> >-  if (NS_WARN_IF(!mWorkerHolder->HoldWorker(mWorkerPrivate, Canceling))) {
> >-    mWorkerHolder = nullptr;
> >+  RefPtr<WebSocketImpl> self = this;
> >+
> >+  // In workers we have to keep the worker alive using a strong reference in
> >+  // order to dispatch messages correctly.
> >+  RefPtr<StrongWorkerRef> workerRef =
> >+    CreateStrongWorkerRef("WebSocketImpl", Canceling, [self]()
> HoldWorker fails if status is Canceling, CreateStrongWorkerRef succeeds.
Or I guess CreateStrongWorkerRef doesn't succeed either, but you pass aMaxStatus to it, yet,  that doesn't mean
max status, but first failing status.

Comment 31

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Some important changes here:

1. WeakWorkerRef and StrongWorkerRef are created from WorkerPrivate instead having Create*WorkerRef() functions.

2. They are created only if the worker is not already shutting down.

Comment 32

[Andrea Marchesini [:baku]]

Attachment: part 2 - BroadcastChannel/MessagePort

Note that here I also introduced a new functionality (with WPT included). If the worker is shutting down, BroadcastChannel and MessagePort can be still created but they are not able to proceed they tasks.

I also filed a bug about having BroadcastChannel.postMessage() not throwing an exception: https://github.com/whatwg/html/issues/3528

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini [:baku] from comment #31)
> Created attachment 8955768 [details] [diff] [review]
> part 1 - WorkerRef
> 
> Some important changes here:
> 
> 1. WeakWorkerRef and StrongWorkerRef are created from WorkerPrivate instead
> having Create*WorkerRef() functions.
Not quite sure I understand what this means, but ok, some change how the objects are created.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini [:baku] from comment #31)
> 
> 2. They are created only if the worker is not already shutting down.
Not quite true. Terminating is passed to HoldWorker, not Closing, so it is possible that 
Worker is already in closing state, which, I guess, should count as 'shutting down'.
But perhaps we want to allow creating holders still at that point.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8955768 [details] [diff] [review]
part 1 - WorkerRef

>+ *
>+ * When a DOM Worker goes away, it does several steps. See more in
>+ * WorkerHolder.h. The DOM Worker thread will basically stop scheduling
>+ * WorkerRunnables, and eventually WorkerControlRunnables. But if there is
>+ * something preventing the shutting down, it will always possible to dispatch
>+ * WorkerControlRunnables.  Of course, at some point, the worker _must_ be
>+ * released, otherwise firefox will leak it and the browser shutdown will hang.
>+ *
>+ * WeakWorkerRef is a refcounted, NON thread-safe object.
>+ *
>+ * From this object, you can obtain a WorkerPrivate, calling
>+ * WeakWorkerRef::GetPrivate(). It returns nullptr if the worker is shutting
>+ * down or if it is already gone away.
>+ *
>+ * If you want to know when a DOM Worker starts the shutting down procedure,
>+ * pass a callback to the mozilla::dom::WeakWorkerRef::Create() method.
>+ * Your function will be called. Note that _after_ the callback,
>+ * WeakWorkerRef::GetPrivate() will return nullptr.
>+ *
>+ * How to keep a DOM Worker alive?
>+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>+ *
>+ * If you need to keep the worker alive, you must use StrongWorkerRef.
>+ * You can have this refcounted, NON thread-safe object, calling
>+ * mozilla::dom::StrongWorkerRef::Create(WorkerPrivate* aWorkerPrivate);
>+ *
>+ * If you have a StrongWorkerRef:
>+ * a. the DOM Worker is kept alive.
>+ * b. you can have access to the WorkerPrivate, calling: Private().
>+ * c. WorkerControlRunnable can be dispatched.
>+ *
>+ * Note that the DOM Worker shutdown can start at any time, but having a
>+ * StrongWorkerRef prevents the full shutdown. Also with StrongWorkerRef, you
>+ * can pass a callback when calling mozilla::dom::StrongWorkerRef::Create().
>+ *
>+ * When the DOM Worker shutdown starts, WorkerRunnable cannot be dispatched
>+ * anymore. At this point, you should dispatch WorkerControlRunnable just to
>+ * release resources.
>+ *
>+ * How to have a thread-safe DOM Worker reference?
>+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>+ *
>+ * Sometimes you need to play with threads and you need a thread-safe worker
>+ * reference. ThreadSafeWorkerRef is what you want.
>+ *
>+ * Just because this object can be sent to different threads, we don't allow the
>+ * setting of a callback. It would be confusing.
>+ *
>+ * ThreadSafeWorkerRef can be destroyed in any thread. Internally it keeps a
>+ * reference to its StrongWorkerRef creator and this ref will be dropped on the
>+ * correct thread when the ThreadSafeWorkerRef is deleted.

somewhere in the documentation say that "shutting down" process in this context means anything > Closing state and refer to
WorkerHolder.h

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8955768 [details] [diff] [review]
part 1 - WorkerRef

>+/* static */ already_AddRefed<WeakWorkerRef>
>+WeakWorkerRef::Create(WorkerPrivate* aWorkerPrivate,
>+                      const std::function<void()>& aCallback)
>+{
>+  MOZ_ASSERT(aWorkerPrivate);
>+  aWorkerPrivate->AssertIsOnWorkerThread();
>+
>+  RefPtr<WeakWorkerRef> ref = new WeakWorkerRef(aWorkerPrivate);
>+
>+  // This holder doesn't keep the worker alive.
>+  UniquePtr<Holder> holder(new Holder("WeakWorkerRef::Holder", ref,
>+                                      WorkerHolder::AllowIdleShutdownStart));
>+  if (NS_WARN_IF(!holder->HoldWorker(aWorkerPrivate, Terminating))) {
Hmm, so this uses Terminating, so < Terminating is fine

+class WorkerRef::Holder final : public mozilla::dom::WorkerHolder
+{
+public:
+  Holder(const char* aName, WorkerRef* aWorkerRef, Behavior aBehavior)
+    : mozilla::dom::WorkerHolder(aName, aBehavior)
+    , mWorkerRef(aWorkerRef)
+  {}
+
+  bool
+  Notify(WorkerStatus aStatus) override
+  {
+    mWorkerRef->Notify();
+    return true;
+  }
but this just calls Notify anyhow, even when transitioning from Pending to Running, or from Running to Closing.

I think the status changes aren't after all clear to me yet.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8955769 [details] [diff] [review]
part 2 - BroadcastChannel/MessagePort

>+    RefPtr<StrongWorkerRef> workerRef =
>+      StrongWorkerRef::Create(workerPrivate, "BroadcastChannel",
>+                            [bc] () { bc->Shutdown(); });
fix indentation.


So, ::Create won't fail when we're closing

>+    // We are already shutting down the worker. Let's return a non-active
>+    // object.
>+    if (NS_WARN_IF(!workerRef)) {
>+      bc->mState = StateClosed;
>+      return bc.forget();
>+    }
>+
>+    RefPtr<ThreadSafeWorkerRef> tsr = new ThreadSafeWorkerRef(workerRef);
>+
>     RefPtr<InitializeRunnable> runnable =
>-      new InitializeRunnable(workerPrivate, origin, principalInfo, aRv);
>+      new InitializeRunnable(tsr, origin, principalInfo, aRv);
>     runnable->Dispatch(Closing, aRv);
>+    if (aRv.Failed()) {
>+      return nullptr;
>+    }
but this will.
That is ok, but did you verify the setup is ok everywhere, that creating *Refs while closing is accepted?

>-  } else {
>-    bc->mWorkerHolder = new BroadcastChannelWorkerHolder(bc);
>-    if (NS_WARN_IF(!bc->mWorkerHolder->HoldWorker(workerPrivate, Closing))) {
ok, and there Closing state isn't accepted

>-    nsAutoPtr<WorkerHolder> workerHolder(new MessagePortWorkerHolder(this));
>-    if (NS_WARN_IF(!workerHolder->HoldWorker(workerPrivate, Closing))) {
>-      aRv.Throw(NS_ERROR_FAILURE);
>+    // When the callback is executed, we cannot process messages anymore because
>+    // we cannot dispatch new runnables. Let's force a Close().
>+    RefPtr<StrongWorkerRef> strongWorkerRef =
>+      StrongWorkerRef::Create(workerPrivate, "MessagePort",
>+                            [self]() { self->CloseForced(); });
nit, indentation.

And if we're in closing state, this code succeeds.
Yet the old doesn't.

Comment 38

[Andrea Marchesini [:baku]]

> but this just calls Notify anyhow, even when transitioning from Pending to
> Running, or from Running to Closing.
> 
> I think the status changes aren't after all clear to me yet.

No. Notify() is called only when the status is > Running:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#3979

Comment 39

[Andrea Marchesini [:baku]]

Attachment: part 3 - WebSocket

That was the wrong patch. This patch contains a WPT test too.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8955769 [details] [diff] [review]
part 2 - BroadcastChannel/MessagePort

oops, I wasn't going to r+ this one. closing vs terminating handling isn't clear.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8955768 [details] [diff] [review]
part 1 - WorkerRef

I guess this is after all not the final patch.

Comment 42

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Comment 43

[Andrea Marchesini [:baku]]

Attachment: part 1 - WorkerRef

Comment 44

[Andrea Marchesini [:baku]]

Attachment: part 2 - BroadcastChannel/MessagePort

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8956727 [details] [diff] [review]
part 2 - BroadcastChannel/MessagePort

>+      StrongWorkerRef::Create(workerPrivate, "BroadcastChannel",
>+                            [bc] () { bc->Shutdown(); });
fix indentation

// When the callback is executed, we cannot process messages anymore because
>+    // we cannot dispatch new runnables. Let's force a Close().
>+    RefPtr<StrongWorkerRef> strongWorkerRef =
>+      StrongWorkerRef::Create(workerPrivate, "MessagePort",
>+                            [self]() { self->CloseForced(); });
ditto

Comment 46

[Andrea Marchesini [:baku]]

Attachment: part 5 - FileReader

Comment 47

[Andrea Marchesini [:baku]]

Attachment: part 4 - Connection

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8956747 [details] [diff] [review]
part 4 - Connection

>+  ConnectionProxy(ConnectionWorker* aConnection)
>+    : mConnection(aConnection)
>   {
>-    MOZ_ASSERT(mWorkerPrivate);
>-    MOZ_ASSERT(mConnection);
>-    mWorkerPrivate->AssertIsOnWorkerThread();
>+    MOZ_ASSERT(mWorkerRef);
This can't be right. mWorkerRef is explicitly set after ctor call.
So, the patch was never compiled/tested on a debug build?

Comment 49

[Andrea Marchesini [:baku]]

Attachment: part 4 - Connection

Right. that was the wrong patch. Here the right one. Green on try.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5118e942a2d7
Implement WeakWorkerRef, StrongWorkerRef and ThreadSafeWorkerRef, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd00b80ae9b7
Use WorkerRef in BroadcastChannel and MessagePort, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b59d4d53efd6
Use WorkerRef in WebSocket, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f210278ed5
Use WorkerRef in DOM Connection, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7344e313e2e9
Use WorkerRef in FileReader, r=smaug

Comment 51

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/5118e942a2d7
https://hg.mozilla.org/mozilla-central/rev/fd00b80ae9b7
https://hg.mozilla.org/mozilla-central/rev/b59d4d53efd6
https://hg.mozilla.org/mozilla-central/rev/c3f210278ed5
https://hg.mozilla.org/mozilla-central/rev/7344e313e2e9

Comment 52

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10038 for changes under testing/web-platform/tests

Comment 53

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 54

[Marijn]

The tests that were upstreamed here seem to rely on a non-standard firefox-only method (toSource) for no good reason... Could somebody please fix these tests to actually work on standards compliant browsers?

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Apologies. Need to switch to toString()

Comment 56

[Andrea Marchesini [:baku]]

Filed bug 1446285.