Closed Bug 1379243 Opened 5 years ago Closed 4 years ago

make WorkerGlobalScope::EventTargetFor() continue to work after worker shutdown is started

Categories

(Core :: DOM: Workers, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 10 obsolete files)

2.61 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.40 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.76 KB, patch
baku
: review+
Details | Diff | Splinter Review
1019 bytes, patch
baku
: review+
Details | Diff | Splinter Review
6.90 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.24 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
In bug 1366089 we made WorkerGlobalScope::EventTargetFor() call WorkerPrivate::GetEventTarget().  This mostly works, but its not very useful since the event target just stops working the moment the worker starts shutting down.

We should make this continue to work through at least part of the Worker shutdown process.

I think the simplest thing here would be to completely remove the existing WorkerPrivate::GetEventTarget() and instead make EventTargetFor() return WorkerPrivate::ControlEventTarget() instead.

Alternatively, we could merge the two event targets into a hybrid.  It tries a normal WorkerRunnable and if that fails falls back to a control runnable.
See Also: → 1375659
This patch makes the WorkerControlEventTarget implement nsISerialEventTarget.  This is necessary in order to use it in EventTargetFor().
Attachment #8884400 - Flags: review?(amarchesini)
This is a somewhat unrelated patch, but fixes some spurious warnings in WorkerPrivate that were annoying me.
Attachment #8884401 - Flags: review?(amarchesini)
This makes WorkerGlobalScope::EventTargetFor() return the WorkerControlEventTarget.  I think this is the safest thing to do for now.  I can't change EventTarget since sync loops use it currently.

If in the future we come across some code that wants to use EventTargetFor(), but needs to maintain WorkerRunnable ordering we can then create the hybrid target I described above.  For now its more important that this target remain usable while the worker thread has not died yet.
Attachment #8884402 - Flags: review?(amarchesini)
Nothing else was using WorkerPrivate::GetEventTarget().  Likes nuke it for now.  We can resurrect if necessary.
Attachment #8884403 - Flags: review?(amarchesini)
Not a lot is using this API on workers yet, but lets do a try anyway:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdbec438943af216c87785017af63f5269975f5a
Comment on attachment 8884402 [details] [diff] [review]
P3 Make WorkerGlobalScope::EventTargetFor() return the WorkerPrivate::ControlEventTarget(). r=baku

I thought about this a bit more over the weekend.  I think we should do the "hybrid" approach instead of just using ControlEventTarget().  Using the control target could too easily result in script running during a sync event loop.
Attachment #8884402 - Flags: review?(amarchesini)
Attachment #8884403 - Flags: review?(amarchesini)
Attachment #8884401 - Attachment is obsolete: true
Attachment #8884401 - Flags: review?(amarchesini)
Comment on attachment 8884942 [details] [diff] [review]
P1 Make WorkerControlEventTarget implement nsISerialEventTarget. r=baku

Make the WorkerControlEventTarget an nsISerialEventTarget since that is the type EventTargetFor() returns.
Attachment #8884942 - Flags: review?(amarchesini)
Comment on attachment 8884943 [details] [diff] [review]
P2 Fix some spurious warnings in WorkerPrivate. r=baku

Again, this just cleans up some warnings in worker code.
Attachment #8884943 - Flags: review?(amarchesini)
Comment on attachment 8884979 [details] [diff] [review]
P3 Rename WorkerControlEventTarget and make it support a "hybrid" dispatch mode. r=baku

This modifies WorkerControlEventTarget to take a configurable behavior parameter.  You can either create the target to always dispatch control runnables as before, or you can create it in "hybrid" mode.  A hybrid target will attempt a normal runnable, but if that fails to dispatch it will fall back to a cotnrol runnable.

This also renames WorkerControlEventTarget to WorkerEventTarget.
Attachment #8884979 - Flags: review?(amarchesini)
This patch creates an mWorkerHybridEventTarget on the WorkerPrivate and exposes it via WorkerPrivate::HybridEventTarget().  It then makes WorkerScope.cpp use HybridEventTarget() for EventTargetFor().
Attachment #8884948 - Attachment is obsolete: true
Comment on attachment 8885000 [details] [diff] [review]
P4 Make worker EventTargetFor() return a "hybrid" WorkerEventTarget. r=baku

