1320753 - Refactor the DocGroup event dispatching API

Status: RESOLVED FIXED

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

Description

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

This bug covers the follow-on work described in bug 1305926 comment 38 and comment 39. Quoting:

1. It doesn't make a ton of sense for the name to be part of the event target. Instead, we could add a getter for it to nsIRunnable and add a SetName method to Runnable. People can invoke this as they see fit.

2. That means that each DocGroup would only have one event target per TaskCategory. Each DocGroup could save the event target for each task category in an array to avoid creating duplicates.

3. We'll change the ThrottledEventQueue for workers to feed directly into the TabGroup instead of into the window's ThrottledEventQueue. That way we could give more specific TaskCategories to these two queues.

4. Make the nsIEventTarget associated with the timer category be a ThrottledEventQueue.  The API could be written in terms of nsIEventTarget with a QI interface for code that wants to call the ThrottledEventQueue Length()/AwaitIdle()/etc methods.

Comment 1

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

Attachment: Add nsINamed

This patch adds nsINamed (a generic thing that runnables and timers can use). mozilla::Runnable inherits from it. The name is stored as a string literal (for space reasons), but is queried as an nsCString. That way, more complex objects can expose non-literal names.

Comment 2

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

Attachment: Given MessageChannel runnable a name

This patch names the MessageTask runnable with the name of the message being dispatched.

Comment 3

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

Attachment: Make nsITimer runnable implement nsINamed

This patch causes the runnable dispatched for nsITimers to support nsINamed. The name comes from a name that we already store on timers.

Comment 4

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

Attachment: CreateEventTarget -> EventTargetFor

This patch is kind of the main refactoring.

1. Since we won't be creating fresh event targets anymore, CreateEventTarget is renamed to EventTargetFor.
2. Event targets are eagerly created for each TaskCategory when a TabGroup is created. These are what EventTargetFor returns.
3. The Dispatch methods on various classes still take a name (to encourage people to provide one) and they use nsINamed::SetName on the runnable that's passed in.
4. I added const to some methods because I thought I would need it. It turned out I didn't. I don't think the extra qualifier does any harm, so I left it in the patch. Can remove it if desired.

Comment 5

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

Attachment: ThrottledEventQueue refactoring

This part handles ThrottledEventQueue. With this patch there are now two ThrottledEventQueues: one for workers and one for timers. I ended up not using the do_QueryObject thing and instead kept separate pointers. This just seems easier.

I'm not entirely sure that it's correct to enable backpressure for workers when setTimeout runs. But I think that's the way it works now, so I've left that alone.

Comment 6

[(no longer active)]

Comment on attachment 8815013 [details] [diff] [review]
Add nsINamed

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

::: xpcom/glue/nsThreadUtils.cpp
@@ +44,5 @@
>  
> +NS_IMPL_ISUPPORTS(Runnable, nsIRunnable, nsINamed)
> +
> +Runnable::Runnable()
> +  : mName(nullptr)

You need the #ifdef's here as well but I suggest changing the declaration in the header to assign a default of nullptr right there so that you can save one of these #ifdef's.

::: xpcom/threads/nsINamed.idl
@@ +8,5 @@
> +/**
> + * Represents an object with a name, such as a runnable or a timer.
> + */
> +
> +[scriptable, function, uuid(0c5fe7de-7e83-4d0d-a8a6-4a6518b9a7b3)]

You don't want [function] here...

Comment 7

[(no longer active)]

Comment on attachment 8815017 [details] [diff] [review]
Make nsITimer runnable implement nsINamed

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

r=me with the below fixed.

