Some cleanup in WorkerPrivate.{h,cpp}

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(6 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
This is needed to avoid a name conflict between Worker class, in a Worker.h file, and the current Workers.h file.
Assignee: nobody → amarchesini
Attachment #8923722 - Flags: review?(bkelly)
(Assignee)

Comment 2

2 years ago
MessageEventRunnable will be used by Worker class. Let's move it into a separate file.
Attachment #8923723 - Flags: review?(bkelly)
(Assignee)

Comment 3

2 years ago
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.
Attachment #8923724 - Flags: review?(bkelly)
(Assignee)

Comment 4

2 years ago
Attachment #8923725 - Flags: review?(bkelly)
(Assignee)

Comment 5

2 years ago
Separate files for WorkerLoadInfo.
Attachment #8923727 - Flags: review?(bkelly)
(Assignee)

Comment 7

2 years ago
Attachment #8923729 - Flags: review?(bkelly)
(Assignee)

Comment 8

2 years ago
Attachment #8923731 - Flags: review?(bkelly)
(Assignee)

Comment 9

2 years ago
Posted patch mozilla::Monitor (obsolete) — Splinter Review
Attachment #8923773 - Flags: review?(bkelly)
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?
Attachment #8923773 - Flags: review?(bkelly)
Priority: -- → P2
Attachment #8923722 - Flags: review?(bkelly) → review+
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.
Attachment #8923723 - Flags: review?(bkelly) → review+
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.
Attachment #8923724 - Flags: review?(bkelly) → review-
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 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.
Attachment #8923725 - Flags: review?(bkelly)
Attachment #8923727 - Flags: review?(bkelly) → review+
Attachment #8923728 - Flags: review?(bkelly) → review+
Attachment #8923729 - Flags: review?(bkelly) → review+
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!
Attachment #8923731 - Flags: review?(bkelly) → review+
(Assignee)

Comment 16

a year ago
>   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.
(Assignee)

Comment 17

a year ago
Using weakRef instead of RefPtr. I'm going to add a test in a separate patch.
Attachment #8923724 - Attachment is obsolete: true
Attachment #8933392 - Flags: review?(bkelly)
(Assignee)

Updated

a year ago
Attachment #8933392 - Flags: review?(bkelly)
(Assignee)

Updated

a year ago
Attachment #8923725 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8923773 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8933392 - Attachment is obsolete: true

Comment 18

a year ago
Pushed by amarchesini@mozilla.com:
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
(Assignee)

Comment 19

a year ago
I want to land all the reviewed patches. The ChromeWorker and Worker objects and the removing of the WorkerPrivateParent template can be done separately.
You need to log in before you can comment on or make changes to this bug.