990812 - Window-level frame scripts are loaded into add-on panels and cause errors

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[Matthew N. [:MattN]]

When I start the browser in a debug build on Win7 I see the following error.

System JS : ERROR chrome://browser/content/content-sessionStore.js:230 - TypeError: docShell.QueryInterface(...).sessionHistory is null

It seems to appear near System JS : ERROR chrome://browser/content/browser.js:14430 - TypeError: value is not a non-null object

but I don't know if it's related

The same is being seen in automation such as the logs in bug 989188.

I don't know if this has any user-facing impact.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

I unfortunately can't reproduce it with a debug build on Win 7. It however seems like .sessionHistory is null just because it isn't set yet.

I'm not sure how this could happen as in bug 971697 I made sure to set the sessionHistory before creating the message manager... OTOH, accessing .messageManager in browser.xml might load the frame script before the docShell is ready?

I hope we can find some way to reproduce this...

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

I just hit this while working on side project that adds a toolbar button with panel:

System JS : ERROR chrome://browser/content/browser.js:14681 - TypeError: value is not a non-null object
System JS : ERROR chrome://browser/content/content-sessionStore.js:230 - TypeError: docShell.QueryInterface(...).sessionHistory is null

The first error is coming from the gPageStyleMenu. I think this is indeed the problem of using the window's messageManager to attach content scripts to frame loaders. Panels will load those as well. It means that this isn't a sessionstore-only problem, browser/base/content/content.js is loaded as well.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Currently, getting things wrong by adding a pending frame script to the global or a window's message manager is very easy to get wrong. This does also potentially affect lots of add-ons that don't expect this type of behavior and naively inject a <browser> or the like into the browser window.

An idea might be to disallow "global" frame scripts without specifying a condition. This could be a function that is called whenever we want to load this frame script and check whether it returns true for the given frame loader owner. The browser window and sessionstore could then just add a method that returns true if the given element is a browser contained in gBrowser.browsers.

A different idea would be to somehow make it possible for frame scripts to bail out if loaded in the wrong context. This might not be easy to do due to the lack of information and the separation of content scripts. It would also be costlier than not loading the script at all.

Comment 4

[Mike Hoye [:mhoye]]

