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.

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.

Comment on attachment 8812015 [details] Bug 1318506 - Label MessagePort runnables by DocGroup () https://reviewboard.mozilla.org/r/93906/#review94182

Comment on attachment 8812012 [details] Bug 1318506 - Label PluginInstance IPC messages with plugin's DocGroup () https://reviewboard.mozilla.org/r/93900/#review94260

Comment on attachment 8812011 [details] Bug 1318506 - Label XHR timer runnables with DocGroup () https://reviewboard.mozilla.org/r/93898/#review94290

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.

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.

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.

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

(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

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.

Comment on attachment 8812354 [details] Bug 1318506 - Assign DocGroup for ScriptLoader runnable () https://reviewboard.mozilla.org/r/94104/#review94604

Comment on attachment 8812350 [details] Bug 1318506 - Assign TabGroup to runnable for TabGroup's throttled event queue () https://reviewboard.mozilla.org/r/94096/#review94620

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 } ```

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.

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).

(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

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.)

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

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

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.

https://hg.mozilla.org/mozilla-central/rev/2b9305479122 https://hg.mozilla.org/mozilla-central/rev/1d97f42563af https://hg.mozilla.org/mozilla-central/rev/044347624e60 https://hg.mozilla.org/mozilla-central/rev/526959c0620c https://hg.mozilla.org/mozilla-central/rev/0576b5d99809 https://hg.mozilla.org/mozilla-central/rev/279c4b0395ea

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.

Comment on attachment 8812347 [details] Bug 1318506 - Label HttpChannelChild actors with DocGroup/TabGroup () Jason asked me to take a look as well.

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?

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

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

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)

https://hg.mozilla.org/mozilla-central/rev/ecd1cb767041 https://hg.mozilla.org/mozilla-central/rev/83c362909069 https://hg.mozilla.org/mozilla-central/rev/32a005fd81f3 https://hg.mozilla.org/mozilla-central/rev/30b32a2aabb1