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

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: billm, Assigned: bkelly)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
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?
Flags: needinfo?(bkelly)
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?
Flags: needinfo?(bkelly)
(Assignee)

Updated

2 years ago
Blocks: 1293277
(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.
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #8870136 - Flags: review?(bkelly)
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.
Attachment #8870136 - Flags: review?(bkelly)
(Assignee)

Updated

2 years ago
Assignee: wmccloskey → bkelly
(Assignee)

Updated

2 years ago
Attachment #8870136 - Attachment is obsolete: true
Attachment #8880921 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
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().
Attachment #8881264 - Attachment is obsolete: true
Attachment #8881859 - Flags: review?(amarchesini)
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
Attachment #8881859 - Flags: review?(amarchesini) → review+

Comment 15

2 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f83eb621e672
Make EventTargetFor() support worker threads. r=baku

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f83eb621e672
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

2 years ago
Depends on: 1379243
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.