1413112 - Some cleanup in WorkerPrivate.{h,cpp}

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

WorkerPrivate code is getting hard to maintain. This bug is about:

1. Implement Worker and ChromeWorker classes instead having them as part of WorkerPrivateParent.
2. Remove WorkerPrivate template and make it a simpler class.
3. Splitting WorkerPrivate in several sub files.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - Renaming Workers.h to WorkerCommon.h

This is needed to avoid a name conflict between Worker class, in a Worker.h file, and the current Workers.h file.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - MessageEventRunnable

MessageEventRunnable will be used by Worker class. Let's move it into a separate file.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - Worker and ChromeWorker classes

This, together with part 4, is the most important patch of this set. Here I create 2 new classes: Worker and ChromeWorker to match what the spec says.

WorkerPrivate is no longer the 'DOM' interface, exposed via WorkerPrivateParent.
Next patch will get rid of WorkerPrivateParent.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - Get rid of WorkerPrivateParent template

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - WorkerLoadInfo in separate files

Separate files for WorkerLoadInfo.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 6 - WorkerDebugger in separate files

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 7 - WorkerError in separate files

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 8 - Fixing includes

Comment 9

[Andrea Marchesini [:baku]]

Attachment: mozilla::Monitor

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923773 [details] [diff] [review]
mozilla::Monitor

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

Can you provide some explanation of why you want to use a Monitor instead of a CondVar?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923723 [details] [diff] [review]
part 2 - MessageEventRunnable

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

I'm assuming the contents of the code was just moved with no significant changes.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923724 [details] [diff] [review]
part 3 - Worker and ChromeWorker classes

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

Overall looks like a good approach.  Will be great to split up WorkerPrivate files.

I'm concerned with the ref-cycle, though:

  Worker -> Worker::mWorkerPrivate -> WorkerPrivate -> WorkerPrivate::mParentEventTarget -> Worker

Is we you break this in the WorkerFinishedRunnable when we clear our self ref in the past, but does this prevent the DOM Worker object from being collected triggering idle shutdown?

For example:

  function foo() {
    var w = new Worker('data:text/javascript,setInterval(_ = {}, 0);');
  }
  foo();
  // execute GC collecting the Worker object from foo() call
  // worker thread should start shutdown and stop

Does the ref-cycle prevent this from working any more?  I believe this works today via:

https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4086

But it seems this patch removes that code.  Is there a replacement mechanism I did not see?

::: dom/workers/ChromeWorker.cpp
@@ +57,5 @@
> +  : Worker(aGlobalObject, Move(aWorkerPrivate))
> +{}
> +
> +ChromeWorker::~ChromeWorker()
> +{}

= default?

::: dom/workers/Worker.cpp
@@ +55,5 @@
> +    return true;
> +  }
> +
> +  // Else check the pref.
> +  return Preferences::GetBool(PREF_WORKERS_ENABLED);

Maybe we should consider removing the pref at some point.

@@ +64,5 @@
> +  : DOMEventTargetHelper(aGlobalObject)
> +  , mWorkerPrivate(Move(aWorkerPrivate))
> +{
> +  MOZ_ASSERT(mWorkerPrivate);
> +  mWorkerPrivate->SetParentEventTargetRef(this);

This creates a cycle between your new Worker class and the WorkerPrivate.  How can the Worker DOM object be GC'd without an explicit termination of the worker thread?  Should this Worker::mWorkerPrivate ref replace the WorkerPrivate::mSelfRef?

@@ +68,5 @@
> +  mWorkerPrivate->SetParentEventTargetRef(this);
> +}
> +
> +Worker::~Worker()
> +{}

~Worker() = default ?

@@ +79,5 @@
> +
> +void
> +Worker::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
> +                    const Sequence<JSObject*>& aTransferable,
> +                    ErrorResult& aRv)

NS_ASSERT_OWNINGTHREAD(Worker)

@@ +131,5 @@
> +  }
> +}
> +
> +void
> +Worker::Terminate()

NS_ASSERT_OWNINGTHREAD(Worker)

