1372733 - EventTargetFor should return an nsISerialEventTarget

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

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

Attachment: 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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

See question in comment 1.

Comment 3

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

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(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?

Comment 5

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

Attachment: patch v2

With the comment change.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8408c88a471c
EventTargetFor should return an nsISerialEventTarget (r=bkelly)

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8408c88a471c