1305926 - API for assigning DocGroups to runnables

Status: RESOLVED FIXED

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

Description

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

I started thinking about what this API should look like. I wanted to post something preliminary so that we can discuss it tomorrow. This API depends on the DocGroup stuff in bug 1303196. The basic idea is to be able to label each runnable with some information:
- a name
- categorization (UI, DOM, networking, etc.)
- DocGroup it's allowed to touch
Each runnable is allowed to touch at most one DocGroup.

The API will only be used for dispatching to the main thread, although it could be useful to generalize it if there are places where we generically dispatch to the main thread or some other thread.

Comment 1

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

Attachment: API

This is kind of a start. There's a Dispatcher class, which takes the necessary arguments. It's default implementation is to dispatch to the main thread. nsIGlobalObject and nsIDocument inherit from it. This allows you to dispatch to these classes.

One piece that I'm still working out is thread safety. In order to dispatch off the main thread, we need to get a Dispatcher instance that we can use safely. It would be sort of nice if we could use AddRef/Release on that instance, although not strictly necessary. So I also made DocGroup be a Dispatcher.

I'm not sure if it should be legal to call nsIDocument::Dispatch or nsIGlobalWindow::Dispatch off the main thread. The ways they look up their current document group is not thread safe right now. We could probably change that pretty easily for nsIDocument, and maybe with more trouble for nsIGlobalWindow. We could also just use a lock, but I would kinda like to avoid that.

From here, it shouldn't be too hard to assert that a labelled runnable only touches its own DocGroup. We can add assertions to DOM event dispatch and JS execution, for example. We can also add telemetry for how long these runnables take.

We'll also need to add mechanisms to simplify using Dispatch for timers, IPC code, media code (including moz promises and task queues), and workers.

Comment 2

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

Attachment: examples

I started doing some labelling just to get a feel for it.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

I think it would be reasonable to make DocGroup and TabGroup both be threadsafe refcounted, and to make the debug only assertions in the DocGroup/TabGroup getters be guarded by some sort of `if main thread` method. That way we could make them threadsafe to run and thus make it safe to get DocGroup/TabGroup from the document from off of the main thread.

Comment 4

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

Ehsan's feedback on the thread safety issue was to figure out if we ever post runnables between when the initial docgroup is created and when we set the final docgroup via SetOpenerWindow. He points out that this isn't really a thread safety issue but just an issue of having the correct docgroup when we need it. I can add some assertions about that and push to try.

Comment 5

[(no longer active)]

Comment on attachment 8795604 [details] [diff] [review]
API

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

::: dom/base/nsDocument.cpp
@@ +12914,5 @@
>    return is;
>  }
> +
> +DocGroup*
> +nsDocument::GetDocGroup()

Why does this need to be a virtual function?

@@ +12930,5 @@
> +                     TimeStamp aSoftDeadline,
> +                     TimeStamp aHardDeadline)
> +{
> +  // FIXME: In order for this to be safe to run on other threads, we need to
> +  // cache the DocGroup in the document.

Comment 4 summarizes my thoughts about this pretty well.  I really don't want to start making things thread safe, at least before we understand the use case of dispatching runnables before the real docgroup is determined.

::: xpcom/threads/Dispatcher.h
@@ +30,5 @@
> +  // Vsync notifications
> +  RefreshDriver,
> +
> +  // Most DOM events (postMessage, media, plugins)
> +  DOM,

What do you think about calling this Other?

Comment 6

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

(In reply to :Ehsan Akhgari from comment #5)
> > +DocGroup*
> > +nsDocument::GetDocGroup()
> 
> Why does this need to be a virtual function?

I want it to be defined on nsIDocument so that people who have an nsIDocument can use it without casting. I guess I could put the whole implementation in nsIDocument. That felt sorta wrong since nsIDocument doesn't even have a .cpp file. But I now see that some nsIDocument methods are defined in nsDocument.cpp. So maybe I'll just do that :-). I assume there's some plan to unify these classes eventually?

> ::: xpcom/threads/Dispatcher.h
> @@ +30,5 @@
> > +  // Vsync notifications
> > +  RefreshDriver,
> > +
> > +  // Most DOM events (postMessage, media, plugins)
> > +  DOM,
> 
> What do you think about calling this Other?

Sure, sounds fine.

Comment 7

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #6)
> (In reply to :Ehsan Akhgari from comment #5)
> > > +DocGroup*
> > > +nsDocument::GetDocGroup()
> > 
> > Why does this need to be a virtual function?
> 
> I want it to be defined on nsIDocument so that people who have an
> nsIDocument can use it without casting. I guess I could put the whole
> implementation in nsIDocument. That felt sorta wrong since nsIDocument
> doesn't even have a .cpp file. But I now see that some nsIDocument methods
> are defined in nsDocument.cpp. So maybe I'll just do that :-).

Yep.  :-)

> I assume
> there's some plan to unify these classes eventually?

You're assuming there's a plan?  ;-)

More seriously speaking, there's no real distinction between nsIDocument and nsDocument, except that nsDocument implements a whole bunch of interfaces alongside nsIDocument.  It's hard to blame nsIDocument for having state and concrete methods and whatnot given that it inherits from nsINode.  And given all of this, I don't really think we can ever merge these two classes together.  :(


---

Following up on comment 4, I talked with Bill and Michael on IRC about bug 1303196.  It seems possible to move setting the opener to happen a bit earlier and eliminate the need for the transitional state where our docgroup may change, and if that approach works out fine, then we won't have this problem to begin with.  I think we should wait for Michael to try that out first.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