See comment 18.
Attachment #8885000 - Flags: review?(amarchesini)
Comment on attachment 8884949 [details] [diff] [review]
P5 Remove WorkerPrivate::GetEventTarget(). r=baku

This removes the now unused WorkerPrivate::GetEventTarget() method and WorkerPrivate::mEventTarget member.  The WorkerPrivate::EventTarget class is now solely used for sync loops.  I'm a bit hesitant to touch that code, so just leaving it for now.
Attachment #8884949 - Flags: review?(amarchesini)
Comment on attachment 8884950 [details] [diff] [review]
P6 Override Pre/Post dispatch methods in ExternalRunnableWrapper so it can be used off main thread. r=baku

The ExternalRunnableWrapper class has the annoying Pre/PostDispatch() assertions enforcing parent/main thread usage.  Override these so we can use the existing WorkerPrivate::MaybeWrapAsWorkerRunnable() in our hybrid event target.
Attachment #8884950 - Flags: review?(amarchesini)
Blocks: 1380320
Attachment #8884942 - Flags: review?(amarchesini) → review+
Attachment #8884943 - Flags: review?(amarchesini) → review+
Attachment #8884949 - Flags: review?(amarchesini) → review+
I want to discuss this with you. IRC?
Andrea, per our IRC conversation I made the hybrid target stop functioning after Killing is reached.  I did this by disabling the target when Killing is set instead of checking the status on every dispatch.  This avoids a lot of unnecessary locking.

So this patch "to use the hybrid code" is changed, but the previous P3 patch introducing the hybrid target is unchanged.

I do think we should re-design shutdown soon.  I'd like to do it in a way such that we end up with a common API that we can use with quantum DOM fibers as well.  I filed bug 1379209 to cover this.  We should get the right folks in a room to talk about it.
Attachment #8885000 - Attachment is obsolete: true
Attachment #8885000 - Flags: review?(amarchesini)
Attachment #8886446 - Flags: review?(amarchesini)
Comment on attachment 8886446 [details] [diff] [review]
P4 Make worker EventTargetFor() return a "hybrid" WorkerEventTarget. r=baku

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

::: dom/workers/WorkerPrivate.cpp
@@ +5067,5 @@
>          MOZ_ASSERT(!JS_IsExceptionPending(aCx));
>  
> +        // Make sure the hybrid event target stops dispatching runnables
> +        // once we reaching the killing state.
> +        mWorkerHybridEventTarget->ForgetWorkerPrivate(this);

Maybe I should do this inside the NotifyInternal() where its holding a lock.  I wasn't sure if it was safe to nest the event target lock with that one or not.  I think it would be ok.  Let me know what you think.
One more version of the patch.  I decided to disable the hybrid target while the lock is held in NotifyInternal().  Its the best way to avoid races and there should not be any chance of deadlock here, AFAICT.

I also fixed an assertion so that the hybrid target can be disabled more than once since my new code expects to do this sometimes.
Attachment #8886446 - Attachment is obsolete: true
Attachment #8886446 - Flags: review?(amarchesini)
Attachment #8886449 - Flags: review?(amarchesini)
Attachment #8884950 - Flags: review?(amarchesini) → review+
Attachment #8886449 - Flags: review?(amarchesini) → review+
Comment on attachment 8884979 [details] [diff] [review]
P3 Rename WorkerControlEventTarget and make it support a "hybrid" dispatch mode. r=baku

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

::: xpcom/threads/MozPromise.h
@@ +405,5 @@
>        }
>  
>        nsresult Cancel() override
>        {
> +        printf_stderr("### ### [%p] ResolveOrRejectRunnable::Cancel() mThenValue:%p mPromise:%p\n",

Do we really want to use printf here?
Attachment #8884979 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e732a302c5
P1 Make WorkerControlEventTarget implement nsISerialEventTarget. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4943cb22f01
P2 Fix some spurious warnings in WorkerPrivate. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/739dffee0f16
P3 Rename WorkerControlEventTarget and make it support a "hybrid" dispatch mode. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7a312cb4fc
P4 Make worker EventTargetFor() return a "hybrid" WorkerEventTarget. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2197cb6f7889
P5 Remove WorkerPrivate::GetEventTarget(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dafae4d09fb
P6 Override Pre/Post dispatch methods in ExternalRunnableWrapper so it can be used off main thread. r=baku
Depends on: 1387211
You need to log in before you can comment on or make changes to this bug.