1366089 - nsIGlobalObject::EventTargetFor should return the worker thread for worker globals

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Right now, nsIGlobalObject::EventTargetFor returns the main thread for all non-nsGlobalWindow globals. It would make sense for it to return the worker thread for any worker globals.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I started to work on a patch for this, but I'm not sure what event target to return. Can I just return mWorkerPrivate->mThread? Will that work if the worker hasn't started up yet or something (is that possible once we have a global for it)? Also, how should I deal with mThread being private? Can I make an accessor, or should I make WorkerGlobalScope a friend class?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Try WorkerPrivate::GetEventTarget():

http://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#3001

It can return null, though.  I guess the nsIGlobalObject method should return a failure code in that case?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #2)
> I guess the nsIGlobalObject method should
> return a failure code in that case?

Also Dispatch() can return failure if you hold the event target past the lifetime of the thread.

I think these are just the reality of working with an nsIEventTarget representing an ephemeral thread.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

There were a couple annoying issues here:

1. AbstractMainThreadFor doesn't make a lot of sense for workers, so I just made it assert.

2. EventTargetFor is never supposed to return null (based on the name), but mWorkerPrivate->GetEventTarget() can return null. So I invented this FakeEventTarget thing that always returns an error when you dispatch to it. I'm not very happy with the name.

3. EventTargetFor doesn't return an already_AddRefed, so someone else needs to own the FakeEventTarget. So I created it in the WorkerGlobalScope constructor.

Happy to take suggestions about better ways to do this.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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

So, I think we should probably just make WorkerPrivate::GetEventTarget() an infallible EventTarget() method instead.  It can do the Dispatch() failing if the thread is not active.

I'll steal this bug to make that change.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make EventTargetFor() support worker threads. r=billm

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89cc767811f07e6b5ae827988ce297be8a277ec2

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make EventTargetFor() support worker threads. r=billm

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make EventTargetFor() support worker threads. r=billm

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4d961e69eaf9ae2a03e771018944742a596eafb

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make EventTargetFor() support worker threads. r=billm

https://treeherder.mozilla.org/#/jobs?repo=try&revision=606a44f2447ff118fcc13b1f04d425a81ae5166a

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make EventTargetFor() support worker threads. r=billm

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b7db5c99fe172ff731917cc5922dac9f1807025

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=322736fa59f407f3d9814c50c5311597280d2fb0

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dca9c021db05cd222822be7add029cf970bc96f1

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make EventTargetFor() support worker threads. r=baku

Andrea, this patch does a few things:

1) Make WorkerPrivate::GetEventTarget() always return an nsISerialEvent target.  If we would have returned nullptr before we now just Disable() the event target instead.  So we have an event target, but any Dispatch() will return an error.
2) Drop the parent status check in GetEventTarget().  This makes the check match the WorkerHolder checks.  I don't think the parent check is really necessary or makes sense here.
3) Make the WorkerGlobalScope::EventTargetFor() return the WorkerPrivate::GetEventTarget().

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8881859 [details] [diff] [review]
Make EventTargetFor() support worker threads. r=baku

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

::: dom/workers/WorkerScope.cpp
@@ +493,5 @@
>    }
>  }
>  
> +nsresult
> +WorkerGlobalScope::Dispatch(const char* aname, TaskCategory aCategory,

aName

@@ +1077,5 @@
>    }
>  }
>  
> +nsresult
> +WorkerDebuggerGlobalScope::Dispatch(const char* aname, TaskCategory aCategory,

aName

::: dom/workers/WorkerScope.h
@@ +208,5 @@
> +  // Override DispatchTrait API to target the worker thread.  Dispatch may
> +  // return failure if the worker thread is not alive.
> +  nsresult
> +  Dispatch(const char* aname, TaskCategory aCategory,
> +           already_AddRefed<nsIRunnable>&& aRunnable) override;

aName

@@ +420,5 @@
>  
> +  // Override DispatchTrait API to target the worker thread.  Dispatch may
> +  // return failure if the worker thread is not alive.
> +  nsresult
> +  Dispatch(const char* aname, TaskCategory aCategory,

aName

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f83eb621e672
Make EventTargetFor() support worker threads. r=baku

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f83eb621e672