@@ +147,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Worker,
> +                                                DOMEventTargetHelper)
> +  tmp->Terminate();

I don't see how this can run without somehow clearing the WorkerPrivate::mParentEventTarget strong reference somehow.

::: dom/workers/WorkerPrivate.cpp
@@ -3943,5 @@
> -  // ensures that the WorkerPrivate won't be deleted before we're done shutting
> -  // down the thread.
> -
> -  if (!tmp->mBusyCount && !tmp->mMainThreadObjectsForgotten) {
> -    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSelfRef)

I think removing this code prevents idle shutdown based on the Worker DOM object getting garbage collected.

::: dom/workers/WorkerPrivate.h
@@ +267,3 @@
>    // traversed by the cycle collector if the busy count is zero.
>    RefPtr<WorkerPrivate> mSelfRef;
> +  RefPtr<DOMEventTargetHelper> mParentEventTargetRef;

Probably need to note that this creates a ref-cycle and needs special handling.

@@ +1450,5 @@
>  
> +  DOMEventTargetHelper*
> +  GetParentEventTargetRef() const
> +  {
> +    return mParentEventTargetRef;

Maybe just ParentEventTargetRef() and diagnostic assert its non-nullptr?

@@ +1456,5 @@
> +
> +  void
> +  SetParentEventTargetRef(DOMEventTargetHelper* aParentEventTargetRef)
> +  {
> +    MOZ_ASSERT(!mParentEventTargetRef);

Diagnostic assert this and also that aParentEventTargetRef is non-null.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Ran out of time to review today, but probably a good place to stop until the issue in the P3 patch is addressed.  Sorry for the delays in reviewing.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923725 [details] [diff] [review]
part 4 - Get rid of WorkerPrivateParent template

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

This patch is really hard to read unfortunately.  Its intermixing the removal of WorkerPrivateParent with modifications to other existing classes.

Could you maybe split it into two patches?

1. Modify the classes that will remain such that they don't depend on WorkerPrivateParent any more.
2. Remove WorkerPrivateParent.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923731 [details] [diff] [review]
part 8 - Fixing includes

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

Would be nice to clean up our exported header locations at some point as well.  We should just install all our publicly accessible headers in the standard mozilla/dom location.  Thats a separate patch/bug, though.

Thanks for all this cleanup!

Comment 16

[Andrea Marchesini [:baku]]

>   function foo() {
>     var w = new Worker('data:text/javascript,setInterval(_ = {}, 0);');
>   }
>   foo();
>   // execute GC collecting the Worker object from foo() call
>   // worker thread should start shutdown and stop
> 
> Does the ref-cycle prevent this from working any more?  I believe this works
> today via:

No. This still works because when 'w' is GC, in UNLINK, it calls Worker::Terminate().
This starts the shutting down of the worker.

> Maybe we should consider removing the pref at some point.

Right. follow up.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 3 - Worker and ChromeWorker classes

Using weakRef instead of RefPtr. I'm going to add a test in a separate patch.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76dc85918f42
Renaming Workers.h to WorkerCommon.h, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bad9d53ed36
Moving MessageEventRunnable out of WorkerPrivate.cpp, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/f891bdb45bc7
Move WorkerLoadInfo into separate files, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a21f55ba5530
Separate files for WorkerDebugger, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8899d7c036
WorkerError in separate files, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/aed2d290201e
Move FileReaderSync into dom/file, r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5431fce5ba
Fixing includes in dom/workers, r=bkelly

Comment 19

[Andrea Marchesini [:baku]]

I want to land all the reviewed patches. The ChromeWorker and Worker objects and the removing of the WorkerPrivateParent template can be done separately.

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/76dc85918f42
https://hg.mozilla.org/mozilla-central/rev/1bad9d53ed36
https://hg.mozilla.org/mozilla-central/rev/f891bdb45bc7
https://hg.mozilla.org/mozilla-central/rev/a21f55ba5530
https://hg.mozilla.org/mozilla-central/rev/fb8899d7c036
https://hg.mozilla.org/mozilla-central/rev/aed2d290201e
https://hg.mozilla.org/mozilla-central/rev/5c5431fce5ba