1435196 - Move the webidl binding interfaces out of WorkerPrivate

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

This is another set of patches for the worker code cleanup. Here I want to implement a ChromeWorker class and a Worker class for the webidl bindings. WorkerPrivate remains what it is but without being CCed and exposed to content.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Partially you have already reviewed this patch in bug 1413112.
I applied your comments.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947823 [details] [diff] [review]
patch

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

Overall looks good, but I'd like to understand better the plan for mSelfRef.  If we could get rid of it, that would be great.

::: dom/workers/WorkerPrivate.cpp
@@ +1645,5 @@
> +  // the main thread reference.  We do not unlink it.  This allows the CC to
> +  // break cycles involving the Worker and begin shutting it down (which does
> +  // happen in unlink) but ensures that the WorkerPrivate won't be deleted
> +  // before we're done shutting down the thread.
> +  if (!mBusyCount && !mMainThreadObjectsForgotten) {

Should we instead expose a WorkerPrivate::IsIdle() method and do the conditional traversal in the binding object that holds the strong ref to the WorkerPrivate?  That might be easier to reason about.  Things like ServiceWorker WorkerPrivate objects would then not have any of this stuff in it at all, etc.

::: dom/workers/WorkerPrivate.h
@@ +186,1 @@
>    RefPtr<WorkerPrivate> mSelfRef;

Why do we still need mSelfRef?  It seems like the binding object or ServiceWorkerPrivate should hold the WorkerPrivate alive now.

What is the plan for SharedWorker?  Should have some mParentEventTargetRef that represents a SharedWorker adapter object?  That adapter object could then handle actually all the various binding objects that attach to that SharedWorker.

Or should we just include mSharedWorkers list of SharedWorker references in the conditional cycle collection?

Comment 4

[Andrea Marchesini [:baku]]

> Should we instead expose a WorkerPrivate::IsIdle() method and do the
> conditional traversal in the binding object that holds the strong ref to the
> WorkerPrivate? 

The WorkerPrivate needs to have a reference to the webidl DOMEventTargetHelper (Worker or ChromeWorker) in order to dispatch the messages. It doesn't change too much who does the traverse.

> ::: dom/workers/WorkerPrivate.h
> @@ +186,1 @@
> >    RefPtr<WorkerPrivate> mSelfRef;
> 
> Why do we still need mSelfRef?  It seems like the binding object or
> ServiceWorkerPrivate should hold the WorkerPrivate alive now.

No. WorkerPrivate must keep itself alive, otherwise, when the worker is traversed and unlinked, there is nothing keeping the worker thread alive during the shutdown. WorkerPrivate releases itself just after the unregistering from the RuntimeService.

> What is the plan for SharedWorker?  Should have some mParentEventTargetRef
> that represents a SharedWorker adapter object?  That adapter object could
> then handle actually all the various binding objects that attach to that
> SharedWorker.

With this patch, mParentEventTargetRef is null for SharedWorkers and ServiceWorkers.
What you are proposing is an interesting cleanup and I definitely want to give a try. Are you OK with having it as a follow up?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Hmm, ok.  Can you document how idle shutdown happens when the DOM Worker object is GC'd in the new system?  Also, did we add a test verifying idle shutdown works?  I'm really worried about regressing that here.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: test

Here a test.

Comment 7

[Andrea Marchesini [:baku]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Hmm, ok.  Can you document how idle shutdown happens when the DOM Worker
> object is GC'd in the new system?  Also, did we add a test verifying idle
> shutdown works?  I'm really worried about regressing that here.

First of all, our shutting down algorithm doesn't match what the spec says and we should fix this.
But, related to this particular patch, the logic is not changed:

There are 4 ways a worker can be terminated:

1. GC/CC - When the worker is in idle state (busycount == 0), it allows to traverse the 'hidden' mParentEventTargetRef pointer. This is the exposed Worker webidl object. Doing this, CC will be able to detect a cycle and Unlink is called. In Unlink, Worker calls Terminate().

2. Worker::Terminate() is called - the shutdown procedure starts immediately.

3. WorkerScope::Close() is called - Similar to point 2.

4. xpcom-shutdown notification - We call Kill().

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #7)
> There are 4 ways a worker can be terminated:
> 
> 1. GC/CC - When the worker is in idle state (busycount == 0), it allows to
> traverse the 'hidden' mParentEventTargetRef pointer. This is the exposed
> Worker webidl object. Doing this, CC will be able to detect a cycle and
> Unlink is called. In Unlink, Worker calls Terminate().
> 
> 2. Worker::Terminate() is called - the shutdown procedure starts immediately.
> 
> 3. WorkerScope::Close() is called - Similar to point 2.
> 
> 4. xpcom-shutdown notification - We call Kill().

Thanks!  Can you put this in a comment somewhere?  Or does it exist in a comment somewhere that I missed?  Also maybe mention the part about the worker thread surviving for a while until all holders are gone which is why we need mSelfRef.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8948698 [details] [diff] [review]
test

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

Thanks!

::: dom/workers/test/test_shutdownCheck.xul
@@ +39,5 @@
> +  ok(checkWorker(), "We have the worker");
> +  w.onmessage = () => { resolve(); }
> +}).then(() => {
> +  info("Waiting...");
> +  let id = setInterval(() => {

Instead of setInterval() wrapping forceGC/forceCC, can you use SpecialPowers.exactGC(callback) instead?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #7)
> First of all, our shutting down algorithm doesn't match what the spec says
> and we should fix this.

Can you file a bug on this?  I'm not aware of the deviation from the spec.

Comment 11

[Andrea Marchesini [:baku]]

> Thanks!  Can you put this in a comment somewhere?  Or does it exist in a
> comment somewhere that I missed?  Also maybe mention the part about the
> worker thread surviving for a while until all holders are gone which is why
> we need mSelfRef.

There is something in WorkerHolder.h but I can improve the comment there.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: comments

This is mainly comment removed. There is not close handler any more.
The existing documentation says:

  // The worker is owned by its thread, which is represented here.  This is set
  // in Constructor() and emptied by WorkerFinishedRunnable, and conditionally
  // traversed by the cycle collector if the busy count is zero.

in mParentEventTargetRef.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8948710 [details] [diff] [review]
comments

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

These edits seem good, but I think what I'm missing in the documentation is that we expect the Worker DOM object to call Terminate() in its Unlink.  Also, why Unlink instead of in ~Worker()?

I really liked your "here are the 4 ways shutdown can occur" comment.  I wish we had that somewhere.  The WorkerHolder comment is more about state traversal which is harder to map back to the functional question about how shutdown starts.

Comment 14

[Andrea Marchesini [:baku]]

> I really liked your "here are the 4 ways shutdown can occur" comment.  I

Comment added in WorkerPrivate.h near mParentEventTargetRef.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c76a20ec127d
Move the webidl binding interfaces out of WorkerPrivate - part 1 - WebIDL bindings, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74002a0d31f
Move the webidl binding interfaces out of WorkerPrivate - part 2 - Tests, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b24b952a6e
Move the webidl binding interfaces out of WorkerPrivate - part 3 - Comments, r=bkelly

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c76a20ec127d
https://hg.mozilla.org/mozilla-central/rev/e74002a0d31f
https://hg.mozilla.org/mozilla-central/rev/19b24b952a6e