After the triage meeting, this bug remains unassigned. Tim, would you be willing to mentor a contributor through this?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Mike, this doesn't feel like a good mentoring bug. We don't even now what a possible solution might look like yet. Once we get there I will gladly reconsider turning this into a mentored bug.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(Please see comment #2 and comment #3.)

Bill, do you have any idea how to proceed here? IIRC you wrote the patch that makes us use the window's messageManager to attach the frame script. Can we revert that change? I think you ran into some intermittent failures where the sync handler wasn't ready early enough.

Olli, do you have any ideas or recommendations for how we could tackle this problem? Would a "conditional frame script" be feasible or even a good idea?

Comment 7

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

Olli and I talked a while ago about creating alternate hierarchies of message managers. That would allow us to have a message manager that just covered all the <browser>s in a given window. Then we could use that message manager ourselves.

Is this the same problem as Matt was seeing though?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Conditional might make sense. When we're about to load a new script from global or window level, 
we'd call some callback which checks whether we actually want to load it.
A bit hackish still.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-990812-Allow-a-deeper-frame-message-manager-hier.patch

This patch allows frame message managers to have a deeper hierarchy. Instead of "global > window > tab" we can now also have "global > window > group > tab".

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-990812-Add-nsIDOMChromeWindow.getGroupMessageMan.patch

This patch introduces a new API, nsIDOMChromeWindow.getGroupMessageManager(string groupName). It allows to retrieve (and lazily create) message manager that handle a group of frame loaders in a given window.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-990812-Let-the-frame-loader-use-a-group-message-.patch

This patch makes the frame loader use a group message manager as the parent when the "mmgroup" attribute on a XUL element is set. I wasn't able to come up with a better API but this doesn't seem too terrible?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-990812-Restrict-sessionstore-and-browser-frame-s.patch

This patch lets the browser and sessionstore use the "browsers" group message manager. The whole sessionstore test suite completes successfully with all preceding patches applied.

I also can't reproduce the error messages anymore when launching Firefox with an SDK add-on installed that has a panel.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Writing tests shouldn't be too hard but I'll better wait until I have some feedback.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Curious, why you end up implementing this approach, and not just some simpler callback thing?

(I don't yet have opinion about this. Feels a bit complicated, but perhaps it is still ok.)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #15)
> Curious, why you end up implementing this approach, and not just some
> simpler callback thing?

Good question. I felt that the callback thing was my first quick idea and I had the impression that also you would favor a better solution? I liked the idea of supporting a more complex frame message manager hierarchy - and while there are four patches the changes aren't actually that big :)

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8427699 [details] [diff] [review]
0001-Bug-990812-Allow-a-deeper-frame-message-manager-hier.patch

>+MarkChildMessageManagers(nsIMessageBroadcaster* mm)
aMM


>+{
>+  mm->MarkForCC();
>+
>+  uint32_t tabChildCount = 0;
>+  mm->GetChildCount(&tabChildCount);
>+  for (uint32_t j = 0; j < tabChildCount; ++j) {
>+    nsCOMPtr<nsIMessageListenerManager> childMM;
>+    mm->GetChildAt(j, getter_AddRefs(childMM));
>+    if (!childMM) {
>+      continue;
>+    }
>+
>+    nsCOMPtr<nsIMessageBroadcaster> strongWinMM = do_QueryInterface(childMM);
>+    nsIMessageBroadcaster* winMM = strongWinMM;
So this isn't necessarily window level. So perhaps nonLeafMM or some such


>   // We have parent manager if we're a window message manager.
>   // In that case we want to load the pending scripts from global
>   // message manager.
update the comment

>-  if (mParentManager) {
>-    nsRefPtr<nsFrameMessageManager> globalMM = mParentManager;
>-    for (uint32_t i = 0; i < globalMM->mPendingScripts.Length(); ++i) {
>-      aManager->LoadFrameScript(globalMM->mPendingScripts[i], false,
>-                                globalMM->mPendingScriptsGlobalStates[i]);
>+  nsRefPtr<nsFrameMessageManager> parent = mParentManager;
>+  while (parent) {
>+    for (uint32_t i = 0; i < parent->mPendingScripts.Length(); ++i) {
>+      aManager->LoadFrameScript(parent->mPendingScripts[i], false,
>+                                parent->mPendingScriptsGlobalStates[i]);
>     }
>+
>+    parent = parent->mParentManager;
So this changes ordering. Currently scripts in the global mm are loaded before scripts in window mm.
With this change, if 'this' is group mm, we load first scripts from window mm and then from global mm.


>+++ b/content/base/src/nsFrameMessageManager.h
>@@ -171,17 +171,17 @@ public:
>   {
>     NS_ASSERTION(mChrome || !aParentManager, "Should not set parent manager!");
>     NS_ASSERTION(!mIsBroadcaster || !mCallback,
>                  "Broadcasters cannot have callbacks!");
>     // This is a bit hackish. When parent manager is global, we want
>     // to attach the window message manager to it immediately.
>     // Is it just the frame message manager which waits until the
>     // content process is running.
>-    if (mParentManager && (mCallback || IsWindowLevel())) {
>+    if (mParentManager && (mCallback || IsBroadcaster())) {
Update the comment to not be explicitly about window mm

Comment 18

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

This is really nice Tim! I thought it would be more complex to do something like this.

I really think it's better to do it this way than to do the callback. With the callback, we'd still be sending and receiving messages from non-tab browsers. Most of the time that wouldn't matter, but it seems undesirable.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8427700 [details] [diff] [review]
0002-Bug-990812-Add-nsIDOMChromeWindow.getGroupMessageMan.patch

You need to traverse/unlink mGroupMessageManagers.
* goes with the type, not with member variable name.
You could use nsAutoPtr for mGroupMessageManagers. That way
there wasn't need for manual delete mGroupMessageManagers

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8427704 [details] [diff] [review]
0003-Bug-990812-Let-the-frame-loader-use-a-group-message-.patch

s/mmgroup/messagemanagergroup/

>   if (chromeWindow) {
>-    chromeWindow->GetMessageManager(getter_AddRefs(parentManager));
>+    if (mOwnerContent->IsXUL() &&
>+        mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mmgroup)) {
>+
>+      nsAutoString mmgroup;
>+      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::mmgroup, mmgroup);
GetAttr returns true if it finds an attr. So just use that and no need for separate
HasAttr check.

>+
>+      if (!mmgroup.IsEmpty()) {
Nor IsEmpty check


For tests, at least tab/frameloader swapping should be tested. I think it should just work.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-990812-Allow-a-deeper-frame-message-manager-hier.patch, v2

All comments addressed.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-990812-Make-a11y-jsat-utils-handle-group-message.patch

We need to fix jsat/Utils to handle group message managers.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-990812-Add-nsIDOMChromeWindow.getGroupMessageMan.patch, v2

Comments addressed.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-990812-Let-the-frame-loader-use-a-group-message-.patch, v2

Will handle tests in a separate patch.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-990812-Restrict-sessionstore-and-browser-frame-s.patch, v2

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-990812-Restrict-sessionstore-and-browser-frame-s.patch, v3

Think I figured out the last oranges, should be good to go.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8428123 [details] [diff] [review]
0001-Bug-990812-Allow-a-deeper-frame-message-manager-hier.patch, v2

Try is crashing in nsPresContext::UIResolutionChangedInternal() when swapping docShells, that definitely needs to be updated.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-990812-Allow-a-deeper-frame-message-manager-hier.patch, v3

Fixed nsPresContext::UIResolutionChangedInternal().

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-990812-Restrict-sessionstore-tabview-and-browser.patch, v4

I could have run tabview tests at least locally, sorry.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-990812-Tests-for-group-message-managers.patch

Tests.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Try is looking good:

https://tbpl.mozilla.org/?tree=Try&rev=2a4d5c90d9e4

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8428243 [details] [diff] [review]
0001-Bug-990812-Allow-a-deeper-frame-message-manager-hier.patch, v3

>From 35bbdeb694a6004559ffecb6f32f0ce90464aecb Mon Sep 17 00:00:00 2001
>From: Tim Taubert <[e-mail]>
>Date: Fri, 23 May 2014 14:52:36 +0200
>Subject: Bug 990812 - Allow a deeper frame message manager hierarchy than only
> global > window > tab
>
>
>diff --git a/content/base/src/nsCCUncollectableMarker.cpp b/content/base/src/nsCCUncollectableMarker.cpp
>index b677f458..dac8dd9 100644
>--- a/content/base/src/nsCCUncollectableMarker.cpp
>+++ b/content/base/src/nsCCUncollectableMarker.cpp
>@@ -89,75 +89,81 @@ MarkUserDataHandler(void* aNode, nsIAtom* aKey, void* aValue, void* aData)
> {
>   nsIDocument* d = static_cast<nsINode*>(aNode)->GetCurrentDoc();
>   if (d && nsCCUncollectableMarker::InGeneration(d->GetMarkedCCGeneration())) {
>     Element::MarkUserDataHandler(aNode, aKey, aValue, aData);
>   }
> }
> 
> static void
>+MarkChildMessageManagers(nsIMessageBroadcaster* aMM)
>+{
>+  aMM->MarkForCC();
>+
>+  uint32_t tabChildCount = 0;
>+  aMM->GetChildCount(&tabChildCount);
>+  for (uint32_t j = 0; j < tabChildCount; ++j) {
>+    nsCOMPtr<nsIMessageListenerManager> childMM;
>+    aMM->GetChildAt(j, getter_AddRefs(childMM));
>+    if (!childMM) {
>+      continue;
>+    }
>+
>+    nsCOMPtr<nsIMessageBroadcaster> strongNonLeafMM = do_QueryInterface(childMM);
>+    nsIMessageBroadcaster* nonLeafMM = strongNonLeafMM;
>+
>+    nsCOMPtr<nsIMessageSender> strongTabMM = do_QueryInterface(childMM);
>+    nsIMessageSender* tabMM = strongTabMM;
>+
>+    strongNonLeafMM = nullptr;
>+    strongTabMM = nullptr;
>+    childMM = nullptr;
>+
>+    if (nonLeafMM) {
>+      MarkChildMessageManagers(nonLeafMM);
>+      continue;
>+    }
>+
>+    tabMM->MarkForCC();
>+
>+    //XXX hack warning, but works, since we know that
>+    //    callback is frameloader.
>+    mozilla::dom::ipc::MessageManagerCallback* cb =
>+      static_cast<nsFrameMessageManager*>(tabMM)->GetCallback();
>+    if (cb) {
>+      nsFrameLoader* fl = static_cast<nsFrameLoader*>(cb);
>+      EventTarget* et = fl->GetTabChildGlobalAsEventTarget();
>+      if (!et) {
>+        continue;
>+      }
>+      static_cast<nsInProcessTabChildGlobal*>(et)->MarkForCC();
>+      EventListenerManager* elm = et->GetExistingListenerManager();
>+      if (elm) {
>+        elm->MarkForCC();
>+      }
>+    }
>+  }
>+}
>+
>+static void
> MarkMessageManagers()
> {
>   // The global message manager only exists in the root process.
>   if (XRE_GetProcessType() != GeckoProcessType_Default) {
>     return;
>   }
>   nsCOMPtr<nsIMessageBroadcaster> strongGlobalMM =
>     do_GetService("@mozilla.org/globalmessagemanager;1");
>   if (!strongGlobalMM) {
>     return;
>   }
>   nsIMessageBroadcaster* globalMM = strongGlobalMM;
>   strongGlobalMM = nullptr;
>+  MarkChildMessageManagers(globalMM);
> 
>-  globalMM->MarkForCC();
>-  uint32_t childCount = 0;
>-  globalMM->GetChildCount(&childCount);
>-  for (uint32_t i = 0; i < childCount; ++i) {
>-    nsCOMPtr<nsIMessageListenerManager> childMM;
>-    globalMM->GetChildAt(i, getter_AddRefs(childMM));
>-    if (!childMM) {
>-      continue;
>-    }
>-    nsCOMPtr<nsIMessageBroadcaster> strongWindowMM = do_QueryInterface(childMM);
>-    nsIMessageBroadcaster* windowMM = strongWindowMM;
>-    childMM = nullptr;
>-    strongWindowMM = nullptr;
>-    windowMM->MarkForCC();
>-    uint32_t tabChildCount = 0;
>-    windowMM->GetChildCount(&tabChildCount);
>-    for (uint32_t j = 0; j < tabChildCount; ++j) {
>-      nsCOMPtr<nsIMessageListenerManager> childMM;
>-      windowMM->GetChildAt(j, getter_AddRefs(childMM));
>-      if (!childMM) {
>-        continue;
>-      }
>-      nsCOMPtr<nsIMessageSender> strongTabMM = do_QueryInterface(childMM);
>-      nsIMessageSender* tabMM = strongTabMM;
>-      childMM = nullptr;
>-      strongTabMM = nullptr;
>-      tabMM->MarkForCC();
>-      //XXX hack warning, but works, since we know that
>-      //    callback is frameloader.
>-      mozilla::dom::ipc::MessageManagerCallback* cb =
>-        static_cast<nsFrameMessageManager*>(tabMM)->GetCallback();
>-      if (cb) {
>-        nsFrameLoader* fl = static_cast<nsFrameLoader*>(cb);
>-        EventTarget* et = fl->GetTabChildGlobalAsEventTarget();
>-        if (!et) {
>-          continue;
>-        }
>-        static_cast<nsInProcessTabChildGlobal*>(et)->MarkForCC();
>-        EventListenerManager* elm = et->GetExistingListenerManager();
>-        if (elm) {
>-          elm->MarkForCC();
>-        }
>-      }
>-    }
>-  }
>   if (nsFrameMessageManager::sParentProcessManager) {
>     nsFrameMessageManager::sParentProcessManager->MarkForCC();
>     uint32_t childCount = 0;
>     nsFrameMessageManager::sParentProcessManager->GetChildCount(&childCount);
>     for (uint32_t i = 0; i < childCount; ++i) {
>       nsCOMPtr<nsIMessageListenerManager> childMM;
>       nsFrameMessageManager::sParentProcessManager->
>         GetChildAt(i, getter_AddRefs(childMM));
>diff --git a/content/base/src/nsFrameMessageManager.cpp b/content/base/src/nsFrameMessageManager.cpp
>index 97636bc..d4e3d27 100644
>--- a/content/base/src/nsFrameMessageManager.cpp
>+++ b/content/base/src/nsFrameMessageManager.cpp
>@@ -426,17 +426,17 @@ NS_IMETHODIMP
> nsFrameMessageManager::LoadFrameScript(const nsAString& aURL,
>                                        bool aAllowDelayedLoad,
>                                        bool aRunInGlobalScope)
> {
>   // FIXME: Bug 673569 is currently disabled.
>   aRunInGlobalScope = true;
> 
>   if (aAllowDelayedLoad) {
>-    if (IsGlobal() || IsWindowLevel()) {
>+    if (IsGlobal() || IsBroadcaster()) {
>       // Cache for future windows or frames
>       mPendingScripts.AppendElement(aURL);
>       mPendingScriptsGlobalStates.AppendElement(aRunInGlobalScope);
>     } else if (!mCallback) {
>       // We're frame message manager, which isn't connected yet.
>       mPendingScripts.AppendElement(aURL);
>       mPendingScriptsGlobalStates.AppendElement(aRunInGlobalScope);
>       return NS_OK;
>@@ -476,17 +476,17 @@ nsFrameMessageManager::RemoveDelayedFrameScript(const nsAString& aURL)
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsFrameMessageManager::GetDelayedFrameScripts(JSContext* aCx, JS::MutableHandle<JS::Value> aList)
> {
>   // Frame message managers may return an incomplete list because scripts
>   // that were loaded after it was connected are not added to the list.
>-  if (!IsGlobal() && !IsWindowLevel()) {
>+  if (!IsGlobal() && !IsBroadcaster()) {
>     NS_WARNING("Cannot retrieve list of pending frame scripts for frame"
>                "message managers as it may be incomplete");
>     return NS_ERROR_NOT_IMPLEMENTED;
>   }
> 
>   JS::Rooted<JSObject*> array(aCx, JS_NewArrayObject(aCx, mPendingScripts.Length()));
>   NS_ENSURE_TRUE(array, NS_ERROR_OUT_OF_MEMORY);
> 
>@@ -585,17 +585,17 @@ nsFrameMessageManager::SendMessage(const nsAString& aMessageName,
>                                    JS::Handle<JS::Value> aObjects,
>                                    nsIPrincipal* aPrincipal,
>                                    JSContext* aCx,
>                                    uint8_t aArgc,
>                                    JS::MutableHandle<JS::Value> aRetval,
>                                    bool aIsSync)
> {
>   NS_ASSERTION(!IsGlobal(), "Should not call SendSyncMessage in chrome");
>-  NS_ASSERTION(!IsWindowLevel(), "Should not call SendSyncMessage in chrome");
>+  NS_ASSERTION(!IsBroadcaster(), "Should not call SendSyncMessage in chrome");
>   NS_ASSERTION(!mParentManager, "Should not have parent manager in content!");
> 
>   aRetval.setUndefined();
>   NS_ENSURE_TRUE(mCallback, NS_ERROR_NOT_INITIALIZED);
> 
>   if (sSendingSyncMessage && aIsSync) {
>     // No kind of blocking send should be issued on top of a sync message.
>     return NS_ERROR_UNEXPECTED;
>@@ -1094,29 +1094,36 @@ nsFrameMessageManager::ReceiveMessage(nsISupports* aTarget,
> 
> void
> nsFrameMessageManager::AddChildManager(nsFrameMessageManager* aManager)
> {
>   mChildManagers.AppendObject(aManager);
> 
>   nsRefPtr<nsFrameMessageManager> kungfuDeathGrip = this;
>   nsRefPtr<nsFrameMessageManager> kungfuDeathGrip2 = aManager;
>-  // We have parent manager if we're a window message manager.
>-  // In that case we want to load the pending scripts from global
>-  // message manager.
>-  if (mParentManager) {
>-    nsRefPtr<nsFrameMessageManager> globalMM = mParentManager;
>-    for (uint32_t i = 0; i < globalMM->mPendingScripts.Length(); ++i) {
>-      aManager->LoadFrameScript(globalMM->mPendingScripts[i], false,
>-                                globalMM->mPendingScriptsGlobalStates[i]);
>-    }
>+
>+  LoadPendingScripts(this, aManager);
>+}
>+
>+void
>+nsFrameMessageManager::LoadPendingScripts(nsFrameMessageManager* aManager,
>+                                          nsFrameMessageManager* aChildMM)
>+{
>+  // We have parent manager if we're a message broadcaster.
>+  // In that case we want to load the pending scripts from all parent
>+  // message managers in the hierarchy. Process the parent first so
>+  // that pending scripts higher up in the hierarchy are loaded before others.
>+  if (aManager->mParentManager) {
>+    LoadPendingScripts(aManager->mParentManager, aChildMM);
>   }
>-  for (uint32_t i = 0; i < mPendingScripts.Length(); ++i) {
>-    aManager->LoadFrameScript(mPendingScripts[i], false,
>-                              mPendingScriptsGlobalStates[i]);
>+
>+  for (uint32_t i = 0; i < aManager->mPendingScripts.Length(); ++i) {
>+    aChildMM->LoadFrameScript(aManager->mPendingScripts[i],
>+                              false,
>+                              aManager->mPendingScriptsGlobalStates[i]);
>   }
> }
> 
> void
> nsFrameMessageManager::SetCallback(MessageManagerCallback* aCallback)
> {
>   MOZ_ASSERT(!mIsBroadcaster || !mCallback,
>              "Broadcasters cannot have callbacks!");
>@@ -1133,17 +1140,17 @@ nsFrameMessageManager::InitWithCallback(MessageManagerCallback* aCallback)
> {
>   if (mCallback) {
>     // Initialization should only happen once.
>     return;
>   }
> 
>   SetCallback(aCallback);
> 
>-  // First load global scripts by adding this to parent manager.
>+  // First load parent scripts by adding this to parent manager.
>   if (mParentManager) {
>     mParentManager->AddChildManager(this);
>   }
> 
>   for (uint32_t i = 0; i < mPendingScripts.Length(); ++i) {
>     LoadFrameScript(mPendingScripts[i], false, mPendingScriptsGlobalStates[i]);
>   }
> }
>diff --git a/content/base/src/nsFrameMessageManager.h b/content/base/src/nsFrameMessageManager.h
>index 722258b..f851095 100644
>--- a/content/base/src/nsFrameMessageManager.h
>+++ b/content/base/src/nsFrameMessageManager.h
>@@ -168,20 +168,20 @@ public:
>     mDisconnected(false),
>     mCallback(aCallback),
>     mParentManager(aParentManager)
>   {
>     NS_ASSERTION(mChrome || !aParentManager, "Should not set parent manager!");
>     NS_ASSERTION(!mIsBroadcaster || !mCallback,
>                  "Broadcasters cannot have callbacks!");
>     // This is a bit hackish. When parent manager is global, we want
>-    // to attach the window message manager to it immediately.
>+    // to attach the message manager to it immediately.
>     // Is it just the frame message manager which waits until the
>     // content process is running.
>-    if (mParentManager && (mCallback || IsWindowLevel())) {
>+    if (mParentManager && (mCallback || IsBroadcaster())) {
>       mParentManager->AddChildManager(this);
>     }
>     if (mOwnsCallback) {
>       mOwnedCallback = aCallback;
>     }
>   }
> 
>   ~nsFrameMessageManager()
>@@ -253,17 +253,17 @@ public:
>   nsFrameMessageManager* GetParentManager() { return mParentManager; }
>   void SetParentManager(nsFrameMessageManager* aParent)
>   {
>     NS_ASSERTION(!mParentManager, "We have parent manager already!");
>     NS_ASSERTION(mChrome, "Should not set parent manager!");
>     mParentManager = aParent;
>   }
>   bool IsGlobal() { return mGlobal; }
>-  bool IsWindowLevel() { return mParentManager && mParentManager->IsGlobal(); }
>+  bool IsBroadcaster() { return mIsBroadcaster; }
> 
>   static nsFrameMessageManager* GetParentProcessManager()
>   {
>     return sParentProcessManager;
>   }
>   static nsFrameMessageManager* GetChildProcessManager()
>   {
>     return sChildProcessManager;
>@@ -291,16 +291,19 @@ protected:
>   bool mOwnsCallback;
>   bool mHandlingMessage;
>   bool mDisconnected;
>   mozilla::dom::ipc::MessageManagerCallback* mCallback;
>   nsAutoPtr<mozilla::dom::ipc::MessageManagerCallback> mOwnedCallback;
>   nsFrameMessageManager* mParentManager;
>   nsTArray<nsString> mPendingScripts;
>   nsTArray<bool> mPendingScriptsGlobalStates;
>+
>+  void LoadPendingScripts(nsFrameMessageManager* aManager,
>+                          nsFrameMessageManager* aChildMM);
> public:
>   static nsFrameMessageManager* sParentProcessManager;
>   static nsFrameMessageManager* sChildProcessManager;
>   static nsFrameMessageManager* sSameProcessParentManager;
>   static nsTArray<nsCOMPtr<nsIRunnable> >* sPendingSameProcessAsyncMessages;
> private:
>   enum ProcessCheckerType {
>     PROCESS_CHECKER_PERMISSION,
>diff --git a/layout/base/nsPresContext.cpp b/layout/base/nsPresContext.cpp
>index 9ee5a45..46c363d 100644
>--- a/layout/base/nsPresContext.cpp
>+++ b/layout/base/nsPresContext.cpp
>@@ -1773,44 +1773,57 @@ nsPresContext::UIResolutionChangedInternal()
>   }
> 
>   nsCOMPtr<nsIDOMChromeWindow> chromeWindow(do_QueryInterface(mDocument->GetWindow()));
>   nsCOMPtr<nsIMessageBroadcaster> windowMM;
>   if (chromeWindow) {
>     chromeWindow->GetMessageManager(getter_AddRefs(windowMM));
>   }
>   if (windowMM) {
>-    uint32_t tabChildCount = 0;
>-    windowMM->GetChildCount(&tabChildCount);
>-    for (uint32_t j = 0; j < tabChildCount; ++j) {
>-      nsCOMPtr<nsIMessageListenerManager> childMM;
>-      windowMM->GetChildAt(j, getter_AddRefs(childMM));
>-      if (!childMM) {
>-        continue;
>-      }
>-      nsCOMPtr<nsIMessageSender> tabMM = do_QueryInterface(childMM);
>-
>-      mozilla::dom::ipc::MessageManagerCallback* cb =
>-       static_cast<nsFrameMessageManager*>(tabMM.get())->GetCallback();
>-      if (cb) {
>-        nsFrameLoader* fl = static_cast<nsFrameLoader*>(cb);
>-        PBrowserParent* remoteBrowser = fl->GetRemoteBrowser();
>-        TabParent* remote = static_cast<TabParent*>(remoteBrowser);
>-        if (remote) {
>-          remote->UIResolutionChanged();
>-        }
>-      }
>-    }
>+    NotifyUIResolutionChanged(windowMM);
>   }
> 
>   mDocument->EnumerateSubDocuments(UIResolutionChangedSubdocumentCallback,
>                                    nullptr);
> }
> 
> void
>+nsPresContext::NotifyUIResolutionChanged(nsIMessageBroadcaster* aManager)
>+{
>+  uint32_t tabChildCount = 0;
>+  aManager->GetChildCount(&tabChildCount);
>+  for (uint32_t j = 0; j < tabChildCount; ++j) {
>+    nsCOMPtr<nsIMessageListenerManager> childMM;
>+    aManager->GetChildAt(j, getter_AddRefs(childMM));
>+    if (!childMM) {
>+      continue;
>+    }
>+
>+    nsCOMPtr<nsIMessageBroadcaster> nonLeafMM = do_QueryInterface(childMM);
>+    if (nonLeafMM) {
>+      NotifyUIResolutionChanged(nonLeafMM);
>+      continue;
>+    }
>+
>+    nsCOMPtr<nsIMessageSender> tabMM = do_QueryInterface(childMM);
>+
>+    mozilla::dom::ipc::MessageManagerCallback* cb =
>+     static_cast<nsFrameMessageManager*>(tabMM.get())->GetCallback();
>+    if (cb) {
>+      nsFrameLoader* fl = static_cast<nsFrameLoader*>(cb);
>+      PBrowserParent* remoteBrowser = fl->GetRemoteBrowser();
>+      TabParent* remote = static_cast<TabParent*>(remoteBrowser);
>+      if (remote) {
>+        remote->UIResolutionChanged();
>+      }
>+    }
>+  }
>+}
>+
>+void
> nsPresContext::EmulateMedium(const nsAString& aMediaType)
> {
>   nsIAtom* previousMedium = Medium();
>   mIsEmulatingMedia = true;
> 
>   nsAutoString mediaType;
>   nsContentUtils::ASCIIToLower(aMediaType, mediaType);
> 
>diff --git a/layout/base/nsPresContext.h b/layout/base/nsPresContext.h
>index c8eefcb..2300bb8 100644
>--- a/layout/base/nsPresContext.h
>+++ b/layout/base/nsPresContext.h
>@@ -31,16 +31,17 @@
> #include "nsTArray.h"
> #include "nsAutoPtr.h"
> #include "mozilla/MemoryReporting.h"
> #include "mozilla/TimeStamp.h"
> #include "mozilla/AppUnits.h"
> #include "prclist.h"
> #include "nsThreadUtils.h"
> #include "ScrollbarStyles.h"
>+#include "nsIMessageManager.h"
> 
> class nsBidiPresUtils;
> class nsAString;
> class nsIPrintSettings;
> class nsDocShell;
> class nsIDocShell;
> class nsIDocument;
> class nsILanguageAtomService;
>@@ -779,16 +780,22 @@ public:
> 
>   /*
>    * Notify the pres context that the resolution of the user interface has
>    * changed. This happens if a window is moved between HiDPI and non-HiDPI
>    * displays, so that the ratio of points to device pixels changes.
>    */
>   NS_HIDDEN_(void) UIResolutionChanged();
> 
>+  /**
>+   * Recursively notify all remote leaf descendants of a given message manager
>+   * that the resolution of the user interface has changed.
>+   */
>+  NS_HIDDEN_(void) NotifyUIResolutionChanged(nsIMessageBroadcaster* aManager);
>+
>   /*
>    * Notify the pres context that a system color has changed
>    */
>   NS_HIDDEN_(void) SysColorChanged();
> 
>   /** Printing methods below should only be used for Medium() == print **/
>   NS_HIDDEN_(void) SetPrintSettings(nsIPrintSettings *aPrintSettings);
> 
>-- 
>1.9.3
>

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

oops, sorry about posting the whole patch.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8428139 [details] [diff] [review]
0002-Bug-990812-Add-nsIDOMChromeWindow.getGroupMessageMan.patch, v2

I'm pretty sure this misses the change to
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Window.webidl?rev=c82b3f253337#388
You seem to implement such version of GetGroupMessageManager


This is still missing traverse/unlink

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8428417 [details] [diff] [review]
0006-Bug-990812-Tests-for-group-message-managers.patch

These tests could use some comments what they are testing.

Also, we need tests where there are several frame message managers using the same
message manager group.

But for these tests, r+ (assuming comments are added.)

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-990812-Add-nsIDOMChromeWindow.getGroupMessageMan.patch, v3

(In reply to Olli Pettay [:smaug] (vacation-ish May 26-30) from comment #34)
> I'm pretty sure this misses the change to
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Window.
> webidl?rev=c82b3f253337#388

Added.

> This is still missing traverse/unlink

Took me a while to figure out what you mean by that... but the code is in now and I hopefully got that right.

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-990812-Tests-for-group-message-managers-r-smaug.patch, v2

Added another test for multiple frame loaders and multiple groups. Carrying over r=smaug.

Comment 38

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8428126 [details] [diff] [review]
0005-Bug-990812-Make-a11y-jsat-utils-handle-group-message.patch

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

Looks good, thanks!

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/616a3e276f9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e92642e2df16
https://hg.mozilla.org/integration/mozilla-inbound/rev/69e90953e298
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca179dd048bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e98c6fdfa01

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

Landed the core changes (everything but part 4) to not let it bitrot.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Before closing this, the documentation in http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl#35 should be updated.

Comment 42

[Drew Willcoxon :adw]

Comment on attachment 8428273 [details] [diff] [review]
0004-Bug-990812-Restrict-sessionstore-tabview-and-browser.patch, v4

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

::: browser/base/content/browser.js
@@ +783,5 @@
>      DOMLinkHandler.init();
>      gPageStyleMenu.init();
>      LanguageDetectionListener.init();
>  
> +    let mm = getGroupMessageManager("browsers");

Nit: window.getGroupMessageManager?  It took me a minute to realize that function is defined on the window, but then again I'm not used to seeing it.

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Drew Willcoxon :adw from comment #42)
> > +    let mm = getGroupMessageManager("browsers");
> 
> Nit: window.getGroupMessageManager?  It took me a minute to realize that
> function is defined on the window, but then again I'm not used to seeing it.

Will fix.

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0007-Bug-990812-Fix-nsIMessageManager-documentation.patch

Updates the nsIMessageManager documentation.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/616a3e276f9e
https://hg.mozilla.org/mozilla-central/rev/e92642e2df16
https://hg.mozilla.org/mozilla-central/rev/69e90953e298
https://hg.mozilla.org/mozilla-central/rev/ca179dd048bc
https://hg.mozilla.org/mozilla-central/rev/2e98c6fdfa01

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8429482 [details] [diff] [review]
0007-Bug-990812-Fix-nsIMessageManager-documentation.patch

Thanks.

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c17e7e3cd729
https://hg.mozilla.org/integration/mozilla-inbound/rev/f35d007a22a7

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c17e7e3cd729
https://hg.mozilla.org/mozilla-central/rev/f35d007a22a7

Comment 49

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 50

[IU]

Are any of your recent tab-related changes responsible for tab duplication on tab drag-and-drop with today's nightly build?

Comment 51

[Tim Taubert [:ttaubert] (inactive)]

The frontend changes didn't make it into today's Nightly. So just in theory the backend changes might be at fault but they shouldn't change anything as long as no one uses the new API. I'll check.

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

Bug 1016960 already found out what's causing the regression.

Comment 53

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

There seem to be several automated tests for this... could you let us know what would need to be covered by the Manual QA team here?

Comment 54

[Marco Mucci [:MarcoM]]

Hi Juan, can I QA Contact be assigned to this bug for final verification.

Comment 55

[juan becerra [:juanb]]

Tim, could you suggest use cases we can try to test around this manually, just in case? For example, would restoring sessions with different tabs, windows, private/non-private windows, with some specific type of content (like plugins) help in testing this?

Comment 56

[Tim Taubert [:ttaubert] (inactive)]

Sure, all you need is an add-on that comes with a toolbarbutton and a panel. I wrote a small add-on you could use for testing [1]. When starting up with an affected version of Firefox you should see errors:

System JS : ERROR chrome://browser/content/browser.js:14815 - TypeError: value is not a non-null object
System JS : ERROR chrome://browser/content/content-sessionStore.js:230 - TypeError: docShell.QueryInterface(...).sessionHistory is null

I just verified that those errors show up in Beta and Aurora but are fixed in a recent Nightly.

[1] https://github.com/ttaubert/dothiv-firefox/raw/master/dothiv.xpi

Comment 57

[Bogdan Maris, Desktop QA]

Reproduced this error on old Nightly from 2014-04-01, verified that the error does not appear on latest Nightly debug build using Windows 7 64bit, Windows 8 32bit and Windows 8.1 64bit. Leaving [qa+] until the fix arrives in Aurora and Beta as well.

Comment 58

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as [qa!] since no uplift was done to Beta.