After taking IDB as an example to understand more about the use cases of
this API, I found several difficulty for labeling docgroup to the runnable, 
1. It is difficult for the implementation which has no access to the 
   its window.
   For example, the runnable that processes received IPC messages in 
   both child or parent process.
2. It's difficult to identify whether the runnable is dispatched to the
   main thread or not if we call nsThread::Dispatch() to a refPtr which
   points to the main thread.
3. It's difficult to label a main thread runnable dispatched after a chain
   of off-thread runnables.

After some off-line discussion with :cervantes,
With the assumption that 
1. all runnables in/off main thread in both child/parent processes shall be 
   labeled.
2. each runnable will be labeled with the same docgroup of current runnable
   of the dispatching thread.
If we can implicitly enqueue current docgroup info in nsThread::Dispatch()
at the same time when runnable is enqueued by some API that exposed from
NS_GetCurrentThread(), and dequeue this info when runnable is done, then
we can reduce some difficulty for labeling.

Then, the problems remained are 
1. how to identify all the entry points for setting current docgroup info.
   (A new Dispath() API is required here to explicity specify the docgroup
    when dispatching a runnable)
   So far I know is that
   - in e10s, in the process boundary, we have to set current docgroup to 
     the runnable that processes received IPC messages.
   - any input event dispatched to specific web content.
     (Where is the exact entry point for these input events)
   - anything else?
2. how can current docgroup info be retrieved at these entry points.

Note: TaskCategory and deadlines are not taken into consideration yet in this approach.

Will this approach be better?

More information of all the possible entry points is welcome.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> After some off-line discussion with :cervantes,
> With the assumption that 
> 1. all runnables in/off main thread in both child/parent processes shall be 
>    labeled.
> 2. each runnable will be labeled with the same docgroup of current runnable
>    of the dispatching thread.
Note: There is some exception in MessageLoop::PostTask_Helper(), if there is no GetXPCOMThread available, the runnable will be dispatched to an incoming_queue_ instead.
If the runnables in |incoming_queue_| will dispatch new runnables to other nsThread, then we have to figure out how to dispatch labeling info explicitly.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

NI for the feedback of the suggestion in comment 8.

Comment 11

[(no longer active)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> After taking IDB as an example to understand more about the use cases of
> this API, I found several difficulty for labeling docgroup to the runnable, 
> 1. It is difficult for the implementation which has no access to the 
>    its window.
>    For example, the runnable that processes received IPC messages in 
>    both child or parent process.
> 2. It's difficult to identify whether the runnable is dispatched to the
>    main thread or not if we call nsThread::Dispatch() to a refPtr which
>    points to the main thread.
> 3. It's difficult to label a main thread runnable dispatched after a chain
>    of off-thread runnables.

Can you please point to the relevant runnables in each of these cases?  I'm not familiar with the IDB code, so I'm not sure where these problems are occurring, which makes it hard to suggest solutions.

> After some off-line discussion with :cervantes,
> With the assumption that 
> 1. all runnables in/off main thread in both child/parent processes shall be 
>    labeled.

Hmm, I was really hoping that we can avoid labeling anything in the parent process.  So far our notion of TabGroup and DocGroup is restricted to one content process, so we can't even have unique Doc/TabGroups in multiple child processes.

What's an example of a case where labeling runnables in the parent process is needed?

> 2. each runnable will be labeled with the same docgroup of current runnable
>    of the dispatching thread.
> If we can implicitly enqueue current docgroup info in nsThread::Dispatch()
> at the same time when runnable is enqueued by some API that exposed from
> NS_GetCurrentThread(), and dequeue this info when runnable is done, then
> we can reduce some difficulty for labeling.

If I'm reading this correctly, you mean using the label information of the current runnable which is being processed to populate the info for a newly dispatched runnable.  If this is accurate, then this matches what I had in mind as well.

> Then, the problems remained are 
> 1. how to identify all the entry points for setting current docgroup info.
>    (A new Dispath() API is required here to explicity specify the docgroup
>     when dispatching a runnable)

Basically enumerate all of the callsites where we call the various dispatch functions (such as NS_DispatchToCurrentThread, NS_DispatchToMainThread and friends.)

>    So far I know is that
>    - in e10s, in the process boundary, we have to set current docgroup to 
>      the runnable that processes received IPC messages.

Again my hope was to avoid doing this.

>    - any input event dispatched to specific web content.
>      (Where is the exact entry point for these input events)

The assumption here was that the input event will always be received and processed by foreground page in the content process, and will never be preempted, so we don't need accurate information about the target tab/docgroup for these events.  We just treat them with the highest priority.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to :Ehsan Akhgari from comment #11)
> Can you please point to the relevant runnables in each of these cases?  I'm
> not familiar with the IDB code, so I'm not sure where these problems are
> occurring, which makes it hard to suggest solutions.
Sure,
The core of IDB implementation were done by PBackground ipdl.
With this design, all the requests will be done in PBackgroundActors in parent and ipc messages of the responses will be received in PBackgroundActorChilds, if there is a background child instantiated in child's main thread, by ipdl design, the response message will be wrapped with a runnable dispatched to this main thread to handle the response by invoking the RecvXXX in the ActorChild implementation.
The runnable is posted at:
http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/ipc/chromium/src/base/message_loop.cc#286

Here is a callstack in the child's main thread for the example:
>    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsChild.cpp:1690
>#1  0x00007f06f7ac499d in mozilla::dom::indexedDB::PBackgroundIDBDatabaseChild::OnMessageReceived (
>    this=0x7f06e7ed8540, msg__=...)
>    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/ipc/ipdl/PBackgroundIDBDatabaseChild.cpp:655
>#2  0x00007f06f77d6c67 in mozilla::ipc::PBackgroundChild::OnMessageReceived (this=0x7f06cd24b800, 
>    msg__=...)
>    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/ipc/ipdl/PBackgroundChild.cpp:1551
>#3  0x00007f06f77394dd in mozilla::ipc::MessageChannel::DispatchAsyncMessage (this=0x7f06cd24b830, 
>    aMsg=...) at /home/bevis/Projects/Build/gecko/src/ipc/glue/MessageChannel.cpp:1663
>#4  0x00007f06f7738f87 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (
>    this=0x7f06cd24b830, 
>    aMsg=<unknown type in /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/bin/libxul.so, CU 0x22a22c3, DIE 0x23415fe>) at /home/bevis/Projects/Build/gecko/src/ipc/glue/MessageChannel.cpp:1601
>#5  0x00007f06f7738ca4 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (this=0x7f06cd24b830)
>    at /home/bevis/Projects/Build/gecko/src/ipc/glue/MessageChannel.cpp:1568
>#6  0x00007f06f77577f2 in mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (o=0x7f06cd24b830, 
>    m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f06f7738b2e <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, args=...)
>    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/nsThreadUtils.h:729
>#7  0x00007f06f7757469 in mozilla::detail::RunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)()) (this=0x7f06cd2309b8, o=0x7f06cd24b830, 
>    m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f06f7738b2e <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>)
>    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/nsThreadUtils.h:736
>#8  0x00007f06f7756e0d in mozilla::detail::RunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run (this=0x7f06cd230980)
>    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/nsThreadUtils.h:764
>#9  0x00007f06f7745261 in mozilla::ipc::MessageChannel::RefCountedTask::Run (this=0x7f06cd3d37f0)
>    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/mozilla/ipc/MessageChannel.h:540
>#10 0x00007f06f7745472 in mozilla::ipc::MessageChannel::DequeueTask::Run (this=0x7f06c9405d90)
>    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/mozilla/ipc/MessageChannel.h:559

