Closed Bug 1435196 Opened 2 years ago Closed 2 years ago

Move the webidl binding interfaces out of WorkerPrivate

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Partially you have already reviewed this patch in bug 1413112.
I applied your comments.
Attachment #8947747 - Flags: review?(bkelly)
Attached patch patchSplinter Review
Attachment #8947747 - Attachment is obsolete: true
Attachment #8947747 - Flags: review?(bkelly)
Attachment #8947823 - Flags: review?(bkelly)
Blocks: 1435263
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?
Attachment #8947823 - Flags: review?(bkelly) → review-
> 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?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Attached patch testSplinter Review
Here a test.
Attachment #8948698 - Flags: review?(bkelly)
(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().
(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 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?
Attachment #8948698 - Flags: review?(bkelly) → review+
(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.
> 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.
Priority: -- → P2
Attached patch commentsSplinter Review
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.
Attachment #8948710 - Flags: review?(bkelly)
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.
Attachment #8948710 - Flags: review?(bkelly) → review+
> I really liked your "here are the 4 ways shutdown can occur" comment.  I

Comment added in WorkerPrivate.h near mParentEventTargetRef.
Pushed by amarchesini@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.