EventTargetFor should return an nsISerialEventTarget

RESOLVED FIXED in Firefox 56

Status

()

Core
XPCOM
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 months ago
Created attachment 8877316 [details] [diff] [review]
patch

Now that we have nsISerialEventTarget, I think it makes sense for EventTargetFor() to return that. I don't expect we'll ever want to return a thread pool from this function. And making it an nsISerialEventTarget will allow us to pass the result to a MozPromise.

One hitch here is that ThrottledEventQueue also needs to be made an nsISerialEventTarget. I'm not sure why I didn't do this initially. In theory, we could make ThrottledEventTarget be templated on the base event target, and it would be serial only if the base is serial. But I don't ever expect anyone to pass a thread pool as the base target for a ThrottledEventQueue. If we ever want to do that, we could revisit this decision.
Attachment #8877316 - Flags: review?(bkelly)
(Assignee)

Updated

4 months ago
Blocks: 1372736

Comment 1

4 months ago
Comment on attachment 8877316 [details] [diff] [review]
patch

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

::: xpcom/threads/ThrottledEventQueue.h
@@ +57,5 @@
>  // shutdown.
>  //
>  // Once shutdown begins all events will bypass the queue and be dispatched
>  // straight to the underlying base target.
> +class ThrottledEventQueue final : public nsISerialEventTarget

The nsISerialEventTarget comment says:

"/**
 * A serial event target is an event dispatching interface like
 * nsIEventTarget. Runnables dispatched to an nsISerialEventTarget are required
 * to execute serially. That is, two different runnables dispatched to the
 * target should never be allowed to execute simultaneously. So, for example, a
 * thread pool should not implement nsISerialEventTarget. However, one can
 * "convert" a thread pool into an nsISerialEventTarget by putting a
 * TaskQueue in front of it.
 */"

What does this requirement mean for spun event loops?

The ThrottledEventQueue specifically queues the next runnable before executing the current runnable so that spinning the event loop will drain the queue.  This means you *can* have two runnables from ThrottledEventQueue executing at the same time.

However, nsThread has this same behavior AFAICT and I think its intended to be an nsISerialEventTarget, right?

Is the comment in nsISerialEventTarget.idl overly strict or is there actually a problem here?
Attachment #8877316 - Flags: review?(bkelly)

Comment 2

4 months ago
See question in comment 1.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 3

4 months ago
I would say that the comment is overly strict. IMO, if a promise handler spins a nested event loop, then they should be prepared for another handler to run in the middle.
Flags: needinfo?(wmccloskey)

Comment 4

4 months ago
(In reply to Bill McCloskey (:billm) from comment #3)
> I would say that the comment is overly strict. IMO, if a promise handler
> spins a nested event loop, then they should be prepared for another handler
> to run in the middle.

Can you add a clarifying comment to nsISerialEventTarget.idl explaining the allowed spun event loop behavior?
(Assignee)

Comment 5

4 months ago
Created attachment 8877783 [details] [diff] [review]
patch v2

With the comment change.
Attachment #8877316 - Attachment is obsolete: true
Attachment #8877783 - Flags: review?(bkelly)

Comment 6

4 months ago
Comment on attachment 8877783 [details] [diff] [review]
patch v2

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

::: dom/base/DispatcherTrait.h
@@ +9,5 @@
>  
>  #include "mozilla/AlreadyAddRefed.h"
>  #include "mozilla/TaskCategory.h"
>  
>  class nsIEventTarget;

I think you can remove the old nsIEventTarget forward declaration now.

@@ +31,5 @@
>                              TaskCategory aCategory,
>                              already_AddRefed<nsIRunnable>&& aRunnable);
>  
>    // This method may or may not be safe off of the main thread. For nsIDocument
>    // it is safe. For nsIGlobalWindow it is not safe. The nsIEventTarget can

nit: comment still references nsIEventTarget

::: dom/base/nsPIDOMWindow.h
@@ +28,5 @@
>  class nsICSSDeclaration;
>  class nsIDocShell;
>  class nsIDocShellLoadInfo;
>  class nsIDocument;
>  class nsIEventTarget;

I think you can remove the nsIEventTarget forward declaration now.  There are no more references in this file.
Attachment #8877783 - Flags: review?(bkelly) → review+

Comment 7

4 months ago
Another thing I like about this change is it prevents someone from passing a thread pool (not an nsISerialEventTarget) as the base to ThrottledEventQueue.  That is good because ThrottledEventQueue does not function as a serial target in that case.

Comment 8

4 months ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8408c88a471c
EventTargetFor should return an nsISerialEventTarget (r=bkelly)
https://hg.mozilla.org/mozilla-central/rev/8408c88a471c
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.