IMO, these runnables shall be labeled with proper docgroup because it's related to task of a specific window.
The same situation happens in any PContent ipc messages in child because they will be handled in a runnable dispatched to the main thread of the child process.
In addition, if only the runnables in main thread has to be labeled, then the labeling for this case will be more complicated because PBackgroundChild instances could be created in main thread and other worker threads.

> 
> > After some off-line discussion with :cervantes,
> > With the assumption that 
> > 1. all runnables in/off main thread in both child/parent processes shall be 
> >    labeled.
> 
> Hmm, I was really hoping that we can avoid labeling anything in the parent
> process.  So far our notion of TabGroup and DocGroup is restricted to one
> content process, so we can't even have unique Doc/TabGroups in multiple
> child processes.
Does that mean this labeling stuff will only be applied in e10s-single and not in e10s-multi and e10s-disabled?
> 
> What's an example of a case where labeling runnables in the parent process
> is needed?
> 
In IDB's ActorParent running in PBackground thread, some part of task such as permission check has to be delegated to the main thread, then continue the remained part of task back in PBackground Thread after permission check is done in main thread:
http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/dom/indexedDB/ActorsParent.cpp#20744

So, this runnable dispatched to the main thread in parent is related an operation of a window in the child process.
In addition, if we take the ipc messages received by PContentParent and its derivatives, then these IPC messages will also be wrapped with a runnable and dispatched to the main thread of the parent.
IMO, these runnables in parent are supposed to be labeled, isn't it?
Or we just assume that the runnables in parent are priority-less because the priority has been decided in child process, so the runnables dispatched to the parent process has certain level of the relationship to the foreground windows in child process.
> If I'm reading this correctly, you mean using the label information of the
> current runnable which is being processed to populate the info for a newly
> dispatched runnable.  If this is accurate, then this matches what I had in
> mind as well.
Yes, but it will be less meaningful if we cannot label runnables as much as possible including the ones dispatched to non-main threads.
IMO, the dispatching sequence of a runnable to the main thread could begin at any thread not only from the main thread.
Labeling ruunables as much as we can helps us a lot to implicitly label the docgroup to the runnables in all threads without modifying all the implementation that invokes NS_DispatchToCurrentThread, NS_DispatchToMainThread and friends.

With this implicit labeling design, the only thing we need to take care is the entry point that needs *explicit labeling*, like the IPC messages mentioned above.

The entry points here are the interrupts to the process i.e. gecko that could dispatch a new runnable into gecko at very beginning when the docgroup info is available.

> The assumption here was that the input event will always be received and
> processed by foreground page in the content process, and will never be
> preempted, so we don't need accurate information about the target
> tab/docgroup for these events.  We just treat them with the highest priority.

Yes, the input event will be in highest priority, but while processing an input event, further runnables/tasks related to a window could be triggered and are supposed to be labeled.
The idea is that if we can label the runnable as early as possible in the dispatching sequence, the less we have to modify the use cases that invoke NS_DispatchToCurrentThread, NS_DispatchToMainThread and their friend APIs.
Hence, labeling these input events if we can looks important to me for implicit labeling.

Comment 13

