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, a=sledru
authorDave Townsend <dtownsend@oxymoronical.com>
Mon, 29 Dec 2014 17:09:52 -0800
changeset 242841 08f30b223076
parent 242840 44e174f9d843
child 242842 fc494bb31bec
push id4320
push userryanvm@gmail.com
push date2015-01-14 14:48 +0000
treeherdermozilla-beta@06bb4d89e2bf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley, sledru
bugs1094312
milestone36.0
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, a=sledru 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
@@ -1467,16 +1467,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
@@ -13,16 +13,21 @@
     <implementation type="application/javascript" implements="nsIObserver, nsIDOMEventListener, nsIMessageListener">
 
       <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);
@@ -34,45 +39,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>
 
@@ -200,16 +217,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";
@@ -240,20 +259,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");