Bug 1094312: Properly destroy browsers when switching between remote and non-remove pages and override the default destroy method in remote-browser.xml. r=mconley
authorDave Townsend <dtownsend@oxymoronical.com>
Mon, 29 Dec 2014 17:09:52 -0800
changeset 248730 be0ad231ea19afc28da6aa0cde8aa4f949b569db
parent 248729 0d113455b05c375db4e77cb2bef6fc87a402316a
child 248731 a24a887963b79da377bfe0cd735468d21d87f8d4
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley
bugs1094312
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1094312: Properly destroy browsers when switching between remote and non-remove pages and override the default destroy method in remote-browser.xml. r=mconley In some cases removing an element from a document doesn't call its XBL constructor. tabbrowser knows this and so calls a destroy method on browser elements when removing them. When remote-browser was added we forgot to add a specific destroy method so the base browser binding's would be used. This would mean remote-browsers aren't being destroyed correctly. Also when we switch from remote to non-remote browsers or vice versa we don't call the destroy method at all so sometimes the browser isn't destroyed properly and observer notifications get fired on dead browser elements.
browser/base/content/tabbrowser.xml
toolkit/content/widgets/remote-browser.xml
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -1461,16 +1461,18 @@
 
             let wasActive = document.activeElement == aBrowser;
 
             // Unhook our progress listener.
             let tab = this.getTabForBrowser(aBrowser);
             let index = tab._tPos;
             let filter = this.mTabFilters[index];
             aBrowser.webProgress.removeProgressListener(filter);
+            // Make sure the browser is destroyed so it unregisters from observer notifications
+            aBrowser.destroy();
 
             // Change the "remote" attribute.
             let parent = aBrowser.parentNode;
             let permanentKey = aBrowser.permanentKey;
             parent.removeChild(aBrowser);
             aBrowser.setAttribute("remote", aShouldBeRemote ? "true" : "false");
             // Tearing down the browser gives a new permanentKey but we want to
             // keep the old one. Re-set it explicitly after unbinding from DOM.
--- a/toolkit/content/widgets/remote-browser.xml
+++ b/toolkit/content/widgets/remote-browser.xml
@@ -14,16 +14,21 @@
                     implements="nsIObserver, nsIDOMEventListener, nsIMessageListener, nsIRemoteBrowser">
 
       <field name="_securityUI">null</field>
 
       <property name="securityUI"
                 readonly="true">
         <getter><![CDATA[
           if (!this._securityUI) {
+            // Don't attempt to create the remote web progress if the
+            // messageManager has already gone away
+            if (!this.messageManager)
+              return null;
+
             let jsm = "resource://gre/modules/RemoteSecurityUI.jsm";
             let RemoteSecurityUI = Components.utils.import(jsm, {}).RemoteSecurityUI;
             this._securityUI = new RemoteSecurityUI();
           }
 
           // We want to double-wrap the JS implemented interface, so that QI and instanceof works.
           var ptr = Cc["@mozilla.org/supports-interface-pointer;1"].
                         createInstance(Ci.nsISupportsInterfacePointer);
@@ -35,45 +40,57 @@
       <method name="adjustPriority">
         <parameter name="adjustment"/>
         <body><![CDATA[
           this.messageManager.sendAsyncMessage("NetworkPrioritizer:AdjustPriority",
                                                {adjustment: adjustment});
         ]]></body>
       </method>
 
+      <field name="_controller">null</field>
+
       <field name="_remoteWebNavigation">null</field>
 
       <property name="webNavigation"
                 onget="return this._remoteWebNavigation;"
                 readonly="true"/>
 
       <field name="_remoteWebProgress">null</field>
 
       <property name="webProgress" readonly="true">
       	<getter>
       	  <![CDATA[
             if (!this._remoteWebProgress) {
+              // Don't attempt to create the remote web progress if the
+              // messageManager has already gone away
+              if (!this.messageManager)
+                return null;
+
               let jsm = "resource://gre/modules/RemoteWebProgress.jsm";
-              let RemoteWebProgressManager = Cu.import(jsm, {}).RemoteWebProgressManager;
+              let { RemoteWebProgressManager } = Cu.import(jsm, {});
               this._remoteWebProgressManager = new RemoteWebProgressManager(this);
               this._remoteWebProgress = this._remoteWebProgressManager.topLevelWebProgress;
             }
             return this._remoteWebProgress;
       	  ]]>
       	</getter>
       </property>
 
       <field name="_remoteFinder">null</field>
 
       <property name="finder" readonly="true">
         <getter><![CDATA[
           if (!this._remoteFinder) {
+            // Don't attempt to create the remote web progress if the
+            // messageManager has already gone away
+            if (!this.messageManager)
+              return null;
+
             let jsm = "resource://gre/modules/RemoteFinder.jsm";
-            let RemoteFinder = Cu.import(jsm, {}).RemoteFinder;
+            let { RemoteFinder } = Cu.import(jsm, {});
             this._remoteFinder = new RemoteFinder(this);
           }
           return this._remoteFinder;
         ]]></getter>
       </property>
 
       <field name="_documentURI">null</field>
 
@@ -201,16 +218,18 @@
           <![CDATA[
             let {frameLoader} = this.QueryInterface(Ci.nsIFrameLoaderOwner);
             frameLoader.tabParent.setIsDocShellActive(val);
             return val;
           ]]>
         </setter>
       </property>
 
+      <field name="mDestroyed">false</field>
+
       <constructor>
         <![CDATA[
           /*
            * Don't try to send messages from this function. The message manager for
            * the <browser> element may not be initialized yet.
            */
 
           let jsm = "resource://gre/modules/RemoteWebNavigation.jsm";
@@ -241,20 +260,34 @@
           this.controllers.appendController(this._controller);
 
           Services.obs.addObserver(this, "ask-children-to-exit-fullscreen", false);
         ]]>
       </constructor>
 
       <destructor>
         <![CDATA[
-          Services.obs.removeObserver(this, "ask-children-to-exit-fullscreen");
+          this.destroy();
         ]]>
       </destructor>
 
+      <!-- This is necessary because the destructor doesn't always get called when
+           we are removed from a tabbrowser. This will be explicitly called by tabbrowser -->
+      <method name="destroy">
+        <body><![CDATA[
+          if (this.mDestroyed)
+            return;
+          this.mDestroyed = true;
+
+          this.controllers.removeController(this._controller);
+
+          Services.obs.removeObserver(this, "ask-children-to-exit-fullscreen");
+        ]]></body>
+      </method>
+
       <method name="receiveMessage">
         <parameter name="aMessage"/>
         <body><![CDATA[
           let data = aMessage.data;
           switch (aMessage.name) {
             case "Browser:Init":
               let result = {};
               result.useGlobalHistory = !this.hasAttribute("disableglobalhistory");