[(no longer active)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #12)
> (In reply to :Ehsan Akhgari from comment #11)
> > Can you please point to the relevant runnables in each of these cases?  I'm
> > not familiar with the IDB code, so I'm not sure where these problems are
> > occurring, which makes it hard to suggest solutions.
> Sure,
> The core of IDB implementation were done by PBackground ipdl.
> With this design, all the requests will be done in PBackgroundActors in
> parent and ipc messages of the responses will be received in
> PBackgroundActorChilds, if there is a background child instantiated in
> child's main thread, by ipdl design, the response message will be wrapped
> with a runnable dispatched to this main thread to handle the response by
> invoking the RecvXXX in the ActorChild implementation.
> The runnable is posted at:
> http://searchfox.org/mozilla-central/rev/
> 2142de26c16c05f23e543be4fa1a651c4d29604e/ipc/chromium/src/base/message_loop.
> cc#286
> 
> Here is a callstack in the child's main thread for the example:
> >    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsChild.cpp:1690
> >#1  0x00007f06f7ac499d in mozilla::dom::indexedDB::PBackgroundIDBDatabaseChild::OnMessageReceived (
> >    this=0x7f06e7ed8540, msg__=...)
> >    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/ipc/ipdl/PBackgroundIDBDatabaseChild.cpp:655
> >#2  0x00007f06f77d6c67 in mozilla::ipc::PBackgroundChild::OnMessageReceived (this=0x7f06cd24b800, 
> >    msg__=...)
> >    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/ipc/ipdl/PBackgroundChild.cpp:1551
> >#3  0x00007f06f77394dd in mozilla::ipc::MessageChannel::DispatchAsyncMessage (this=0x7f06cd24b830, 
> >    aMsg=...) at /home/bevis/Projects/Build/gecko/src/ipc/glue/MessageChannel.cpp:1663
> >#4  0x00007f06f7738f87 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (
> >    this=0x7f06cd24b830, 
> >    aMsg=<unknown type in /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/bin/libxul.so, CU 0x22a22c3, DIE 0x23415fe>) at /home/bevis/Projects/Build/gecko/src/ipc/glue/MessageChannel.cpp:1601
> >#5  0x00007f06f7738ca4 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (this=0x7f06cd24b830)
> >    at /home/bevis/Projects/Build/gecko/src/ipc/glue/MessageChannel.cpp:1568
> >#6  0x00007f06f77577f2 in mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (o=0x7f06cd24b830, 
> >    m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f06f7738b2e <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, args=...)
> >    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/nsThreadUtils.h:729
> >#7  0x00007f06f7757469 in mozilla::detail::RunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)()) (this=0x7f06cd2309b8, o=0x7f06cd24b830, 
> >    m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f06f7738b2e <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>)
> >    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/nsThreadUtils.h:736
> >#8  0x00007f06f7756e0d in mozilla::detail::RunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run (this=0x7f06cd230980)
> >    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/nsThreadUtils.h:764
> >#9  0x00007f06f7745261 in mozilla::ipc::MessageChannel::RefCountedTask::Run (this=0x7f06cd3d37f0)
> >    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/mozilla/ipc/MessageChannel.h:540
> >#10 0x00007f06f7745472 in mozilla::ipc::MessageChannel::DequeueTask::Run (this=0x7f06c9405d90)
> >    at /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/include/mozilla/ipc/MessageChannel.h:559
> 
> IMO, these runnables shall be labeled with proper docgroup because it's
> related to task of a specific window.
> The same situation happens in any PContent ipc messages in child because
> they will be handled in a runnable dispatched to the main thread of the
> child process.
> In addition, if only the runnables in main thread has to be labeled, then
> the labeling for this case will be more complicated because PBackgroundChild
> instances could be created in main thread and other worker threads.

OK, I think I understand the problem now.

How are these actors set up?  Can we for example guarantee that each docgroup will get its own child actor?  In that case, we can attach the label info to the actor, and have it attach them to the runnables that it creates.  Perhaps we can even do it in the IPDL layer so that we can use the same machinery elsewhere.

I'd also like to see what Bill thinks about this, as he has been thinking about the labeling issue more.

> > > After some off-line discussion with :cervantes,
> > > With the assumption that 
> > > 1. all runnables in/off main thread in both child/parent processes shall be 
> > >    labeled.
> > 
> > Hmm, I was really hoping that we can avoid labeling anything in the parent
> > process.  So far our notion of TabGroup and DocGroup is restricted to one
> > content process, so we can't even have unique Doc/TabGroups in multiple
> > child processes.
> Does that mean this labeling stuff will only be applied in e10s-single and
> not in e10s-multi and e10s-disabled?

No, we'll have the labels in every case.  It's just that in the case of e10s-multi, I'm hoping that we wouldn't need to sync up doc groups in different processes.

> > What's an example of a case where labeling runnables in the parent process
> > is needed?
> > 
> In IDB's ActorParent running in PBackground thread, some part of task such
> as permission check has to be delegated to the main thread, then continue
> the remained part of task back in PBackground Thread after permission check
> is done in main thread:
> http://searchfox.org/mozilla-central/rev/
> 2142de26c16c05f23e543be4fa1a651c4d29604e/dom/indexedDB/ActorsParent.cpp#20744
> 
> So, this runnable dispatched to the main thread in parent is related an
> operation of a window in the child process.
> In addition, if we take the ipc messages received by PContentParent and its
> derivatives, then these IPC messages will also be wrapped with a runnable
> and dispatched to the main thread of the parent.
> IMO, these runnables in parent are supposed to be labeled, isn't it?
> Or we just assume that the runnables in parent are priority-less because the
> priority has been decided in child process, so the runnables dispatched to
> the parent process has certain level of the relationship to the foreground
> windows in child process.

I think the assumption is that we don't want the separate priority queues in the parent process, so there's no need to label runnables in the parent.  I think the examples above for example can happen without explicit labeling.

