Closed Bug 990812 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected
firefox32 --- verified

People

(Reporter: MattN, Assigned: ttaubert)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [diamond] p=5 s=it-32c-31a-30b.3 [qa!])

Attachments

(7 files, 9 obsolete files)

1.37 KB, patch
yzen
: review+
Details | Diff | Splinter Review
2.45 KB, patch
smaug
: review+
Details | Diff | Splinter Review
16.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.53 KB, patch
adw
: review+
Details | Diff | Splinter Review
10.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.31 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
3.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
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...
Flags: firefox-backlog?
Keywords: regression
Flags: firefox-backlog? → firefox-backlog+
Summary: content-sessionStore.js:230 - TypeError: docShell.QueryInterface(...).sessionHistory is null → [Investigation] content-sessionStore.js:230 - TypeError: docShell.QueryInterface(...).sessionHistory is null
Whiteboard: p=5
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.
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.
Keywords: regression
OS: Windows 7 → All
After the triage meeting, this bug remains unassigned. Tim, would you be willing to mentor a contributor through this?
Flags: needinfo?(ttaubert)
Whiteboard: p=5 → p=5 [diamond]
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.
Flags: needinfo?(ttaubert)
(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?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bugs)
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?
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(bugs)
Duplicate of this bug: 1012170
Component: Session Restore → IPC
Product: Firefox → Core
Summary: [Investigation] content-sessionStore.js:230 - TypeError: docShell.QueryInterface(...).sessionHistory is null → Window-level frame scripts are loaded into add-on panels and cause errors
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".
Attachment #8427699 - Flags: review?(bugs)
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.
Attachment #8427700 - Flags: review?(bugs)
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?
Attachment #8427704 - Flags: review?(bugs)
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Writing tests shouldn't be too hard but I'll better wait until I have some feedback.
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.)
(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 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
Attachment #8427699 - Flags: review?(bugs) → review-
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 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
Attachment #8427700 - Flags: review?(bugs) → review-
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.
Attachment #8427704 - Flags: review?(bugs) → review-
All comments addressed.
Attachment #8427699 - Attachment is obsolete: true
Attachment #8428123 - Flags: review?(bugs)
We need to fix jsat/Utils to handle group message managers.
Attachment #8428126 - Flags: review?(yzenevich)
Comments addressed.
Attachment #8428139 - Flags: review?(bugs)
Will handle tests in a separate patch.
Attachment #8427700 - Attachment is obsolete: true
Attachment #8427704 - Attachment is obsolete: true
Attachment #8428155 - Flags: review?(bugs)
Think I figured out the last oranges, should be good to go.
Attachment #8428156 - Attachment is obsolete: true
Attachment #8428230 - Flags: review?(adw)
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.
Attachment #8428123 - Flags: review?(bugs)
Fixed nsPresContext::UIResolutionChangedInternal().
Attachment #8428123 - Attachment is obsolete: true
Attachment #8428243 - Flags: review?(bugs)
I could have run tabview tests at least locally, sorry.
Attachment #8428230 - Attachment is obsolete: true
Attachment #8428230 - Flags: review?(adw)
Attachment #8428273 - Flags: review?(adw)
Whiteboard: p=5 [diamond] → [diamond] p=5 [qa?]
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 <ttaubert@mozilla.com>
>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
>
Attachment #8428243 - Flags: review?(bugs) → review+
oops, sorry about posting the whole patch.
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
Attachment #8428139 - Flags: review?(bugs) → review-
Attachment #8428155 - Flags: review?(bugs) → review+
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.)
Attachment #8428417 - Flags: review?(bugs) → review+
(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.
Attachment #8428139 - Attachment is obsolete: true
Attachment #8428798 - Flags: review?(bugs)
Added another test for multiple frame loaders and multiple groups. Carrying over r=smaug.
Attachment #8428417 - Attachment is obsolete: true
Attachment #8428811 - Flags: review+
Attachment #8428798 - Flags: review?(bugs) → review+
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!
Attachment #8428126 - Flags: review?(yzenevich) → review+
Landed the core changes (everything but part 4) to not let it bitrot.
Keywords: leave-open
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.
Attachment #8428273 - Flags: review?(adw) → review+
(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.
Updates the nsIMessageManager documentation.
Attachment #8429482 - Flags: review?(bugs)
Comment on attachment 8429482 [details] [diff] [review]
0007-Bug-990812-Fix-nsIMessageManager-documentation.patch

Thanks.
Attachment #8429482 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/c17e7e3cd729
https://hg.mozilla.org/mozilla-central/rev/f35d007a22a7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Added to Iteration 32.3
Whiteboard: [diamond] p=5 [qa+] → [diamond] p=5 s=it-32c-31a-30b.3 [qa+]
Are any of your recent tab-related changes responsible for tab duplication on tab drag-and-drop with today's nightly build?
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert)
Bug 1016960 already found out what's causing the regression.
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?
Hi Juan, can I QA Contact be assigned to this bug for final verification.
Flags: needinfo?(jbecerra)
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?
Flags: needinfo?(jbecerra) → needinfo?(ttaubert)
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
Flags: needinfo?(ttaubert)
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
Marking as [qa!] since no uplift was done to Beta.
Whiteboard: [diamond] p=5 s=it-32c-31a-30b.3 [qa+] → [diamond] p=5 s=it-32c-31a-30b.3 [qa!]
Depends on: 1018913
See Also: → 1117224
You need to log in before you can comment on or make changes to this bug.