Closed Bug 1372733 Opened 3 years ago Closed 3 years ago

EventTargetFor should return an nsISerialEventTarget

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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)
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)
See question in comment 1.
Flags: needinfo?(wmccloskey)
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)
(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?
Attached patch patch v2Splinter Review
With the comment change.
Attachment #8877316 - Attachment is obsolete: true
Attachment #8877783 - Flags: review?(bkelly)
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+
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.
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.