> > If I'm reading this correctly, you mean using the label information of the
> > current runnable which is being processed to populate the info for a newly
> > dispatched runnable.  If this is accurate, then this matches what I had in
> > mind as well.
> Yes, but it will be less meaningful if we cannot label runnables as much as
> possible including the ones dispatched to non-main threads.
> IMO, the dispatching sequence of a runnable to the main thread could begin
> at any thread not only from the main thread.

Sure.  But still if the runnable that you're processing on thread A has label X, and it dispatches a new runnable to thread B, we can label that one as X too, right?

> Labeling ruunables as much as we can helps us a lot to implicitly label the
> docgroup to the runnables in all threads without modifying all the
> implementation that invokes NS_DispatchToCurrentThread,
> NS_DispatchToMainThread and friends.
> 
> With this implicit labeling design, the only thing we need to take care is
> the entry point that needs *explicit labeling*, like the IPC messages
> mentioned above.

Yes, that's my idea as well.

> The entry points here are the interrupts to the process i.e. gecko that
> could dispatch a new runnable into gecko at very beginning when the docgroup
> info is available.
> 
> > The assumption here was that the input event will always be received and
> > processed by foreground page in the content process, and will never be
> > preempted, so we don't need accurate information about the target
> > tab/docgroup for these events.  We just treat them with the highest priority.
> 
> Yes, the input event will be in highest priority, but while processing an
> input event, further runnables/tasks related to a window could be triggered
> and are supposed to be labeled.

Yes.  In those cases, those need to be labeled explicitly.

> The idea is that if we can label the runnable as early as possible in the
> dispatching sequence, the less we have to modify the use cases that invoke
> NS_DispatchToCurrentThread, NS_DispatchToMainThread and their friend APIs.
> Hence, labeling these input events if we can looks important to me for
> implicit labeling.

If there's an easy way to do that, then I'm all for it.  Basically I don't feel very strongly about any of this.  My only desire is to a) make it so that people can get away with less explicit labeling, and b) facilitate doing the right thing in terms of labeling by providing APIs that allow people to dispatch runnables to things like documents where we can infer some of the labeling information.

Comment 14

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

I looked through ActorsChild.cpp today. I don't know this code at all, but all the actors that I saw seemed to have a pretty direct route to the window they're attached to. It looks like BackgroundDatabaseChild's manager is BackgroundFactoryChild, which then has mFactory. BackgroundDatabaseRequestChild can go through its Manager() to get to the database. Similarly for transactions. The permission stuff seems to have an mFactory pointer.

I would like to avoid labeling stuff that's in the parent process or off the main thread. If there are generic dispatch sites we'll probably end up doing it. But I don't see any reason to otherwise.

Labeling the message handlers that come in from IPDL will be tricky. Ideally I would like to rely on data in the content process to handle that rather than having to label stuff in the parent. For protocols like indexed DB and necko, I think we could label actors. When constructing a new actor, we'll assign a DocGroup to it. Any messages received for that actor will be posted to the given doc group.

That won't work for PBrowser. Most of the time I think PBrowser messages don't actually touch DOM, JS, or layout, so it doesn't matter. In the cases where they do, we might be stuck labeling with a TabGroup. Since frame scripts already prevent us from preempting at the DocGroup granularity, that might not be so bad for now.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to :Ehsan Akhgari from comment #13)
> Sure.  But still if the runnable that you're processing on thread A has
> label X, and it dispatches a new runnable to thread B, we can label that one
> as X too, right?
Yes, that's the main concept of this idea.

Comment 16

[Andrew Overholt [:overholt]]

Just clarifying expectations around timeline here.

Comment 17

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

Attachment: DocGroup labeling API

I just realized I forgot to change DOM to Other. I'll fix that soon.

I found it useful to separate out a DispatcherTrait object that nsIGlobalObject and nsIDocument inherit from. This object doesn't inherit from nsISupports, so you can't use it directly in a RefPtr. However, it provides a nice default implementation of Dispatch, which is useful for the nsIGlobalObject cases that we don't care about (Sandboxes and such).

Dispatcher inherits from nsISupports. It's nice to be able to keep a RefPtr to an object that is either a TabGroup or a DocGroup, and this object serves that purpose.

Comment 18

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

Attachment: Labeled event targets

This patch makes it possible to create a specialized nsIEventTarget that calls Dispatch on the right TabGroup/DocGroup with the right TaskCategory and name. It's very useful for converting code that expects an event target.

I considered keeping a map of these so that there is only one per DocGroup/category/name. But it doesn't seem worth it right now.

Comment 19

[(no longer active)]

Comment on attachment 8811074 [details] [diff] [review]
DocGroup labeling API

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

Thanks, this looks great!  I believe all of my comments below are relatively minor, please feel free to flag me again if the patch ends up changing drastically anyway.

