Bug 1522713 - Don't change node binding to tree when updating remoteness; r=nika
authorKyle Machulis <kyle@nonpolynomial.com>
Thu, 14 Mar 2019 00:52:02 +0000
changeset 521821 0bc302ab1f25
parent 521820 bade3af814f6
child 521822 779dcbea91ce
push id10867
push userdvarga@mozilla.com
push dateThu, 14 Mar 2019 15:20:45 +0000
treeherdermozilla-beta@abad13547875 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnika
bugs1522713
milestone67.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 1522713 - Don't change node binding to tree when updating remoteness; r=nika Since we now have a method on nsFrameLoaderOwner/MozFrameLoaderOwner that can update remoteness, we should no longer need to unbind and rebind browser elements to the tree to change their remoteness attributes. We can just call the method and have the Frameloaders rebuilt in the backend. We're still getting some test breakage in Marionette and browser chrome with this patch. Putting this behind a pref so the fission team can still work with it while the tests are being fixed. Depends on D22790 Differential Revision: https://phabricator.services.mozilla.com/D22791
browser/base/content/tabbrowser.js
toolkit/content/widgets/browser-custom-element.js
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -1666,29 +1666,25 @@ window._gBrowser = {
 
     // We'll be creating a new listener, so destroy the old one.
     listener.destroy();
 
     let oldDroppedLinkHandler = aBrowser.droppedLinkHandler;
     let oldSameProcessAsFrameLoader = aBrowser.sameProcessAsFrameLoader;
     let oldUserTypedValue = aBrowser.userTypedValue;
     let hadStartedLoad = aBrowser.didStartLoadSinceLastUserTyping();
+    let parent = aBrowser.parentNode;
+
+    // Change the "remote" attribute.
 
     // Make sure the browser is destroyed so it unregisters from observer notifications
     aBrowser.destroy();
-
-    // Change the "remote" attribute.
-    let parent = aBrowser.parentNode;
-    aBrowser.remove();
-    if (shouldBeRemote) {
-      aBrowser.setAttribute("remote", "true");
-      aBrowser.setAttribute("remoteType", remoteType);
-    } else {
-      aBrowser.setAttribute("remote", "false");
-      aBrowser.removeAttribute("remoteType");
+    // Only remove the node if we're not rebuilding the frameloader via nsFrameLoaderOwner.
+    if (!Services.prefs.getBoolPref("fission.rebuild_frameloaders_on_remoteness_change", false)) {
+      aBrowser.remove();
     }
 
     if (recordExecution) {
       aBrowser.setAttribute("recordExecution", recordExecution);
 
       // Web Replay middleman processes need the default URL to be loaded in
       // order to set up their rendering state.
       aBrowser.setAttribute("nodefaultsrc", "false");
@@ -1708,17 +1704,40 @@ window._gBrowser = {
     }
 
     if (opener) {
       // Set the opener window on the browser, such that when the frame
       // loader is created the opener is set correctly.
       aBrowser.presetOpenerWindow(opener);
     }
 
-    parent.appendChild(aBrowser);
+    // Note that this block is also affected by the
+    // rebuild_frameloaders_on_remoteness_change pref. If the pref is set to
+    // false, this attribute change is observed by browser-custom-element,
+    // causing browser destroy()/construct() to be run. If the pref is true,
+    // then we update the attributes, we run the construct() call ourselves
+    // after the new frameloader has been created.
+    if (shouldBeRemote) {
+      aBrowser.setAttribute("remote", "true");
+      aBrowser.setAttribute("remoteType", remoteType);
+    } else {
+      aBrowser.setAttribute("remote", "false");
+      aBrowser.removeAttribute("remoteType");
+    }
+
+    if (!Services.prefs.getBoolPref("fission.rebuild_frameloaders_on_remoteness_change", false)) {
+      parent.appendChild(aBrowser);
+    } else {
+      // This call actually switches out our frameloaders. Do this as late as
+      // possible before rebuilding the browser, as we'll need the new browser
+      // state set up completely first.
+      aBrowser.changeRemoteness({ remoteType });
+      // Once we have new frameloaders, this call sets the browser back up.
+      aBrowser.construct();
+    }
 
     aBrowser.userTypedValue = oldUserTypedValue;
     if (hadStartedLoad) {
       aBrowser.urlbarChangeTracker.startedLoad();
     }
 
     aBrowser.droppedLinkHandler = oldDroppedLinkHandler;
 
--- a/toolkit/content/widgets/browser-custom-element.js
+++ b/toolkit/content/widgets/browser-custom-element.js
@@ -22,17 +22,21 @@ class MozBrowser extends MozElementMixin
   static get observedAttributes() {
     return ["remote"];
   }
 
   attributeChangedCallback(name, oldValue, newValue) {
     // When we have already been set up via connectedCallback and the
     // and the [remote] value changes, we need to start over. This used
     // to happen due to a XBL binding change.
-    if (name === "remote" && oldValue != newValue && this.isConnectedAndReady) {
+    //
+    // Only do this when the rebuild frameloaders pref is off. This update isn't
+    // required when we rebuild the frameloaders in the backend.
+    if (!Services.prefs.getBoolPref("fission.rebuild_frameloaders_on_remoteness_change", false) &&
+        name === "remote" && oldValue != newValue && this.isConnectedAndReady) {
       this.destroy();
       this.construct();
     }
   }
 
   constructor() {
     super();