::: xpcom/threads/nsTimerImpl.cpp
@@ +656,5 @@
> +{
> +  switch (mCallbackType) {
> +    case CallbackType::Function:
> +      if (mName.is<NameString>()) {
> +        aName = nsDependentCString(mName.as<NameString>());

Technically returning an nsDependentCString here will mean that the caller must know that the lifetime of the string is the same as the lifetime of the timer object, but see below.

@@ +661,5 @@
> +      } else if (mName.is<NameFunc>()) {
> +        static const size_t buflen = 1024;
> +        char buf[buflen];
> +        mName.as<NameFunc>()(mITimer, mClosure, buf, buflen);
> +        aName = nsCString(buf);

This one is not OK though, you're returning a reference to a temporary object which dies at the end of this statement.  Attempting to use aName past this point is UAF.

I think you can avoid both of these issues by calling the Assign() function that takes a const char*.

Comment 8

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

(In reply to :Ehsan Akhgari from comment #7)
> Comment on attachment 8815017 [details] [diff] [review]
> Make nsITimer runnable implement nsINamed
> 
> Review of attachment 8815017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the below fixed.
> 
> ::: xpcom/threads/nsTimerImpl.cpp
> @@ +656,5 @@
> > +{
> > +  switch (mCallbackType) {
> > +    case CallbackType::Function:
> > +      if (mName.is<NameString>()) {
> > +        aName = nsDependentCString(mName.as<NameString>());
> 
> Technically returning an nsDependentCString here will mean that the caller
> must know that the lifetime of the string is the same as the lifetime of the
> timer object, but see below.

My understanding of the contract here is that the NameString variant is supposed to be a string literal. So it should be valid forever. Existing code requires the name to live as long as the timer, and that's all this patch require too I think.

> @@ +661,5 @@
> > +      } else if (mName.is<NameFunc>()) {
> > +        static const size_t buflen = 1024;
> > +        char buf[buflen];
> > +        mName.as<NameFunc>()(mITimer, mClosure, buf, buflen);
> > +        aName = nsCString(buf);
> 
> This one is not OK though, you're returning a reference to a temporary
> object which dies at the end of this statement.  Attempting to use aName
> past this point is UAF.

Doesn't the nsCString constructor just call Assign? I find it impossible to understand this code, but it looks like it does:
http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/xpcom/string/nsTString.h#41

Comment 9

[(no longer active)]

(In reply to [PTO to Dec5] Bill McCloskey (:billm) from comment #8)
> (In reply to :Ehsan Akhgari from comment #7)
> > Comment on attachment 8815017 [details] [diff] [review]
> > Make nsITimer runnable implement nsINamed
> > 
> > Review of attachment 8815017 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the below fixed.
> > 
> > ::: xpcom/threads/nsTimerImpl.cpp
> > @@ +656,5 @@
> > > +{
> > > +  switch (mCallbackType) {
> > > +    case CallbackType::Function:
> > > +      if (mName.is<NameString>()) {
> > > +        aName = nsDependentCString(mName.as<NameString>());
> > 
> > Technically returning an nsDependentCString here will mean that the caller
> > must know that the lifetime of the string is the same as the lifetime of the
> > timer object, but see below.
> 
> My understanding of the contract here is that the NameString variant is
> supposed to be a string literal. So it should be valid forever. Existing
> code requires the name to live as long as the timer, and that's all this
> patch require too I think.

Oh, OK sounds good.

> > @@ +661,5 @@
> > > +      } else if (mName.is<NameFunc>()) {
> > > +        static const size_t buflen = 1024;
> > > +        char buf[buflen];
> > > +        mName.as<NameFunc>()(mITimer, mClosure, buf, buflen);
> > > +        aName = nsCString(buf);
> > 
> > This one is not OK though, you're returning a reference to a temporary
> > object which dies at the end of this statement.  Attempting to use aName
> > past this point is UAF.
> 
> Doesn't the nsCString constructor just call Assign? I find it impossible to
> understand this code, but it looks like it does:
> http://searchfox.org/mozilla-central/rev/
> 957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/xpcom/string/nsTString.h#41

Yes, but that happens during the construction of the temporary.  The assignment calls this overload: <http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/xpcom/string/nsTString.h#86> which is defined here: <http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/xpcom/string/nsTSubstring.cpp#418>.  That should do the right thing if the temporary string has the F_SHARED flag, I suppose.  Reading this code it seems like that will be the case, but I'm not 100% sure if my understanding there is correct.  So, maybe you're right that this is in fact safe.  But I will sleep a bit easier if you changed this to call Assign() directly.  At worst it will save you the cost of constructing one temporary.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97c6218a8654
Add nsINamed and have Runnable implement it (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/704915079f6b
Adding nsINamed naming to nsITimer (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac84d68036e4
Refactor for new event target idea (r=ehsan)

Comment 11

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

I wanted to get this in before the workweek. Ben, if you have any changes you'd like, I can make them after the landing.

Comment 12

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

(And, to be clear, I didn't land the last patch. It's not needed for the labeling stuff.)

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Sorry, my review queue got a bit backed up.  Working on it today.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8815022 [details] [diff] [review]
CreateEventTarget -> EventTargetFor

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

I think this is going in the right direction.  I think the API could still be improved in some places, but that could be done in follow-up bugs.

I think there is one bug, though, in that TabGroup::Dispatch() goes straight to the main thread instead of using the carefully constructed event target system.

::: dom/base/Dispatcher.h
@@ +53,5 @@
>    // 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
>    // always be used off the main thread.
>    virtual already_AddRefed<nsIEventTarget>
> +  EventTargetFor(TaskCategory aCategory) const;

Since this is no longer creating an event target I think it might be slightly nicer to just return nsIEventTarget* instead of already_AddRefed<nsIEventTarget>.  It would make it easier for code to use the event target like:

  tg->EventTargetFor(cat)->Dispatch(r.forget(), NS_DISPATCH_NORMAL);

::: dom/base/TabGroup.cpp
@@ +106,5 @@
>    MOZ_ASSERT(mWindows.Contains(aWindow));
>    mWindows.RemoveElement(aWindow);
> +
> +  if (mWindows.IsEmpty()) {
> +    mLastWindowLeft = true;

Can you MOZ_ASSERT(mLastWindowLeft) in ~TabGroup()?

@@ +189,5 @@
>                     TaskCategory aCategory,
>                     already_AddRefed<nsIRunnable>&& aRunnable)
>  {
> +  nsCOMPtr<nsIRunnable> runnable(aRunnable);
> +  if (aName) {

Any reason we are not use nsACString for the name?  I realize today you are just using literals, but adding an API is going to let people use this in other ways.  It would be nice if it took advantage of our string system by default.  I don't see why we need const char* here when this can use NS_LITERAL_CSTRING("foo") at the callsite.

@@ +190,5 @@
>                     already_AddRefed<nsIRunnable>&& aRunnable)
>  {
> +  nsCOMPtr<nsIRunnable> runnable(aRunnable);
> +  if (aName) {
> +    if (nsCOMPtr<nsINamed> named = do_QueryInterface(runnable)) {

Thanks.  This is exactly what I had in mind.

@@ +194,5 @@
> +    if (nsCOMPtr<nsINamed> named = do_QueryInterface(runnable)) {
> +      named->SetName(aName);
> +    }
> +  }
> +  return NS_DispatchToMainThread(runnable.forget());

This looks wrong.  Shouldn't it be:

  return EventTargetFor(aCategory)->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

@@ +200,5 @@
> +
> +already_AddRefed<nsIEventTarget>
> +TabGroup::EventTargetFor(TaskCategory aCategory) const
> +{
> +  MOZ_RELEASE_ASSERT(!mLastWindowLeft);

This can be called on any thread?  Can you add a comment to that effect?  I assume thats why you are using a boolean instead of just calling mWindows.IsEmpty() here.

::: dom/base/TabGroup.h
@@ +116,4 @@
>  private:
>    ~TabGroup();
>    DocGroupMap mDocGroups;
> +  bool mLastWindowLeft;

I think this should be Atomic<bool> if its accessed across threads, no?  Also, might be worth commenting that once it is set to true it should never be set back to false.

Also, its only used in assertions AFAICT.  Can you make it DebugOnly<> or #ifdef DEBUG?  (I guess DebugOnly<> is discouraged in class members now because it still requires memory footprint.)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8815024 [details] [diff] [review]
ThrottledEventQueue refactoring

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

I agree with the overall approach here, but I don't think I can agree to having:

1. TabGroup::GetThrottledEventQueue()
2. TabGroup::EventTargetFor() <- DocGroup::EventTargetFor() <- nsGlobalWindow::EventTargetFor()

It forces things that need ThrottledEventQueue functionality to go straight to the TabGroup bypassing the event target hierarchy that has been set up.  This means anyone using ThrottledEventQueue has to understand the inner workings of that hierarchy to know if that TabGroup queue is correct or not.

I think we either need:

1. TabGroup::GetThrottledEventQueue() <- DocGroup::GetThrottledEventQueue() <- nsGlobalWindow::GetThrottledEventQueue()
2. TabGroup::EventTargetFor() <- DocGroup::EventTargetFor() <- nsGlobalWindow::EventTargetFor()

Or:

1. TabGroup::EventTargetFor() <- DocGroup::EventTargetFor() <- nsGlobalWindow::EventTargetFor()
2. RefPtr<ThrottledEventQueue> throttled = do_QueryObject(EventTargetFor());

I really encourage the second option here since I think it reflects whats going on better.  There is only a single event target for a category and the queue interface is really just a type decoration of that target.

Sorry again for the delay in reviewing.

::: dom/base/nsGlobalWindow.cpp
@@ +9665,5 @@
>    if (!outer) {
>      return nullptr;
>    }
>  
> +  return TabGroup()->GetThrottledEventQueue(aCategory);

Shouldn't this use DocGroup() now that you've plumbed everything to go through that layer?  Otherwise when you change DocGroup to do additional work this one call site won't get the effect.

But I think this shows a downside of not doing the QI approach for ThrottledEventQueue.  You only plumbed EventTargetFor() through DocGroup and nsGlobalWindow.  To do this write you also need to expose the ThrottledEventQueue on these objects as well.  Otherwise you are forcing callers to implicitly understand that the window EventTargetFor(Timer) magically matches TabGroup()->GetThrottledEventQueue(Timer).

I really think it would be a lot cleaner if we just removed GetThrottledEventQueue() completely and QI'd EventTargetFor() to ThrottledEventQueue instead.  This avoids callers needing to have any special knowledge outside of their immediate code.

@@ +12044,5 @@
>        t->remove();
>        continue;
>      }
>  
> +    nsresult rv = t->InitTimer(GetThrottledEventQueue(TaskCategory::Timer), delay);

InitTimer() just needs an nsIEventTarget.  I think it would be better to just use EventTargetFor(TaskCategory::Timer) for these calls.

@@ +13234,5 @@
>  
>    mTimeoutInsertionPoint = last_insertion_point;
>  
> +  MaybeApplyBackPressure(TaskCategory::Timer);
> +  MaybeApplyBackPressure(TaskCategory::Worker);

Workers implement their own back pressure mechanism here:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4694

Since workers create their own throttled event queue they will only place a single runnable per worker in the TabGroup event target at a time.  So for workers to trigger this back pressure here would require ~5000 Worker threads.

Therefore I would prefer if you removed the argument from MaybeApplyBackPressure() and UpdateOrCancelBackPressure().  Instead just rename them to:

  MaybeApplyTimerBackPressure()
  UpdateOrCancelTimerBackPressure()

And then hard code the timer category in those methods.

If/when we add back pressure for other task categories they will likely need to operate in a different way.

::: dom/workers/WorkerPrivate.cpp
@@ +4061,5 @@
>      return;
>    }
>  
>    MOZ_ASSERT(NS_IsMainThread());
> +  target = GetWindow() ? GetWindow()->GetThrottledEventQueue(TaskCategory::Worker) : nullptr;

This can just be EventTargetFor().  This code does not actually use any of the ThrottledEventQueue-specific methods.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97c6218a8654
https://hg.mozilla.org/mozilla-central/rev/704915079f6b
https://hg.mozilla.org/mozilla-central/rev/ac84d68036e4

Comment 17

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

Attachment: stop returning already_AddRefed from EventTargetFor

This patch addresses Ben's feedback for the EventTargetFor patch. Mainly it removes the use of already_AddRefed for the return type of EventTargetFor. That does seem to make things simpler.

Comment 18

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

(In reply to Ben Kelly [:bkelly] from comment #14)
> ::: dom/base/Dispatcher.h
> @@ +53,5 @@
> >    // 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
> >    // always be used off the main thread.
> >    virtual already_AddRefed<nsIEventTarget>
> > +  EventTargetFor(TaskCategory aCategory) const;
> 
> Since this is no longer creating an event target I think it might be
> slightly nicer to just return nsIEventTarget* instead of
> already_AddRefed<nsIEventTarget>.  It would make it easier for code to use
> the event target like:
> 
>   tg->EventTargetFor(cat)->Dispatch(r.forget(), NS_DISPATCH_NORMAL);

Posted a new patch for thos.

> ::: dom/base/TabGroup.cpp
> @@ +106,5 @@
> >    MOZ_ASSERT(mWindows.Contains(aWindow));
> >    mWindows.RemoveElement(aWindow);
> > +
> > +  if (mWindows.IsEmpty()) {
> > +    mLastWindowLeft = true;
> 
> Can you MOZ_ASSERT(mLastWindowLeft) in ~TabGroup()?

I dunno. It might be useful to allow people to create a TabGroup and then delete it before adding any windows. It seems like the mWindows.IsEmpty() assertion is probably good enough.

> @@ +189,5 @@
> >                     TaskCategory aCategory,
> >                     already_AddRefed<nsIRunnable>&& aRunnable)
> >  {
> > +  nsCOMPtr<nsIRunnable> runnable(aRunnable);
> > +  if (aName) {
> 
> Any reason we are not use nsACString for the name?  I realize today you are
> just using literals, but adding an API is going to let people use this in
> other ways.  It would be nice if it took advantage of our string system by
> default.  I don't see why we need const char* here when this can use
> NS_LITERAL_CSTRING("foo") at the callsite.

Here's the rationale:
1. The param to Dispatch is really just passed to SetName on the Runnable.
2. I didn't want to add to much overhead to Runnable, so I used const char* there instead of nsCString. It looks like nsCString takes 3 words versus 1 for a char*.
3. If people want to provide a more complex name, they can override the GetName method on their runnable, which is allowed to return an nsACString.

> @@ +194,5 @@
> > +    if (nsCOMPtr<nsINamed> named = do_QueryInterface(runnable)) {
> > +      named->SetName(aName);
> > +    }
> > +  }
> > +  return NS_DispatchToMainThread(runnable.forget());
> 
> This looks wrong.  Shouldn't it be:
> 
>   return EventTargetFor(aCategory)->Dispatch(runnable.forget(),
> NS_DISPATCH_NORMAL);

Right now, event targets delegate to TabGroup::Dispatch. We can't have them delegate to each other. I could instead have TabGroup::Dispatch delegate to the event target. I'm not sure it matters though. It's just an implementation detail (which will change once we start scheduling runnables in more sophisticated ways).

Comment 19

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

Attachment: ThrottledEventQueue refactoring, v2

Comment 20

[(no longer active)]

Comment on attachment 8819074 [details] [diff] [review]
stop returning already_AddRefed from EventTargetFor

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

::: dom/base/TabGroup.h
@@ +130,5 @@
>    void EnsureThrottledEventQueues();
>  
>    ~TabGroup();
>    DocGroupMap mDocGroups;
> +  Atomic<bool> mLastWindowLeft;

Does this change belong in this patch?

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8819080 [details] [diff] [review]
ThrottledEventQueue refactoring, v2

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

Awesome.  Thanks so much for doing this! r=me

::: dom/base/nsGlobalWindow.cpp
@@ +13336,5 @@
>        InsertTimeoutIntoList(timeout);
>        timeout->mFiringDepth = firingDepth;
>        timeout->Release();
>  
> +      nsresult rv = timeout->InitTimer(EventTargetFor(TaskCategory::Timer),

Note, you are going to conflict with the work Ehsan has landed and is still writing patches for.  You might want to coordinate landings with him.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f980cbff8052
Stop using already_AddRefed for EventTargetFor (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9f3f0d6d8e
Make TabGroup event target be a ThrottledEventQueue for timers, workers (r=bkelly)

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f980cbff8052
https://hg.mozilla.org/mozilla-central/rev/4e9f3f0d6d8e