Status

()

defect
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 10 obsolete attachments)

58 bytes, text/x-review-board-request
bkelly
: review+
Details
58 bytes, text/x-review-board-request
Ehsan
: review+
Details
58 bytes, text/x-review-board-request
jduell.mcbugs
: review+
mayhemer
: review+
Details
58 bytes, text/x-review-board-request
Ehsan
: review+
Nika
: review+
Details
58 bytes, text/x-review-board-request
jimm
: review+
Details
58 bytes, text/x-review-board-request
Ehsan
: review+
Details
58 bytes, text/x-review-board-request
bkelly
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
Ehsan
: review+
Details
7.02 KB, patch
dvander
: review+
Details | Diff | Splinter Review
I'll post patches here to assign DocGroups to some common runnables.
The goal here is roughly to have separate event queues per document. So, for each event, we need to decide which queue it should go into.
I wrote a wiki page on this stuff here:
https://wiki.mozilla.org/Quantum/DOM
It's a first draft, but maybe it will help to review this code.
Comment on attachment 8812011 [details]
Bug 1318506 - Label XHR timer runnables with DocGroup ()

https://reviewboard.mozilla.org/r/93898/#review94178

::: dom/xhr/XMLHttpRequestMainThread.cpp:3091
(Diff revision 1)
>  
>  void
> +XMLHttpRequestMainThread::SetTimerEventTarget(const char* aName, nsITimer* aTimer)
> +{
> +  if (nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal()) {
> +    nsCOMPtr<nsIEventTarget> target = global->CreateEventTarget("XHRTimer", TaskCategory::Other);

As I commented in bug 1305926, I think CreateEventTarget is poorly named.  This is likely to reuse an existing event target, correct?

::: dom/xhr/XMLHttpRequestMainThread.cpp:3091
(Diff revision 1)
>  
>  void
> +XMLHttpRequestMainThread::SetTimerEventTarget(const char* aName, nsITimer* aTimer)
> +{
> +  if (nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal()) {
> +    nsCOMPtr<nsIEventTarget> target = global->CreateEventTarget("XHRTimer", TaskCategory::Other);

You pass in `aName`, but don't actually use it.  You hard code everything to `"XHRTimer"` instead.
Attachment #8812011 - Flags: review?(bkelly) → review-
Comment on attachment 8812013 [details]
Bug 1318506 - Assign TabGroup to runnable for TabGroup's throttled event queue ()

https://reviewboard.mozilla.org/r/93902/#review94180

I guess this is ok, but I don't understand why you are keeping the TabGroup ThrottledEventQueue if you are moving to named event targets at a lower level.  What purpose will the TEQ serve?  For example, timers are going through the ThrottledEventQueue name, but from your TaskCategory stuff it seems you would rather go straight to a labeled event target.
Attachment #8812013 - Flags: review?(bkelly) → review+
Comment on attachment 8812015 [details]
Bug 1318506 - Label MessagePort runnables by DocGroup ()

https://reviewboard.mozilla.org/r/93906/#review94182
Attachment #8812015 - Flags: review?(bkelly) → review+
Comment on attachment 8812012 [details]
Bug 1318506 - Label PluginInstance IPC messages with plugin's DocGroup ()

https://reviewboard.mozilla.org/r/93900/#review94260
Attachment #8812012 - Flags: review?(aklotz) → review+
Attachment #8812008 - Attachment is obsolete: true
Attachment #8812008 - Flags: review?(ehsan)
Attachment #8812009 - Attachment is obsolete: true
Attachment #8812009 - Flags: review?(jduell.mcbugs)
Attachment #8812010 - Attachment is obsolete: true
Attachment #8812010 - Flags: review?(michael)
Attachment #8812010 - Flags: review?(ehsan)
Attachment #8812012 - Attachment is obsolete: true
Attachment #8812013 - Attachment is obsolete: true
Attachment #8812014 - Attachment is obsolete: true
Attachment #8812014 - Flags: review?(ehsan)
Attachment #8812015 - Attachment is obsolete: true
Attachment #8812016 - Attachment is obsolete: true
Attachment #8812016 - Flags: review?(ehsan)
Attachment #8812017 - Attachment is obsolete: true
Attachment #8812017 - Flags: review?(ehsan)
Comment on attachment 8812011 [details]
Bug 1318506 - Label XHR timer runnables with DocGroup ()

https://reviewboard.mozilla.org/r/93898/#review94290
Attachment #8812011 - Flags: review?(bkelly) → review+
Comment on attachment 8812010 [details]
Bug 1318506 - Assign a TabGroup to every PBrowser ()

https://reviewboard.mozilla.org/r/93896/#review94282

I'm not a big fan of recovering the TabGroup reference from a nsIEventTarget through a series of unchecked static_casts. I would much prefer it if we made the association between a TabChild and a TabGroup more explicit through an accessor to get the TabGroup off of a TabChild, and a mechanism for explicitly assigning TabGroups to TabChilds other than just setting an event target.

::: dom/base/TabGroup.cpp:91
(Diff revision 1)
> +  // We have an event target. We assume the IPC code created it via
> +  // TabGroup::CreateEventTarget.

I would prefer it if instead of doing this gymnastics to extract the TabGroup out of the nsIEventTarget we just kept a reference to the TabGroup alive on the TabChild, which we can grab directly.

It seems like it would be simpler, and potentially less error prone.

If we do decide to stick with this method of doing it I would like a more clear comment explaining that we are recovering the TabGroup from the nsIEventTarget under the assumption that the nsIEventTarget was created through a call to TabGroup::CreateEventTarget.

::: dom/ipc/ContentChild.cpp:721
(Diff revision 1)
> +    tabGroup = aTabOpener->TabGroup();
> +  } else {
> +    tabGroup = new TabGroup();
> +  }
> +  nsCOMPtr<nsIEventTarget> target = tabGroup->CreateEventTarget("TabChild", TaskCategory::Other);
> +  SetEventTargetForActor(newChild, target);

Is there any way we could also just pass the TabGroup explicitly to the actor to hold onto so we don't have to recover it from the nsIEventTarget later?

I imagine we could do something similar to how we handle windows, where we have a getter for TabGroup() which would generate a TabGroup if it is called before one is explicitly set, but we allow it to be explicitly set before it is called and RELEASE_ASSERT that one or the other path is taken. It feels like it might be more clear than this.
Attachment #8812010 - Flags: review-
Sorry about the mess here. I've never used mozreview before and now I hate it more than ever. I'll try to fix this after lunch.
Comment on attachment 8812010 [details]
Bug 1318506 - Assign a TabGroup to every PBrowser ()

https://reviewboard.mozilla.org/r/93896/#review94296

::: dom/base/TabGroup.cpp:91
(Diff revision 1)
> +  // We have an event target. We assume the IPC code created it via
> +  // TabGroup::CreateEventTarget.

The basic problem is that the parent process can send the child an IPC message asking it to create a new TabChild. It can also send a bunch of messages to that new actor before it's ever even created on the child side. We want all of these messages to be dispatched to a new TabGroup for the new TabChild. But, at the time that we need to dispatch runnables for these messages, the TabChild doesn't even exist yet.

So my solution was to create a TabGroup for IPC. Then when we do create the TabChild, pull its TabGroup out of IPC. And for that I need a cast.

Perhaps there's some other place where we could store the TabGroup. Maybe a map on the ContentChild that associates actor IDs with TabGroups.
Comment on attachment 8812010 [details]
Bug 1318506 - Assign a TabGroup to every PBrowser ()

https://reviewboard.mozilla.org/r/93896/#review94296

> The basic problem is that the parent process can send the child an IPC message asking it to create a new TabChild. It can also send a bunch of messages to that new actor before it's ever even created on the child side. We want all of these messages to be dispatched to a new TabGroup for the new TabChild. But, at the time that we need to dispatch runnables for these messages, the TabChild doesn't even exist yet.
> 
> So my solution was to create a TabGroup for IPC. Then when we do create the TabChild, pull its TabGroup out of IPC. And for that I need a cast.
> 
> Perhaps there's some other place where we could store the TabGroup. Maybe a map on the ContentChild that associates actor IDs with TabGroups.

That makes some sense. It might even be nice just to have some sort of dynamically checked mechanism for recovering this property, even if it is just a checked do_QueryInterface, just because I don't like that if someone else creates an EventTarget, innocently thinking it will be fine, we just cast it around assuming it is one which we created and cause a sec bug.
(In reply to Bill McCloskey (:billm) from comment #20)
> Sorry about the mess here. I've never used mozreview before and now I hate
> it more than ever. I'll try to fix this after lunch.

Not sure what happened here.  I opened this bug because I had some reviews on it pending but now they've all disappeared, and I see that bkelly and mystor have been reviewing some of the patches.  I'm lost as to what to do with this bug.  :-)
Whoa and now the reviews are back!!!  I'll review next week, sorry I didn't get to it today...
Comment on attachment 8812348 [details]
Bug 1318506 - Assign a TabGroup to every PBrowser ()

https://reviewboard.mozilla.org/r/94092/#review94344

I like this much more. I think it's really important to have assertions to double-check our assumptions here.

r=me with the extra assertion. With regard to exactly how we do the checked cast to a TabGroup, I'm fairly indifferent.

::: dom/base/Dispatcher.h:82
(Diff revision 1)
> +  virtual TabGroup* AsTabGroup() = 0;
> +  virtual DocGroup* AsDocGroup() = 0;

Why not implement these as 
```
virtual TabGroup* AsTabGroup() {
  return nullptr;
}
virtual DocGroup* AsDocGroup() {
  return nullptr;
}
```
and then only override those methods on the relevant types. It would take less code, and reduce some duplication, no?

It's pretty gross - but it might also work to define TabGroup and DocGroup as pseudo-interfaces (give them an IID), allowing you to do_QueryInterface directly to them, removing the need for these specific methods. I've seen code which does that around the codebase, but I don't know how taboo it is.

If you don't see the need for additional subclasses of Dispatcher however, then sticking with the methods avoids that grossness.

::: dom/base/nsGlobalWindow.cpp:14866
(Diff revision 1)
> +    } else {
> +      // If the tab was created by the parent process, the IPC code may have
> +      // already created a TabGroup for us. Fetch it in that case.
> +      toJoin = TabGroup::GetFromWindowActor(AsOuter());
>      }
>      mTabGroup = mozilla::dom::TabGroup::Join(AsOuter(), toJoin);

I think it would be good to assert that our tabGroup matches the one which is attached to our window actor at this point. Something like
```
#ifdef DEBUG
mozilla::dom::TabGroup* actorTabGroup = TabGroup::GetFromWindowActor(AsOuter());
MOZ_ASSERT_IF(actorTabGroup, actorTabGroup == toJoin);
#endif
```
Because, if the toJoin was set by any of the other branches, we have no checks that the tab group on the actor is correct. I don't think we need to do this in release (it would be obviously slow), but it would be good to check.
Attachment #8812348 - Flags: review?(michael) → review+
Comment on attachment 8812346 [details]
Bug 1318506 - Label some common runnables with a DocGroup ()

https://reviewboard.mozilla.org/r/94088/#review94578

::: dom/base/nsScriptLoader.cpp:1731
(Diff revision 1)
>    NotifyOffThreadScriptLoadCompletedRunnable(nsScriptLoadRequest* aRequest,
>                                               nsScriptLoader* aLoader)
>      : mRequest(aRequest), mLoader(aLoader), mToken(nullptr)
> -  {}
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    nsCOMPtr<nsINode> scriptElem = do_QueryInterface(aRequest->mElement);

mElement can be null for preloaded scripts created from here: <http://searchfox.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#2778>.  I'm not sure if such scripts can ever get to here, but I would feel a bit better if you would null check this, or somehow add an assertion making sure that off-main-thread compiled scripts can never come from preloading.

::: dom/base/nsScriptLoader.cpp:1745
(Diff revision 1)
>    }
>  
> +  static void Dispatch(already_AddRefed<NotifyOffThreadScriptLoadCompletedRunnable>&& aSelf) {
> +    RefPtr<NotifyOffThreadScriptLoadCompletedRunnable> self = aSelf;
> +    RefPtr<DocGroup> docGroup = self->mDocGroup;
> +    docGroup->Dispatch("OffThreadScriptLoader", TaskCategory::Other, self.forget());

The above opens the question of what to do here if the docgroup is null.  Perhaps we can capture the docgroup in nsScriptLoadRequest directly?

::: dom/html/HTMLMediaElement.cpp:5153
(Diff revision 1)
>      mPendingEvents.AppendElement(aName);
>      return NS_OK;
>    }
>  
>    nsCOMPtr<nsIRunnable> event = new nsAsyncEventRunner(aName, this);
> -  NS_DispatchToMainThread(event);
> +  OwnerDoc()->Dispatch("HTMLMediaElement::DispatchEvent",

Nit: DispatchAsyncEvent.
Attachment #8812346 - Flags: review?(ehsan) → review-
Comment on attachment 8812348 [details]
Bug 1318506 - Assign a TabGroup to every PBrowser ()

https://reviewboard.mozilla.org/r/94092/#review94588

::: dom/base/Dispatcher.h:82
(Diff revision 1)
>    virtual already_AddRefed<nsIEventTarget>
>    CreateEventTarget(const char* aName, TaskCategory aCategory);
> +
> +  // These methods perform a safe cast. They return null if |this| is not of the
> +  // requested type.
> +  virtual TabGroup* AsTabGroup() = 0;

With AsDocGroup() removed, I also think you can give this a default body and only override it in TabGroup.

::: dom/base/Dispatcher.h:83
(Diff revision 1)
>    CreateEventTarget(const char* aName, TaskCategory aCategory);
> +
> +  // These methods perform a safe cast. They return null if |this| is not of the
> +  // requested type.
> +  virtual TabGroup* AsTabGroup() = 0;
> +  virtual DocGroup* AsDocGroup() = 0;

Nobody is calling AsDocGroup(), so I think you can remove it.

::: dom/ipc/ContentChild.cpp:715
(Diff revision 1)
> +  // We need to assign a TabGroup to the PBrowser actor before we send it to the
> +  // parent. Otherwise, the parent could send messages to us before we have a
> +  // proper TabGroup for that actor.
> +  RefPtr<TabGroup> tabGroup;
> +  if (aTabOpener && !aForceNoOpener) {
> +    // The new actor will use the same event target as the opener.

s/event target/tab group/

::: dom/ipc/ContentChild.cpp:721
(Diff revision 1)
> +    tabGroup = aTabOpener->TabGroup();
> +  } else {
> +    tabGroup = new TabGroup();
> +  }
> +  nsCOMPtr<nsIEventTarget> target = tabGroup->CreateEventTarget("TabChild", TaskCategory::Other);
> +  SetEventTargetForActor(newChild, target);

Hmm, I can't find where SetEventTargetForActor() is defined...
Attachment #8812348 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #37)
> Comment on attachment 8812348 [details]
> Bug 1318506 - Assign a TabGroup to every PBrowser ()

Sorry, MozReview was way too eager to submit my review before I was finished.  :-)

I meant to say that I'm r+ing this without seeing where the event target/actor mapping has been implemented, but at least the names of the functions make me believe that part is pretty straightforward.  Still I'd like to see that implementation too, if possible.
Comment on attachment 8812351 [details]
Bug 1318506 - Label DispatchContentLoaded events with a DocGroup ()

https://reviewboard.mozilla.org/r/94098/#review94594
Attachment #8812351 - Flags: review?(ehsan) → review+
Comment on attachment 8812353 [details]
Bug 1318506 - Label AsyncEventDispatcher runnables with DocGroup ()

https://reviewboard.mozilla.org/r/94102/#review94598

::: dom/events/AsyncEventDispatcher.cpp:27
(Diff revision 1)
> +  if (nsCOMPtr<nsIGlobalObject> global = dispatcher->mTarget->GetOwnerGlobal()) {
> +    return global;
> +  }
> +
> +  // Sometimes GetOwnerGlobal returns null because it uses
> +  // GetScriptHandlingObject rather than GetScopeObject.

Hmm, the reason why GetScriptHandlingObject returns null is that we don't want to dispatch events to dynamically created documents, such as the one in the test case for bug 403167.

That being said, I would kind of assume that some code must be checking whether GetScriptHandlingObject() returns null and prevent event dispatching, but I can't find such code.  I'm a bit worried that I can't find it, and I think maybe bz would be a better reviewer for this patch.  I would suggest smaug but he's on vacation.
Attachment #8812353 - Flags: review?(ehsan)
Comment on attachment 8812354 [details]
Bug 1318506 - Assign DocGroup for ScriptLoader runnable ()

https://reviewboard.mozilla.org/r/94104/#review94604
Attachment #8812354 - Flags: review?(ehsan) → review+
Comment on attachment 8812350 [details]
Bug 1318506 - Assign TabGroup to runnable for TabGroup's throttled event queue ()

https://reviewboard.mozilla.org/r/94096/#review94620
Attachment #8812350 - Flags: review?(bkelly) → review+
Comment on attachment 8812352 [details]
Bug 1318506 - Label MessagePort runnables by DocGroup ()

https://reviewboard.mozilla.org/r/94100/#review94622

::: dom/messagechannel/MessagePort.cpp:568
(Diff revision 1)
>    mMessages.RemoveElementAt(0);
>  
>    mPostMessageRunnable = new PostMessageRunnable(this, data);
>  
> +  if (NS_IsMainThread()) {
> +    if (nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal()) {

Unnecessary nesting here.  You can simplify to:

```
if (NS_IsMainThread() && nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal()) {
  // dispatch
}
```
Attachment #8812352 - Flags: review?(bkelly) → review+
Comment on attachment 8812349 [details]
Bug 1318506 - Label PluginInstance IPC messages with plugin's DocGroup ()

https://reviewboard.mozilla.org/r/94094/#review95978

::: dom/plugins/ipc/PluginModuleParent.cpp:2759
(Diff revision 1)
>      RefPtr<PluginAsyncSurrogate> surrogate(
>          dont_AddRef(PluginAsyncSurrogate::Cast(instance)));
>      // Now replace it with the instance
>      instance->pdata = static_cast<PluginDataResolver*>(parentInstance);
>  
> +    RefPtr<nsPluginInstanceOwner> owner = parentInstance->GetOwner();

Please add a comment here explaining what this code block is doing.
Attachment #8812349 - Flags: review?(jmathies) → review+
Comment on attachment 8812353 [details]
Bug 1318506 - Label AsyncEventDispatcher runnables with DocGroup ()

https://reviewboard.mozilla.org/r/94102/#review96178

::: dom/events/AsyncEventDispatcher.cpp:27
(Diff revision 1)
> +  if (nsCOMPtr<nsIGlobalObject> global = dispatcher->mTarget->GetOwnerGlobal()) {
> +    return global;
> +  }
> +
> +  // Sometimes GetOwnerGlobal returns null because it uses
> +  // GetScriptHandlingObject rather than GetScopeObject.

OK, I'll redirect to smaug. However, this patch should not change whether the event gets dispatched or not. It's just a question of putting it in the right queue. So I think that resolving the issue you've raised should be separate from this bug.
Comment on attachment 8812352 [details]
Bug 1318506 - Label MessagePort runnables by DocGroup ()

https://reviewboard.mozilla.org/r/94100/#review94622

> Unnecessary nesting here.  You can simplify to:
> 
> ```
> if (NS_IsMainThread() && nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal()) {
>   // dispatch
> }
> ```

That doesn't mean the same thing. I can change the code around some other way, of course. Your call.
Comment on attachment 8812346 [details]
Bug 1318506 - Label some common runnables with a DocGroup ()

https://reviewboard.mozilla.org/r/94088/#review94578

> mElement can be null for preloaded scripts created from here: <http://searchfox.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#2778>.  I'm not sure if such scripts can ever get to here, but I would feel a bit better if you would null check this, or somehow add an assertion making sure that off-main-thread compiled scripts can never come from preloading.

I converted the code to use the document from the nsScriptLoader. I think that should work better.
Comment on attachment 8812348 [details]
Bug 1318506 - Assign a TabGroup to every PBrowser ()

https://reviewboard.mozilla.org/r/94092/#review94588

> Hmm, I can't find where SetEventTargetForActor() is defined...

The code for this has now landed (bug 1317844).
Attachment #8812350 - Attachment is obsolete: true
(In reply to Bill McCloskey (:billm) from comment #46)
> > Unnecessary nesting here.  You can simplify to:
> > 
> > ```
> > if (NS_IsMainThread() && nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal()) {
> >   // dispatch
> > }
> > ```
> 
> That doesn't mean the same thing. I can change the code around some other
> way, of course. Your call.

Not sure I understand.  This:

  if (a) {
    if (b) {
      // do stuff
    }
  }

Is logically equivalent to:

  if (a && b) {
    // do stuff
  }

Is it related to declaring the variable in the conditional somehow?
(In reply to Ben Kelly [:bkelly] from comment #58)
> Is it related to declaring the variable in the conditional somehow?

Yes. C++ allows you to write:
  if (<type-specifier-seq> <declarator> = <assignment-expression>) { ... }
In this case, the scope of the variable is restricted to the true branch of the conditional. It's kind of a nice way of restricting the scope.

Probably the most obvious way to change this would be:

nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal();
if (NS_IsMainThread() && global) { ... }

I don't really care either way. The scope ends right after this.
Comment on attachment 8812346 [details]
Bug 1318506 - Label some common runnables with a DocGroup ()

https://reviewboard.mozilla.org/r/94088/#review96198
Attachment #8812346 - Flags: review?(ehsan) → review+
Comment on attachment 8812353 [details]
Bug 1318506 - Label AsyncEventDispatcher runnables with DocGroup ()

(In reply to [PTO to Dec5] Bill McCloskey (:billm) from comment #45)
> Comment on attachment 8812353 [details]
> Bug 1318506 - Label AsyncEventDispatcher runnables with DocGroup ()
> 
> https://reviewboard.mozilla.org/r/94102/#review96178
> 
> ::: dom/events/AsyncEventDispatcher.cpp:27
> (Diff revision 1)
> > +  if (nsCOMPtr<nsIGlobalObject> global = dispatcher->mTarget->GetOwnerGlobal()) {
> > +    return global;
> > +  }
> > +
> > +  // Sometimes GetOwnerGlobal returns null because it uses
> > +  // GetScriptHandlingObject rather than GetScopeObject.
> 
> OK, I'll redirect to smaug. However, this patch should not change whether
> the event gets dispatched or not. It's just a question of putting it in the
> right queue. So I think that resolving the issue you've raised should be
> separate from this bug.

Fair.  I'm happy with the patch if smaug is.

Clearing the Bugzilla flag -- it's out of sync with MozReview.)
Attachment #8812353 - Flags: review?(ehsan)
(In reply to [PTO to Dec5] Bill McCloskey (:billm) from comment #59)
> nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal();
> if (NS_IsMainThread() && global) { ... }

That would be fine with me too.  I think its a lot easier to read.

You could also just use GetOwnerGlobal() in the conditional and again in the body.  You pay the slight cost of a second QI inside GetOwnerGlobal(), but you also avoid a needless AddRef().

Anyway, it was just a style nit.
Comment on attachment 8812353 [details]
Bug 1318506 - Label AsyncEventDispatcher runnables with DocGroup ()

https://reviewboard.mozilla.org/r/94102/#review96598

::: dom/events/AsyncEventDispatcher.cpp:19
(Diff revision 2)
>  
>  namespace mozilla {
>  
>  using namespace dom;
>  
> +static DispatcherTrait*

(I wonder what is DispatcherTrait. Sounds odd. Should be nsIEventTarget or something similar. Why *Trait)

::: dom/events/AsyncEventDispatcher.cpp:20
(Diff revision 2)
>  namespace mozilla {
>  
>  using namespace dom;
>  
> +static DispatcherTrait*
> +GetDispatcher(AsyncEventDispatcher* dispatcher)

Arguments should be named aName, so
aDispatcher

::: dom/events/AsyncEventDispatcher.cpp:28
(Diff revision 2)
> +    return global;
> +  }
> +
> +  // Sometimes GetOwnerGlobal returns null because it uses
> +  // GetScriptHandlingObject rather than GetScopeObject.
> +  if (nsCOMPtr<nsINode> node = do_QueryInterface(dispatcher->mTarget)) {

This is rather unfortunate special case. But at least return the scopeobject of OwnerDoc() in this case and not OwnerDoc() itself, so that the returned value is always a global.
Attachment #8812353 - Flags: review?(bugs) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee28526a9033
Label some common runnables with a DocGroup (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0c2912650a
Label XHR timer runnables with DocGroup (r=bkelly)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3411c3d9c141
Label PluginInstance IPC messages with plugin's DocGroup (r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b3690476c9
Label DispatchContentLoaded events with a DocGroup (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e642f1df626d
Label MessagePort runnables by DocGroup (r=bkelly)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb19a439bfbd
Assign DocGroup for ScriptLoader runnable (r=ehsan)
Backed out for frequent failure in clearkey-mp4-setmediakeys-multiple-times-with-the-same-mediakeys.html (most likely on OS X 10.10 debug, but also hits Windows):

https://hg.mozilla.org/integration/mozilla-inbound/rev/89d204e96e8d7715da4c309f8861c5a51184c690
https://hg.mozilla.org/integration/mozilla-inbound/rev/e36f92b56d8146438954a910a053d8df5715686b
https://hg.mozilla.org/integration/mozilla-inbound/rev/237125712ba359a94f5f7f91e9884be64269e9a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9efd9724f08fd0dfe83f1deafe5375b12f6111ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca73f213f05a4ae120619fbee6fe6cd11d577638
https://hg.mozilla.org/integration/mozilla-inbound/rev/d48bc2da8280c50096c8dbc587ed1e2113c4f908

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bb19a439bfbda749e043add759db08e54d939c2e
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40148659&repo=mozilla-inbound

19:30:12     INFO - TEST-START | /encrypted-media/clearkey-mp4-setmediakeys-multiple-times-with-the-same-mediakeys.html
19:30:12     INFO - PROCESS | 1650 | [1650] WARNING: NS_ENSURE_TRUE(layerManager) failed: file /builds/slave/m-in-m64-d-0000000000000000000/build/src/dom/media/MediaDecoder.cpp, line 303
19:30:13     INFO - PROCESS | 1650 | 
19:30:13     INFO - PROCESS | 1650 | ###!!! [Child][MessageChannel] Error: (msgtype=0x6E0007,name=PGMP::Msg_PGMPContentChildDestroyed) Channel error: cannot send/recv
19:30:13     INFO - PROCESS | 1650 | 
19:30:13     INFO - TEST-UNEXPECTED-ERROR | /encrypted-media/clearkey-mp4-setmediakeys-multiple-times-with-the-same-mediakeys.html | 'NoneType' object has no attribute 'remove'

Another failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40167564&repo=mozilla-inbound

02:01:10     INFO - TEST-START | /encrypted-media/clearkey-mp4-setmediakeys-multiple-times-with-the-same-mediakeys.html
02:01:10     INFO - PROCESS | 1643 | [1643] WARNING: NS_ENSURE_TRUE(layerManager) failed: file /builds/slave/m-in-m64-d-0000000000000000000/build/src/dom/media/MediaDecoder.cpp, line 303
02:01:11     INFO - PROCESS | 1643 | 
02:01:11     INFO - PROCESS | 1643 | ###!!! [Child][MessageChannel] Error: (msgtype=0x6E0007,name=PGMP::Msg_PGMPContentChildDestroyed) Channel error: cannot send/recv
02:01:11     INFO - PROCESS | 1643 | 
02:01:11     INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/DJoiToiNRJ-aizZUxdmAiQ/artifacts/public/build/firefox-53.0a1.en-US.mac64.crashreporter-symbols.zip
02:01:18     INFO - mozcrash Copy/paste: /builds/slave/test/build/macosx64-minidump_stackwalk /var/folders/5n/sn2p23ks3csg46__d68h6j6m00000w/T/tmphx3UnA.mozrunner/minidumps/55206789-3C74-4CA1-BE57-317C0D7A8F44.dmp /var/folders/5n/sn2p23ks3csg46__d68h6j6m00000w/T/tmpDU1A3A
02:01:32     INFO - mozcrash Saved minidump as /builds/slave/test/build/blobber_upload_dir/55206789-3C74-4CA1-BE57-317C0D7A8F44.dmp
02:01:32     INFO - mozcrash Saved app info as /builds/slave/test/build/blobber_upload_dir/55206789-3C74-4CA1-BE57-317C0D7A8F44.extra
02:01:33     INFO - PROCESS-CRASH | /encrypted-media/clearkey-mp4-setmediakeys-multiple-times-with-the-same-mediakeys.html | application crashed [@ mozilla::dom::SourceBuffer::QueueAsyncSimpleEvent(char const*)]
02:01:33     INFO - Crash dump filename: /var/folders/5n/sn2p23ks3csg46__d68h6j6m00000w/T/tmphx3UnA.mozrunner/minidumps/55206789-3C74-4CA1-BE57-317C0D7A8F44.dmp
02:01:33     INFO - Operating system: Mac OS X
02:01:33     INFO -                   10.10.5 14F27
02:01:33     INFO - CPU: amd64
02:01:33     INFO -      family 6 model 69 stepping 1
02:01:33     INFO -      4 CPUs
02:01:33     INFO - 
02:01:33     INFO - GPU: UNKNOWN
02:01:33     INFO - 
02:01:33     INFO - Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
02:01:33     INFO - Crash address: 0x8
02:01:33     INFO - Process uptime: 373 seconds
02:01:33     INFO - 
02:01:33     INFO - Thread 0 (crashed)
02:01:33     INFO -  0  XUL!mozilla::dom::SourceBuffer::QueueAsyncSimpleEvent(char const*) [SourceBuffer.cpp:0dff674b4497 : 360 + 0x0]
02:01:33     INFO -     rax = 0x0000000000000000   rdx = 0x00007fff5bc554a8
02:01:33     INFO -     rcx = 0x0000000000000000   rbx = 0x0000000000000000
02:01:33     INFO -     rsi = 0x0000000109ffe8f0   rdi = 0x00007fff5bc554a0
02:01:33     INFO -     rbp = 0x00007fff5bc554d0   rsp = 0x00007fff5bc55470
02:01:33     INFO -      r8 = 0x0000000010008293    r9 = 0x0000000040fda04b
02:01:33     INFO -     r10 = 0x0000000000000001   r11 = 0x000000005ffff800
02:01:33     INFO -     r12 = 0xa6005900081a2cc4   r13 = 0x0000000000000000
02:01:33     INFO -     r14 = 0x0000000109a925df   r15 = 0x00000001259d7a00
02:01:33     INFO -     rip = 0x0000000106d958d6
02:01:33     INFO -     Found by: given as instruction pointer in context
02:01:33     INFO -  1  XUL!mozilla::dom::SourceBuffer::StopUpdating() [SourceBuffer.cpp:0dff674b4497 : 384 + 0xf]
02:01:33     INFO -     rbx = 0x00000001259d7a00   rbp = 0x00007fff5bc554f0
02:01:33     INFO -     rsp = 0x00007fff5bc554e0   r12 = 0xa6005900081a2cc4
02:01:33     INFO -     r13 = 0x0000000000000000   r14 = 0x00007fff5bc555c0
02:01:33     INFO -     r15 = 0x0000000123589e80   rip = 0x0000000106d95a31
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  2  XUL!mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true>::MethodThenValue<mozilla::dom::SourceBuffer, void (mozilla::dom::SourceBuffer::*)(mozilla::Pair<bool, mozilla::SourceBufferAttributes>), void (mozilla::dom::SourceBuffer::*)(mozilla::MediaResult const&)>::DoResolveOrRejectInternal(mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true>::ResolveOrRejectValue const&) [MozPromise.h:0dff674b4497 : 458 + 0x2]
02:01:33     INFO -     rbx = 0x0000000123589e80   rbp = 0x00007fff5bc555a0
02:01:33     INFO -     rsp = 0x00007fff5bc55500   r12 = 0xa6005900081a2cc4
02:01:33     INFO -     r13 = 0x0000000000000000   r14 = 0x00007fff5bc555c0
02:01:33     INFO -     r15 = 0x0000000123589e80   rip = 0x0000000106db5ad1
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  3  XUL!mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true>::ThenValueBase::DoResolveOrReject(mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true>::ResolveOrRejectValue const&) [MozPromise.h:0dff674b4497 : 408 + 0x10]
02:01:33     INFO -     rbx = 0x0000000123589e80   rbp = 0x00007fff5bc555e0
02:01:33     INFO -     rsp = 0x00007fff5bc555b0   r12 = 0x00000001042bc1f0
02:01:33     INFO -     r13 = 0x0000000000000000   r14 = 0x0000000123402e88
02:01:33     INFO -     r15 = 0x00000001042bc1a0   rip = 0x0000000106db717f
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  4  XUL!mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true>::ThenValueBase::ResolveOrRejectRunnable::Run() [MozPromise.h:0dff674b4497 : 324 + 0x5]
02:01:33     INFO -     rbx = 0x0000000123b40470   rbp = 0x00007fff5bc55600
02:01:33     INFO -     rsp = 0x00007fff5bc555f0   r12 = 0x00000001042bc1f0
02:01:33     INFO -     r13 = 0x0000000000000000   r14 = 0x0000000123b40468
02:01:33     INFO -     r15 = 0x00000001042bc1a0   rip = 0x0000000106db6f3c
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  5  XUL!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp:0dff674b4497 : 1213 + 0x6]
02:01:33     INFO -     rbx = 0x0000000000000000   rbp = 0x00007fff5bc556b0
02:01:33     INFO -     rsp = 0x00007fff5bc55610   r12 = 0x00000001042bc1f0
02:01:33     INFO -     r13 = 0x0000000000000000   r14 = 0x00000001042bc1c0
02:01:33     INFO -     r15 = 0x00000001042bc1a0   rip = 0x00000001046c2b03
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  6  XUL!NS_ProcessPendingEvents(nsIThread*, unsigned int) [nsThreadUtils.cpp:0dff674b4497 : 323 + 0xf]
02:01:33     INFO -     rbx = 0x0000000000000000   rbp = 0x00007fff5bc556f0
02:01:33     INFO -     rsp = 0x00007fff5bc556c0   r12 = 0x00007fff5bc556c7
02:01:33     INFO -     r13 = 0x00000001042bc1a0   r14 = 0x000000000000000a
02:01:33     INFO -     r15 = 0x0000000000088bd1   rip = 0x000000010470667f
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  7  XUL!nsBaseAppShell::NativeEventCallback() [nsBaseAppShell.cpp:0dff674b4497 : 97 + 0xa]
02:01:33     INFO -     rbx = 0x0000000114104230   rbp = 0x00007fff5bc55720
02:01:33     INFO -     rsp = 0x00007fff5bc55700   r12 = 0x0000000000000000
02:01:33     INFO -     r13 = 0x0000000000002403   r14 = 0x00000001042bc1a0
02:01:33     INFO -     r15 = 0x0000000114104200   rip = 0x00000001075118f1
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  8  XUL!nsAppShell::ProcessGeckoEvents(void*) [nsAppShell.mm:0dff674b4497 : 392 + 0x8]
02:01:33     INFO -     rbx = 0x00007f8f83e03a80   rbp = 0x00007fff5bc55770
02:01:33     INFO -     rsp = 0x00007fff5bc55730   r12 = 0x00007f8f83c27160
02:01:33     INFO -     r13 = 0x0000000000002403   r14 = 0x00000001042b2000
02:01:33     INFO -     r15 = 0x0000000114104230   rip = 0x000000010757bb91
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO -  9  CoreFoundation!__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 0x11
02:01:33     INFO -     rbx = 0x00007f8f83e03a80   rbp = 0x00007fff5bc55780
02:01:33     INFO -     rsp = 0x00007fff5bc55780   r12 = 0x00007f8f83c27160
02:01:33     INFO -     r13 = 0x0000000000002403   r14 = 0x00007f8f83c27178
02:01:33     INFO -     r15 = 0x00007f8f83e03988   rip = 0x00007fff8d9e2a01
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO - 10  CoreFoundation!__CFRunLoopDoSources0 + 0x10d
02:01:33     INFO -     rbp = 0x00007fff5bc557e0   rsp = 0x00007fff5bc55790
02:01:33     INFO -     rip = 0x00007fff8d9d4b8d
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 11  CoreFoundation!__CFRunLoopRun + 0x39f
02:01:33     INFO -     rbp = 0x00007fff5bc564c0   rsp = 0x00007fff5bc557f0
02:01:33     INFO -     rip = 0x00007fff8d9d41bf
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 12  CoreFoundation!CFRunLoopRunSpecific + 0x128
02:01:33     INFO -     rbp = 0x00007fff5bc56520   rsp = 0x00007fff5bc564d0
02:01:33     INFO -     rip = 0x00007fff8d9d3bd8
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 13  HIToolbox!RunCurrentEventLoopInMode + 0xeb
02:01:33     INFO -     rbp = 0x00007fff5bc56560   rsp = 0x00007fff5bc56530
02:01:33     INFO -     rip = 0x00007fff8624156f
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 14  HIToolbox!ReceiveNextEventCommon + 0x1af
02:01:33     INFO -     rbp = 0x00007fff5bc565e0   rsp = 0x00007fff5bc56570
02:01:33     INFO -     rip = 0x00007fff862412ea
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 15  HIToolbox!_BlockUntilNextEventMatchingListInModeWithFilter + 0x47
02:01:33     INFO -     rbp = 0x00007fff5bc56600   rsp = 0x00007fff5bc565f0
02:01:33     INFO -     rip = 0x00007fff8624112b
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 16  AppKit!_DPSNextEvent + 0x3d2
02:01:33     INFO -     rbp = 0x00007fff5bc56a70   rsp = 0x00007fff5bc56610
02:01:33     INFO -     rip = 0x00007fff865f38ab
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 17  AppKit!-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 0x15a
02:01:33     INFO -     rbp = 0x00007fff5bc56d10   rsp = 0x00007fff5bc56a80
02:01:33     INFO -     rip = 0x00007fff865f2e58
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 18  XUL!-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] [nsAppShell.mm:0dff674b4497 : 128 + 0x2c]
02:01:33     INFO -     rbp = 0x00007fff5bc56d60   rsp = 0x00007fff5bc56d20
02:01:33     INFO -     rip = 0x000000010757ae86
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 19  AppKit!-[NSApplication run] + 0x252
02:01:33     INFO -     rbx = 0x000000012133b160   rbp = 0x00007fff5bc56de0
02:01:33     INFO -     rsp = 0x00007fff5bc56d70   r12 = 0x0000000000000000
02:01:33     INFO -     r13 = 0x00000001248ad580   r14 = 0x00000001248ad580
02:01:33     INFO -     r15 = 0x00007f8f8400a101   rip = 0x00007fff865e8af3
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO - 20  XUL!nsAppShell::Run() [nsAppShell.mm:0dff674b4497 : 666 + 0x17]
02:01:33     INFO -     rbp = 0x00007fff5bc56e30   rsp = 0x00007fff5bc56df0
02:01:33     INFO -     rip = 0x000000010757c295
02:01:33     INFO -     Found by: previous frame's frame pointer
02:01:33     INFO - 21  XUL!nsAppStartup::Run() [nsAppStartup.cpp:0dff674b4497 : 283 + 0x6]
02:01:33     INFO -     rbx = 0x0000000114140e70   rbp = 0x00007fff5bc56e50
02:01:33     INFO -     rsp = 0x00007fff5bc56e40   r14 = 0x00007fff5bc572f9
02:01:33     INFO -     r15 = 0x00007fff5bc56e98   rip = 0x0000000108220c51
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO - 22  XUL!XREMain::XRE_mainRun() [nsAppRunner.cpp:0dff674b4497 : 4480 + 0x6]
02:01:33     INFO -     rbx = 0x00007fff5bc56e88   rbp = 0x00007fff5bc57110
02:01:33     INFO -     rsp = 0x00007fff5bc56e60   r14 = 0x00007fff5bc572f9
02:01:33     INFO -     r15 = 0x00007fff5bc56e98   rip = 0x00000001082c6193
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO - 23  XUL!XREMain::XRE_main(int, char**, nsXREAppData const*) [nsAppRunner.cpp:0dff674b4497 : 4613 + 0x8]
02:01:33     INFO -     rbx = 0x0000000000000000   rbp = 0x00007fff5bc57180
02:01:33     INFO -     rsp = 0x00007fff5bc57120   r12 = 0x0000000000000001
02:01:33     INFO -     r13 = 0x0000000000000006   r14 = 0x00007fff5bc57190
02:01:33     INFO -     r15 = 0x00007fff5bc577f8   rip = 0x00000001082c6bd3
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO - 24  XUL!XRE_main [nsAppRunner.cpp:0dff674b4497 : 4704 + 0x11]
02:01:33     INFO -     rbx = 0x00007fff5bc572b8   rbp = 0x00007fff5bc57330
02:01:33     INFO -     rsp = 0x00007fff5bc57190   r12 = 0x0000000000000006
02:01:33     INFO -     r13 = 0x00007fff5bc57190   r14 = 0x00007fff5bc573a0
02:01:33     INFO -     r15 = 0x00007fff5bc577f8   rip = 0x00000001082c7169
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO - 25  firefox!main [nsBrowserApp.cpp:0dff674b4497 : 328 + 0x16]
02:01:33     INFO -     rbx = 0x0000000104213580   rbp = 0x00007fff5bc577d0
02:01:33     INFO -     rsp = 0x00007fff5bc57340   r12 = 0x0000000104679060
02:01:33     INFO -     r13 = 0x0000000104213340   r14 = 0x00007fff5bc57360
02:01:33     INFO -     r15 = 0xa6005900081a2cc4   rip = 0x0000000103fa9ea9
02:01:33     INFO -     Found by: call frame info
02:01:33     INFO - 26  firefox!start + 0x34
02:01:33     INFO -     rbx = 0x0000000000000000   rbp = 0x00007fff5bc577e8
02:01:33     INFO -     rsp = 0x00007fff5bc577e0   r12 = 0x0000000000000000
02:01:33     INFO -     r13 = 0x0000000000000000   r14 = 0x0000000000000000
02:01:33     INFO -     r15 = 0x0000000000000000   rip = 0x0000000103fa9714
02:01:33     INFO -     Found by: call frame info
Flags: needinfo?(wmccloskey)
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9305479122
Label some common runnables with a DocGroup (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d97f42563af
Label XHR timer runnables with DocGroup (r=bkelly)
https://hg.mozilla.org/integration/mozilla-inbound/rev/044347624e60
Label PluginInstance IPC messages with plugin's DocGroup (r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/526959c0620c
Label DispatchContentLoaded events with a DocGroup (r=ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0576b5d99809
Label MessagePort runnables by DocGroup (r=bkelly)
https://hg.mozilla.org/integration/mozilla-inbound/rev/279c4b0395ea
Assign DocGroup for ScriptLoader runnable (r=ehsan)
Bug 1317844 allows us to assign an event target to a given actor. I added code in this bug so we can then query an actor's event target with GetActorEventTarget (the "Assign a TabGroup to every PBrowser" patch).

However, there is a time period when GetActorEventTarget doesn't work. While AllocPBrowserChild is running, we haven't registered the ID of the actor, so GetActorEventTarget doesn't know what to look up in the map. I can't think of any way to fix this.

Instead, I think we should just forbid calling GetActorEventTarget at this time. In theory, AllocPBrowserChild should just allocate an object. In practice, it does all kinds of stuff. This patch moves most of the work from AllocPBrowserChild to RecvPBrowserConstructor. I think this is basically doing the same thing, and it's cleaner. We register the actor ID after AllocPBrowserChild and before RecvPBrowserConstructor, so it will also fix my bug.
Flags: needinfo?(wmccloskey)
Attachment #8816364 - Flags: review?(dvander)
Comment on attachment 8812347 [details]
Bug 1318506 - Label HttpChannelChild actors with DocGroup/TabGroup ()

https://reviewboard.mozilla.org/r/94090/#review97454

I assume it's deliberate that we're doing this only for channels that originate on the child, i.e this stuff isn't expected to work in single-process mode.

::: netwerk/protocol/http/HttpChannelChild.cpp:2021
(Diff revision 2)
> +  RefPtr<Dispatcher> dispatcher;
> +  if (doc) {
> +    dispatcher = doc->GetDocGroup();
> +  } else {
> +    // Top-level loads won't have a DocGroup yet. So instead we target them at
> +    // the TabGroup, which is the best we can do at this time.

If it would be useful to assert that the channel is for a top-level document here (as opposed to just assuming it is if !doc), you could assert that the loadFlags of the channel contain LOAD_DOCUMENT_URI.
Attachment #8812347 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8812347 [details]
Bug 1318506 - Label HttpChannelChild actors with DocGroup/TabGroup ()

Jason asked me to take a look as well.
Attachment #8812347 - Flags: review?(honzab.moz)
Comment on attachment 8812347 [details]
Bug 1318506 - Label HttpChannelChild actors with DocGroup/TabGroup ()

https://reviewboard.mozilla.org/r/94090/#review98946

::: netwerk/ipc/ChannelEventQueue.h:79
(Diff revision 2)
>    // dispatched in a new event on the current thread.
>    void Resume();
>  
>    // Retargets delivery of events to the target thread specified.
>    nsresult RetargetDeliveryTo(nsIEventTarget* aTargetThread);
> +  nsresult ResetDeliveryTarget();

comments: when this can be called, what it is doing, etc..

::: netwerk/protocol/http/HttpChannelChild.h:182
(Diff revision 2)
>      RefPtr<InterceptStreamListener> mListener;
>      nsCOMPtr<nsIInputStream> mInput;
>      nsAutoPtr<nsHttpResponseHead> mHead;
>    };
>  
> +  void SetEventTarget();

comments what this exactly do and when it's called

::: netwerk/protocol/http/HttpChannelChild.cpp:1205
(Diff revision 2)
>  {
>    // Hold a ref to this to keep it from being deleted by Send__delete__()
>    RefPtr<HttpChannelChild> self(this);
>    Send__delete__(this);
>  
> +  mEventQ->ResetDeliveryTarget();

add a comment why this is exactly here

::: netwerk/protocol/http/HttpChannelChild.cpp:1609
(Diff revision 2)
>  
>    // The socket transport in the chrome process now holds a logical ref to us
>    // until OnStopRequest, or we do a redirect, or we hit an IPDL error.
>    AddIPDLReference();
>  
> +  SetEventTarget();

should this happen after the construction?
Attachment #8812347 - Flags: review?(honzab.moz)
Comment on attachment 8812347 [details]
Bug 1318506 - Label HttpChannelChild actors with DocGroup/TabGroup ()

https://reviewboard.mozilla.org/r/94090/#review98948

f**** rb..

please just add comments before landing (mandatory!), otherwise this seems to be OK to me, but I didn't take a look at the guts behind SetEventTargetForActor and what risks it could take when used this way
Attachment #8812347 - Flags: review+
Comment on attachment 8816364 [details] [diff] [review]
PBrowser constructor change

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

::: dom/ipc/nsIContentChild.cpp
@@ +83,5 @@
> +{
> +  // This runs after AllocPBrowserChild() returns and the IPC machinery for this
> +  // PBrowserChild has been set up.
> +
> +  auto tabChild = static_cast<TabChild*>(static_cast<TabChild*>(aActor));

nit: double static_cast here
Attachment #8816364 - Flags: review?(dvander) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd1cb767041
Initialize TabChild in constructor message, not allocation (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/83c362909069
Label HttpChannelChild actors with DocGroup/TabGroup (r=jduell)
https://hg.mozilla.org/integration/mozilla-inbound/rev/32a005fd81f3
Assign a TabGroup to every PBrowser (r=mystor,ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/30b32a2aabb1
Label AsyncEventDispatcher runnables with DocGroup (r=ehsan)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.