::: dom/base/Dispatcher.cpp
@@ +13,5 @@
> +DispatcherTrait::Dispatch(const char* aName,
> +                          TaskCategory aCategory,
> +                          already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> +  return NS_DispatchToMainThread(Move(aRunnable));

Please also assert that this is called on the main thread.

::: dom/base/Dispatcher.h
@@ +8,5 @@
> +#define mozilla_dom_Dispatcher_h
> +
> +#include "mozilla/AlreadyAddRefed.h"
> +#include "nsISupports.h"
> +#include "nsISupportsImpl.h"

This include can go away given the below.

@@ +23,5 @@
> +  // Data from the network
> +  Network,
> +
> +  // setTimeout, setInterval
> +  DOMTimer,

Once you rename DOM to Other, perhaps you can also rename this to Timer?

@@ +49,5 @@
> +
> +// Base class for DocGroup and TabGroup.
> +class Dispatcher : public nsISupports {
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

We typically leave implementing nsISupports to the derived-most class in the hierarchy.  In this case since both TabGroup and DocGroup have only one nsISupports base currently, these are equivalent but as soon as one of them obtains a second nsISupports base, it needs to also implement that interface and then the object will have two mRefCnt's which is a no-no.  So please remove this and leave the implementation of nsISupports to DocGroup and TabGroup themselves.

BTW, I noticed that this interface doesn't support being QIed to.  That's fine, just wanted to double check that was intentional.

@@ +56,5 @@
> +                            TaskCategory aCategory,
> +                            already_AddRefed<nsIRunnable>&& aRunnable) = 0;
> +
> +protected:
> +  virtual ~Dispatcher() {}

I don't think this is necessary.  It just bloats the vtable needlessly.

::: dom/base/DocGroup.cpp
@@ +53,5 @@
> +DocGroup::Dispatch(const char* aName,
> +                   TaskCategory aCategory,
> +                   already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> +  return NS_DispatchToMainThread(Move(aRunnable));

Please also assert that this is called on the main thread.

::: dom/base/DocGroup.h
@@ +12,5 @@
>  #include "nsIPrincipal.h"
>  #include "nsTHashtable.h"
>  #include "nsString.h"
>  
> +#include "mozilla/AlreadyAddRefed.h"

You should include Dispatcher.h instead.  I think now you're getting that by mercy of unified builds.  :-)

::: dom/base/TabGroup.cpp
@@ +168,5 @@
> +TabGroup::Dispatch(const char* aName,
> +                   TaskCategory aCategory,
> +                   already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> +  return NS_DispatchToMainThread(Move(aRunnable));

Please also assert main-thread-ness here.

::: dom/base/TabGroup.h
@@ +35,5 @@
>  // window.opener. A DocGroup is a member of exactly one TabGroup.
>  
>  class DocGroup;
>  
> +class TabGroup final : public Dispatcher

Missing #include here too.

::: dom/base/nsDocument.cpp
@@ +2891,5 @@
> +nsIDocument::Dispatch(const char* aName,
> +                      TaskCategory aCategory,
> +                      already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> +  if (mDocGroup) {

Please assert main-thread-ness here too.

Comment 20

[(no longer active)]

Comment on attachment 8811075 [details] [diff] [review]
Labeled event targets

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

::: dom/base/Dispatcher.cpp
@@ +19,5 @@
>  
> +already_AddRefed<nsIEventTarget>
> +DispatcherTrait::CreateEventTarget(const char* aName,
> +                                   mozilla::dom::TaskCategory aCategory)
> +{

Please assert main-thread-ness here.

@@ +22,5 @@
> +                                   mozilla::dom::TaskCategory aCategory)
> +{
> +  nsCOMPtr<nsIThread> main;
> +  NS_GetMainThread(getter_AddRefs(main));
> +  nsCOMPtr<nsIEventTarget> target = main.get();

This can be rewritten more ergonomically like this, I believe:

nsCOMPtr<nsIEventTarget> main = do_GetMainThread();

@@ +30,5 @@
>  NS_IMPL_ISUPPORTS(Dispatcher, nsISupports)
> +
> +namespace {
> +
> +class DispatcherEventTarget : public nsIEventTarget

Nit: mark this final please.

@@ +62,5 @@
> +NS_IMETHODIMP
> +DispatcherEventTarget::DispatchFromScript(nsIRunnable* aRunnable, uint32_t aFlags)
> +{
> +  nsCOMPtr<nsIRunnable> event(aRunnable);
> +  return Dispatch(event.forget(), aFlags);

Could be simply:

  return Dispatch(do_AddRef(aRunnable), aFlags);

@@ +89,5 @@
> +}
> +
> +already_AddRefed<nsIEventTarget>
> +Dispatcher::CreateEventTarget(const char* aName, TaskCategory aCategory)
> +{

Please assert main-thread-ness here.

::: dom/base/Dispatcher.h
@@ +60,5 @@
>                              TaskCategory aCategory,
>                              already_AddRefed<nsIRunnable>&& aRunnable) = 0;
>  
> +  virtual already_AddRefed<nsIEventTarget>
> +  CreateEventTarget(const char* aName, mozilla::dom::TaskCategory aCategory);

The mozilla::dom:: prefix is unnecessary in both places here.

::: dom/base/nsDocument.cpp
@@ +2899,5 @@
>  }
>  
> +already_AddRefed<nsIEventTarget>
> +nsIDocument::CreateEventTarget(const char* aName, mozilla::dom::TaskCategory aCategory)
> +{

Please assert main-thread-ness here.

Comment 21

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

> Please also assert that this is called on the main thread.

Why would we want to do that? There will definitely be runnables that get dispatched from other threads into a DocGroup. The HTML parser does this, for example.

Comment 22

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

For comment 21.

Comment 23

[(no longer active)]

Yeah I guess that's true, but only for DocGroup/TabGroup, right?  Can you please add comments there explaining that the caller may be on any thread, and add the assertions elsewhere (such as nsIDocument::CreateEventTarget etc where we have a main-thread-only object)?  My main comment was about making it explicit which one of these functions should be expected to be main-thread-only and which ones can safely be called from everywhere.  Thanks!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b16c69a1b81
Dispatcher API for assigning DocGroups to runnables (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd436e553ed
Custom event targets for Dispatcher API (r=ehsan)

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8811075 [details] [diff] [review]
Labeled event targets

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

::: dom/base/Dispatcher.h
@@ +47,5 @@
>                              TaskCategory aCategory,
>                              already_AddRefed<nsIRunnable>&& aRunnable);
> +
> +  virtual already_AddRefed<nsIEventTarget>
> +  CreateEventTarget(const char* aName, mozilla::dom::TaskCategory aCategory);

Does the API contract for this method guarantee that a new event target is created every time?  If not, then I feel pretty strongly its mis-named.  Can we call it something like: this?

  GetEventTarget() or
  GetOrCreateEventTarget()

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Please see comment 25.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Also, I'm concerned about adding string comparisons on so many places we call Dispatch().  Yea, we shouldn't micro-optimize, but this stuff is going to get called *a lot*.  I feel like we should have an enumeration defined in a header somewhere instead.

Ehsan, does this not concern you?

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b16c69a1b81
https://hg.mozilla.org/mozilla-central/rev/dcd436e553ed

Comment 29

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

(In reply to Ben Kelly [:bkelly] from comment #25)
> Does the API contract for this method guarantee that a new event target is
> created every time?  If not, then I feel pretty strongly its mis-named.  Can
> we call it something like: this?
> 
>   GetEventTarget() or
>   GetOrCreateEventTarget()

Currently it does create a new event target each time. I suspect we'll stick with that decision, but I can't say for sure. The names you've suggested kind of imply that you can call the method many times with the same arguments and not worry about the allocation cost.

What is the downside of naming it CreateEventTarget when it might return an existing object?

Comment 30

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

Also, not sure what you mean about the string comparisons. I don't think there are any in this code. I guess there would be if we implemented CreateEventTarget as you suggested.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

So for every runnable dispatch we are going to create a new nsIEventTarget object?  I'm trying to understand under what design that makes sense.

If you just want a string associated with a runnable wouldn't it be better to create an nsINamedRunnable interface that classes could implement?

Of course, its likely I'm just very confused.

It would be really nice if you could add comments in the headers defining this API describing how it works and how its intended to be used.  I think I can categorically say its not intuitive at the moment.

Comment 32

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

(In reply to Ben Kelly [:bkelly] from comment #31)
> So for every runnable dispatch we are going to create a new nsIEventTarget
> object?  I'm trying to understand under what design that makes sense.

You don't have to create a new event target for every runnable. If you just want to dispatch one runnable, you can call Dispatch directly (either on a DocGroup, a TabGroup, an nsIDocument, or an nsIGlobalObject). If there is code that you expect to repeatedly call Dispatch using the same parameters, then you can create one of these nsIEventTargets to bundle up the parameters.

The expected use cases for the event target thing are things like IPC, where you want to tell an actor that all the messages destined for it should be associated with a particular DocGroup, name, and TaskCategory. It also works well for things that already expect an event target, like timers. (Admittedly the timer might fire only once, but some timers do fire often.)

> If you just want a string associated with a runnable wouldn't it be better
> to create an nsINamedRunnable interface that classes could implement?

It also associates a DocGroup or TabGroup and a category with the runnable.

It might be worthwhile to have an interface that provides these things for a runnable. Originally that's how I was considering implementing the whole thing. However, it doesn't seem to work very well in practice. A lot of runnables are pretty generic things (timers, lambdas, methods) and so they don't easily have a way to know what DocGroup they're associated with. We could augment them to carry around that information, but it would be a lot more work than what this patch does. And all to save one allocation per runnable, which isn't that big a deal.

> It would be really nice if you could add comments in the headers defining
> this API describing how it works and how its intended to be used.  I think I
> can categorically say its not intuitive at the moment.

I started trying to write documentation for this in the wiki:
  https://wiki.mozilla.org/Quantum/DOM
You're right that there should be more comments though.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bill McCloskey (:billm) from comment #32)
> I started trying to write documentation for this in the wiki:
>   https://wiki.mozilla.org/Quantum/DOM
> You're right that there should be more comments though.

I did read that.  I read it again after your comments and it makes a bit more sense.

I still feel, however, that its a design mistake to expose a Dispatch() on DocGroup with a different signature from nsIEventTarget.  IMO we should be operating on nsIEventTarget and associating this information as meta data on the runnable objects themselves.  For example, this would automatically solve your issues with "getting this through ThrottledEventQueue in the future" from the other bug.

Anyway, I digress.  I'll step back from this.

Comment 34

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

(In reply to Ben Kelly [:bkelly] from comment #33)
> I still feel, however, that its a design mistake to expose a Dispatch() on
> DocGroup with a different signature from nsIEventTarget.  IMO we should be
> operating on nsIEventTarget and associating this information as meta data on
> the runnable objects themselves.  For example, this would automatically
> solve your issues with "getting this through ThrottledEventQueue in the
> future" from the other bug.

I can imagine having an interface (nsILabelingDispatcher or something) with a LabeledDispatch method that takes a name and category. Then TabGroup and DocGroup would inherit from that, as would nsIEventDispatcher, which would still expose the old Dispatch method for compatibility.

That would help somewhat with the ThrottledEventQueue problem. But we would still need a place to store the name and category until we dispatch a runnable to the base event target. We could create a separate data structure for that, but you seemed to be proposing that we would get it from the runnable. As I said, though, I think many runnables could not easily provide this information. For example, the setTimeout runnable is a timer that has no idea it's associated with setTimeout. We could augment nsITimer to provide that information, but it would be a lot of work to do that for all the different places that generically create runnables.

Comment 35

[areya]

https://hg.mozilla.org/mozilla-central/rev/5b16c69a1b81
https://hg.mozilla.org/mozilla-central/rev/dcd436e553ed

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bill McCloskey (:billm) from comment #34)
> That would help somewhat with the ThrottledEventQueue problem. But we would
> still need a place to store the name and category until we dispatch a
> runnable to the base event target. We could create a separate data structure
> for that, but you seemed to be proposing that we would get it from the
> runnable. As I said, though, I think many runnables could not easily provide
> this information. For example, the setTimeout runnable is a timer that has
> no idea it's associated with setTimeout. We could augment nsITimer to
> provide that information, but it would be a lot of work to do that for all
> the different places that generically create runnables.

For things like nsITimer you could still do what you are doing today.  Create an nsIEventTarget that wraps runnables in a nsINamedRunnable or whatever.  That event target would then feed into the ThrottledEventQueue.  The ThrottledEventQueue would then feed to an nsIEventTarget that did any docshell specific stuff (like inspecting attributes on nsINamedRunnable wrapper, etc).

And I don't understand why roto-tilling everything to known specifically about the DocShell concrete class is less work then making them understand a Runnable meta-data interface.  It seems like a similar number of touch points, but makes the code a great deal less flexible by baking this DocShell concept in everywhere.  This just seems unnecessary complicating for code that just needs to know its own runnable name/category and an event target.

At the end of the day I think I just don't understand what these labels are for.  You say they aren't going to use them for comparisons, but then what are they for?

Comment 37

[(no longer active)]

For now, I think the names are only there for debugging purposes.  So are the labels, but we'll later probably use the labels for some scheduling purposes.

I'm not sure what the ThrottledEventQueue problem is, but it seems to me that what you are suggesting is the thing that CreateEventTarget() returns from Bill's patches, i.e., in cases where we need a place to store the name and label to associate them with a docgroup, we'll obtain an nsIEventTarget from CreateEventTarget(), and we'll dispatch to it.  However this is probably better for cases where we can create the event target and use it with a lot of runnables, not for one-off cases.

However in practice there's a lot of cases where you simply want to dispatch a runnable to the main thread for example, and if we did what you suggest (make this proxy event target the only way to label runnables) then the labeling task would need a lot of work, instead of just passing a couple of extra arguments to a function.  This, and the generiticy of the runnables as Bill mentioned were I think the two big reasons behind the current design.

Comment 38

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

Ben and I talked on Vidyo and he had some concrete suggestions that would resolve his concerns.

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.

Ehsan, how does that proposal sound to you? I like these ideas.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

I would also recommend:

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.

This is similar to my previous thought process of creating ThrottledEventQueue targets for the different spec task sources like timers, post message, etc.

Comment 40

[(no longer active)]

These ideas generally sounds good to me.  One question though.  After removing the name from the event target, how would one provide the name when dispatching to such an event target?

Comment 41

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

Probably something along the lines of:
  runnable = MakeRunnable(...);
  runnable->SetName("Hello");
  Dispatch(runnable.forget());

Comment 42

[(no longer active)]

OK, that sounds fine.  I guess it's not the end of the world if people forget to do this...

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

By the way, I would highly recommend not adding SetName to nsIRunnable.  Instead make an nsINamedRunnable or nsINamedObject mix-in interface.  Runnable can implement it, but it avoids having to change all other nsIRunnable implementations immediately.

Comment 44

[(no longer active)]

We probably want an nsNamedRunnable, I'd think, since we probably don't want to inflate all runnables with a nullptr name member.

We can also put off the naming part for now.  IIRC the name was only intended to be used for debugging purposes, and I don't have a great use case for it in my mind right now (but I may be forgetting it?)

Comment 45

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

(In reply to Ben Kelly [:bkelly] from comment #43)
> By the way, I would highly recommend not adding SetName to nsIRunnable. 
> Instead make an nsINamedRunnable or nsINamedObject mix-in interface. 
> Runnable can implement it, but it avoids having to change all other
> nsIRunnable implementations immediately.

That seems fine.

(In reply to :Ehsan Akhgari from comment #44)
> We probably want an nsNamedRunnable, I'd think, since we probably don't want
> to inflate all runnables with a nullptr name member.

That would make it a lot harder. We also have CancelableRunnable that a lot of stuff inherits from. I'd like to avoid using multiple inheritance.

What if we only keep the data around on Nightly/Aurora? If it proves to be useful, we can turn it on in release.

> We can also put off the naming part for now.  IIRC the name was only
> intended to be used for debugging purposes, and I don't have a great use
> case for it in my mind right now (but I may be forgetting it?)

I'd really like it for getting better stacks from telemetry. Right now most of our BHR hangs have an empty pseudostack. This would help a lot to fix that.

Comment 46

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #45)
> (In reply to :Ehsan Akhgari from comment #44)
> > We probably want an nsNamedRunnable, I'd think, since we probably don't want
> > to inflate all runnables with a nullptr name member.
> 
> That would make it a lot harder. We also have CancelableRunnable that a lot
> of stuff inherits from. I'd like to avoid using multiple inheritance.

Ugh, I forgot about that!

> What if we only keep the data around on Nightly/Aurora? If it proves to be
> useful, we can turn it on in release.

Sure, sounds good.

> > We can also put off the naming part for now.  IIRC the name was only
> > intended to be used for debugging purposes, and I don't have a great use
> > case for it in my mind right now (but I may be forgetting it?)
> 
> I'd really like it for getting better stacks from telemetry. Right now most
> of our BHR hangs have an empty pseudostack. This would help a lot to fix
> that.

That's a great use case for the labels!