Bug 1332122 - Re-register the browser for frame script reloads. draft
authorHenrik Skupin <mail@hskupin.info>
Thu, 06 Jul 2017 18:02:19 +0200
changeset 604891 51dceb7d83fa029fa21fbe6676f3e7772ee14c3b
parent 604647 0ba79dd1bafde54e1f21fb24dff22de46d84bbf7
child 604892 13c95b16a634dbd1020412daae01e34825489770
push id67218
push userbmo:hskupin@gmail.com
push dateThu, 06 Jul 2017 16:38:41 +0000
bugs1332122
milestone56.0a1
Bug 1332122 - Re-register the browser for frame script reloads. With multi-processes for content reloads of the frame script can happen at any time, and not only for remoteness changes. As such the current browser has to be re-registered with the new id of the listener. MozReview-Commit-ID: 48MOZfuPTR9
testing/marionette/browser.js
testing/marionette/driver.js
testing/marionette/listener.js
--- a/testing/marionette/browser.js
+++ b/testing/marionette/browser.js
@@ -73,18 +73,18 @@ browser.getTabBrowser = function(win) {
  * @param {nsIDOMWindow} win
  *     The window whose browser needs to be accessed.
  * @param {GeckoDriver} driver
  *     Reference to the driver the browser is attached to.
  */
 browser.Context = class {
 
   /**
-   * @param {<xul:browser>} win
-   *     Frame that is expected to contain the view of the web document.
+   * @param {ChromeWindow} win
+   *     ChromeWindow that contains the top-level browsing context.
    * @param {GeckoDriver} driver
    *     Reference to driver instance.
    */
   constructor(win, driver) {
     this.window = win;
     this.driver = driver;
 
     // In Firefox this is <xul:tabbrowser> (not <xul:browser>!)
@@ -101,29 +101,34 @@ browser.Context = class {
     // A reference to the tab corresponding to the current window handle,
     // if any.  Specifically, this.tab refers to the last tab that Marionette
     // switched to in this browser window. Note that this may not equal the
     // currently selected tab.  For example, if Marionette switches to tab
     // A, and then clicks on a button that opens a new tab B in the same
     // browser window, this.tab will still point to tab A, despite tab B
     // being the currently selected tab.
     this.tab = null;
+
+    // Commands which trigger a page load can cause the frame script to be
+    // reloaded. To not loose the currently active command, or any other
+    // already pushed following command, store them as long as they haven't
+    // been fully processed. The commands get flushed after a new browser
+    // has been registered.
     this.pendingCommands = [];
+    this._needsFlushPendingCommands = false;
 
     // We should have one frame.Manager per browser.Context so that we
     // can handle modals in each <xul:browser>.
     this.frameManager = new frame.Manager(driver);
     this.frameRegsPending = 0;
 
     // register all message listeners
     this.frameManager.addMessageManagerListeners(driver.mm);
     this.getIdForBrowser = driver.getIdForBrowser.bind(driver);
     this.updateIdForBrowser = driver.updateIdForBrowser.bind(driver);
-    this._browserWasRemote = null;
-    this._hasRemotenessChange = false;
   }
 
   /**
    * Returns the content browser for the currently selected tab.
    * If there is no tab selected, null will be returned.
    */
   get contentBrowser() {
     if (this.tab) {
@@ -281,18 +286,17 @@ browser.Context = class {
    * @param {string} uri
    *      URI to open.
    */
   addTab(uri) {
     return this.tabBrowser.addTab(uri, true);
   }
 
   /**
-   * Set the current tab and update remoteness tracking if a tabbrowser
-   * is available.
+   * Set the current tab.
    *
    * @param {number=} index
    *     Tab index to switch to. If the parameter is undefined,
    *     the currently selected tab will be used.
    * @param {nsIDOMWindow=} win
    *     Switch to this window before selecting the tab.
    * @param {boolean=} focus
    *      A boolean value which determins whether to focus
@@ -325,103 +329,71 @@ browser.Context = class {
           // Firefox
           this.tabBrowser.selectedTab = this.tab;
 
         } else {
           throw new UnsupportedOperationError("switchToTab() not supported");
         }
       }
     }
-
-    if (this.driver.appName == "Firefox") {
-      this._browserWasRemote = this.contentBrowser.isRemoteBrowser;
-      this._hasRemotenessChange = false;
-    }
   }
 
   /**
    * Registers a new frame, and sets its current frame id to this frame
    * if it is not already assigned, and if a) we already have a session
    * or b) we're starting a new session and it is the right start frame.
    *
    * @param {string} uid
    *     Frame uid for use by Marionette.
-   * @param the XUL <browser> that was the target of the originating message.
+   * @param {xul:browser} target
+   *     The <xul:browser> that was the target of the originating message.
    */
   register(uid, target) {
-    let remotenessChange = this.hasRemotenessChange();
-    if (this.curFrameId === null || remotenessChange) {
-      if (this.tabBrowser) {
-        // If we're setting up a new session on Firefox, we only process the
-        // registration for this frame if it belongs to the current tab.
-        if (!this.tab) {
-          this.switchToTab();
-        }
+    if (this.tabBrowser) {
+      // If we're setting up a new session on Firefox, we only process the
+      // registration for this frame if it belongs to the current tab.
+      if (!this.tab) {
+        this.switchToTab();
+      }
 
-        if (target === this.contentBrowser) {
-          this.updateIdForBrowser(this.contentBrowser, uid);
-        }
+      if (target === this.contentBrowser) {
+        this.updateIdForBrowser(this.contentBrowser, uid);
+        this._needsFlushPendingCommands = true;
       }
     }
 
     // used to delete sessions
     this.knownFrames.push(uid);
-    return remotenessChange;
   }
 
   /**
-   * When navigating between pages results in changing a browser's
-   * process, we need to take measures not to lose contact with a listener
-   * script. This function does the necessary bookkeeping.
-   */
-  hasRemotenessChange() {
-    // None of these checks are relevant if we don't have a tab yet,
-    // and may not apply on Fennec.
-    if (this.driver.appName != "Firefox" ||
-        this.tab === null ||
-        this.contentBrowser === null) {
-      return false;
-    }
-
-    if (this._hasRemotenessChange) {
-      return true;
-    }
-
-    let currentIsRemote = this.contentBrowser.isRemoteBrowser;
-    this._hasRemotenessChange = this._browserWasRemote !== currentIsRemote;
-    this._browserWasRemote = currentIsRemote;
-    return this._hasRemotenessChange;
-  }
-
-  /**
-   * Flushes any pending commands queued when a remoteness change is being
-   * processed and mark this remotenessUpdate as complete.
+   * Flushes any queued pending commands after a reload of the frame script.
    */
   flushPendingCommands() {
-    if (!this._hasRemotenessChange) {
+    if (!this._needsFlushPendingCommands) {
       return;
     }
 
-    this._hasRemotenessChange = false;
     this.pendingCommands.forEach(cb => cb());
     this.pendingCommands = [];
+    this._needsFlushPendingCommands = false;
   }
 
   /**
     * This function intercepts commands interacting with content and queues
     * or executes them as needed.
     *
     * No commands interacting with content are safe to process until
     * the new listener script is loaded and registers itself.
     * This occurs when a command whose effect is asynchronous (such
-    * as goBack) results in a remoteness change and new commands
+    * as goBack) results in a reload of the frame script and new commands
     * are subsequently posted to the server.
     */
   executeWhenReady(cb) {
-    if (this.hasRemotenessChange()) {
+    if (this._needsFlushPendingCommands) {
       this.pendingCommands.push(cb);
     } else {
       cb();
     }
   }
 
 };
 
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -566,42 +566,39 @@ GeckoDriver.prototype.registerBrowser = 
     // Marionette:switchToFrame, so we'll acknowledge the switchToFrame
     // message here.
     //
     // TODO: Should have a better way of determining that this message
     // is from a remote frame.
     this.curBrowser.frameManager.currentRemoteFrame.targetFrameId = id;
   }
 
-  let reg = {};
-
   // We want to ignore frames that are XUL browsers that aren't in the "main"
   // tabbrowser, but accept things on Fennec (which doesn't have a
   // xul:tabbrowser), and accept HTML iframes (because tests depend on it),
   // as well as XUL frames. Ideally this should be cleaned up and we should
   // keep track of browsers a different way.
   if (this.appName != "Firefox" || be.namespaceURI != XUL_NS ||
       be.nodeName != "browser" || be.getTabBrowser()) {
     // curBrowser holds all the registered frames in knownFrames
-    reg.id = id;
-    reg.remotenessChange = this.curBrowser.register(id, be);
+    this.curBrowser.register(id, be);
   }
 
-  this.wins.set(reg.id, listenerWindow);
+  this.wins.set(id, listenerWindow);
   if (nullPrevious && (this.curBrowser.curFrameId !== null)) {
     this.sendAsync(
         "newSession",
         this.capabilities.toJSON(),
         this.newSessionCommandId);
     if (this.curBrowser.isNewSession) {
       this.newSessionCommandId = null;
     }
   }
 
-  return [reg, this.capabilities.toJSON()];
+  return [id, this.capabilities.toJSON()];
 };
 
 GeckoDriver.prototype.registerPromise = function() {
   const li = "Marionette:register";
 
   return new Promise(resolve => {
     let cb = msg => {
       let wid = msg.json.value;
@@ -621,20 +618,23 @@ GeckoDriver.prototype.registerPromise = 
       return rv;
     };
     this.mm.addMessageListener(li, cb);
   });
 };
 
 GeckoDriver.prototype.listeningPromise = function() {
   const li = "Marionette:listenersAttached";
+
   return new Promise(resolve => {
-    let cb = () => {
-      this.mm.removeMessageListener(li, cb);
-      resolve();
+    let cb = msg => {
+      if (msg.json.listenerId === this.curBrowser.curFrameId) {
+        this.mm.removeMessageListener(li, cb);
+        resolve();
+      }
     };
     this.mm.addMessageListener(li, cb);
   });
 };
 
 /** Create a new session. */
 GeckoDriver.prototype.newSession = function* (cmd, resp) {
   if (this.sessionId) {
@@ -973,19 +973,19 @@ GeckoDriver.prototype.get = function* (c
   assert.content(this.context);
   assert.window(this.getCurrentWindow());
   assert.noUserPrompt(this.dialog);
 
   let url = cmd.parameters.url;
 
   let get = this.listener.get({url, pageTimeout: this.timeouts.pageLoad});
 
-  // If a remoteness update interrupts our page load, this will never return
-  // We need to re-issue this request to correctly poll for readyState and
-  // send errors.
+  // If a reload of the frame script interrupts our page load, this will
+  // never return. We need to re-issue this request to correctly poll for
+  // readyState and send errors.
   this.curBrowser.pendingCommands.push(() => {
     let parameters = {
       // TODO(ato): Bug 1242595
       command_id: this.listener.activeMessageId,
       pageTimeout: this.timeouts.pageLoad,
       startTime: new Date().getTime(),
     };
     this.mm.broadcastAsyncMessage(
@@ -1092,19 +1092,19 @@ GeckoDriver.prototype.goBack = function*
   // If there is no history, just return
   if (!this.curBrowser.contentBrowser.webNavigation.canGoBack) {
     return;
   }
 
   let lastURL = this.currentURL;
   let goBack = this.listener.goBack({pageTimeout: this.timeouts.pageLoad});
 
-  // If a remoteness update interrupts our page load, this will never return
-  // We need to re-issue this request to correctly poll for readyState and
-  // send errors.
+  // If a reload of the frame script interrupts our page load, this will
+  // never return. We need to re-issue this request to correctly poll for
+  // readyState and send errors.
   this.curBrowser.pendingCommands.push(() => {
     let parameters = {
       // TODO(ato): Bug 1242595
       command_id: this.listener.activeMessageId,
       lastSeenURL: lastURL.toString(),
       pageTimeout: this.timeouts.pageLoad,
       startTime: new Date().getTime(),
     };
@@ -1136,19 +1136,19 @@ GeckoDriver.prototype.goForward = functi
   if (!this.curBrowser.contentBrowser.webNavigation.canGoForward) {
     return;
   }
 
   let lastURL = this.currentURL;
   let goForward = this.listener.goForward(
       {pageTimeout: this.timeouts.pageLoad});
 
-  // If a remoteness update interrupts our page load, this will never return
-  // We need to re-issue this request to correctly poll for readyState and
-  // send errors.
+  // If a reload of the frame script interrupts our page load, this will
+  // never return. We need to re-issue this request to correctly poll for
+  // readyState and send errors.
   this.curBrowser.pendingCommands.push(() => {
     let parameters = {
       // TODO(ato): Bug 1242595
       command_id: this.listener.activeMessageId,
       lastSeenURL: lastURL.toString(),
       pageTimeout: this.timeouts.pageLoad,
       startTime: new Date().getTime(),
     };
@@ -1171,17 +1171,35 @@ GeckoDriver.prototype.goForward = functi
  * @throws {UnexpectedAlertOpenError}
  *     A modal dialog is open, blocking this operation.
  */
 GeckoDriver.prototype.refresh = function* (cmd, resp) {
   assert.content(this.context);
   assert.window(this.getCurrentWindow());
   assert.noUserPrompt(this.dialog);
 
-  yield this.listener.refresh({pageTimeout: this.timeouts.pageLoad});
+  let refresh = this.listener.refresh(
+      {pageTimeout: this.timeouts.pageLoad})
+
+  // If a reload of the frame script interrupts our page load, this will
+  // never return. We need to re-issue this request to correctly poll for
+  // readyState and send errors.
+  this.curBrowser.pendingCommands.push(() => {
+    let parameters = {
+      // TODO(ato): Bug 1242595
+      command_id: this.listener.activeMessageId,
+      pageTimeout: this.timeouts.pageLoad,
+      startTime: new Date().getTime(),
+    };
+    this.mm.broadcastAsyncMessage(
+        "Marionette:waitForPageLoaded" + this.curBrowser.curFrameId,
+        parameters);
+  });
+
+  yield refresh;
 };
 
 /**
  * Forces an update for the given browser's id.
  */
 GeckoDriver.prototype.updateIdForBrowser = function(browser, newId) {
   this._browserIds.set(browser.permanentKey, newId);
 };
@@ -2064,19 +2082,19 @@ GeckoDriver.prototype.clickElement = fun
       // so we need to listen for it and then just send an error back.
       // The person making the call should be aware something is not right
       // and handle accordingly.
       this.addFrameCloseListener("click");
 
       let click = this.listener.clickElement(
           {id, pageTimeout: this.timeouts.pageLoad});
 
-      // If a remoteness update interrupts our page load, this will
-      // never return We need to re-issue this request to correctly poll
-      // for readyState and send errors.
+      // If a reload of the frame script interrupts our page load, this will
+      // never return. We need to re-issue this request to correctly poll for
+      // readyState and send errors.
       this.curBrowser.pendingCommands.push(() => {
         let parameters = {
           // TODO(ato): Bug 1242595
           command_id: this.listener.activeMessageId,
           pageTimeout: this.timeouts.pageLoad,
           startTime: new Date().getTime(),
         };
         this.mm.broadcastAsyncMessage(
@@ -3173,19 +3191,19 @@ GeckoDriver.prototype.receiveMessage = f
     case "Marionette:register":
       let wid = message.json.value;
       let be = message.target;
       let rv = this.registerBrowser(wid, be);
       return rv;
 
     case "Marionette:listenersAttached":
       if (message.json.listenerId === this.curBrowser.curFrameId) {
-        // If remoteness gets updated we need to call newSession. In the case
-        // of desktop this just sets up a small amount of state that doesn't
-        // change over the course of a session.
+        // If the frame script gets reloaded we need to call newSession.
+        // In the case of desktop this just sets up a small amount of state
+        // that doesn't change over the course of a session.
         this.sendAsync("newSession", this.capabilities.toJSON());
         this.curBrowser.flushPendingCommands();
       }
       break;
   }
 };
 /* eslint-enable consistent-return */
 
--- a/testing/marionette/listener.js
+++ b/testing/marionette/listener.js
@@ -49,17 +49,16 @@ Cu.import("chrome://marionette/content/s
 
 Cu.importGlobalProperties(["URL"]);
 
 var marionetteTestName;
 var winUtil = content.QueryInterface(Ci.nsIInterfaceRequestor)
     .getInterface(Ci.nsIDOMWindowUtils);
 var listenerId = null; // unique ID of this listener
 var curContainer = {frame: content, shadowRoot: null};
-var isRemoteBrowser = () => curContainer.frame.contentWindow !== null;
 var previousContainer = null;
 
 var seenEls = new element.Store();
 var SUPPORTED_STRATEGIES = new Set([
   element.Strategy.ClassName,
   element.Strategy.Selector,
   element.Strategy.ID,
   element.Strategy.Name,
@@ -113,20 +112,20 @@ var modalHandler = function() {
   curContainer = {frame: content, shadowRoot: null};
 };
 
 // sandbox storage and name of the current sandbox
 var sandboxes = new Sandboxes(() => curContainer.frame);
 var sandboxName = "default";
 
 /**
- * The load listener singleton helps to keep track of active page
- * load activities, and can be used by any command which might cause a
- * navigation to happen.  In the specific case of remoteness changes it
- * allows to continue observing the current page load.
+ * The load listener singleton helps to keep track of active page load
+ * activities, and can be used by any command which might cause a navigation
+ * to happen. In the specific case of a reload of the frame script it allows
+ * to continue observing the current page load.
  */
 var loadListener = {
   command_id: null,
   seenBeforeUnload: false,
   seenUnload: false,
   timeout: null,
   timerPageLoad: null,
   timerPageUnload: null,
@@ -152,17 +151,17 @@ var loadListener = {
 
     this.seenBeforeUnload = false;
     this.seenUnload = false;
 
     this.timerPageLoad = Cc["@mozilla.org/timer;1"]
         .createInstance(Ci.nsITimer);
     this.timerPageUnload = null;
 
-    // In case of a remoteness change, only wait the remaining time
+    // In case the frame script has been reloaded, wait the remaining time
     timeout = startTime + timeout - new Date().getTime();
 
     if (timeout <= 0) {
       this.notify(this.timerPageLoad);
       return;
     }
 
     if (waitForUnloaded) {
@@ -207,18 +206,18 @@ var loadListener = {
       curContainer.frame.removeEventListener("beforeunload", this);
       curContainer.frame.removeEventListener("unload", this);
     } catch (e) {
       if (e.name != "TypeError") {
         throw e;
       }
     }
 
-    // In the case when the observer was added before a remoteness change,
-    // it will no longer be available. Exceptions can be silently ignored.
+    // In case the observer was added before the frame script has been
+    // reloaded, it will no longer be available. Exceptions can be ignored.
     try {
       Services.obs.removeObserver(this, "outer-window-destroyed");
     } catch (e) {}
   },
 
   /**
    * Callback for registered DOM events.
    */
@@ -339,29 +338,29 @@ var loadListener = {
           this.stop();
           sendOk(this.command_id);
         }
         break;
     }
   },
 
   /**
-   * Continue to listen for page load events after a remoteness change
-   * happened.
+   * Continue to listen for page load events after the frame script has been
+   * reloaded.
    *
    * @param {number} command_id
    *     ID of the currently handled message between the driver and
    *     listener.
    * @param {number} timeout
    *     Timeout in milliseconds the method has to wait for the page
    *     being finished loading.
    * @param {number} startTime
    *     Unix timestap when the navitation request got triggered.
    */
-  waitForLoadAfterRemotenessChange(command_id, timeout, startTime) {
+  waitForLoadAfterFramescriptReload(command_id, timeout, startTime) {
     this.start(command_id, timeout, startTime, false);
   },
 
   /**
    * Use a trigger callback to initiate a page load, and attach listeners if
    * a page load is expected.
    *
    * @param {function} trigger
@@ -425,28 +424,23 @@ var loadListener = {
  */
 function registerSelf() {
   let msg = {value: winUtil.outerWindowID};
   logger.debug(`Register listener.js for window ${msg.value}`);
 
   // register will have the ID and a boolean describing if this is the
   // main process or not
   let register = sendSyncMessage("Marionette:register", msg);
-
   if (register[0]) {
-    let {id, remotenessChange} = register[0][0];
+    listenerId = register[0][0];
     capabilities = session.Capabilities.fromJSON(register[0][1]);
-    listenerId = id;
-    if (typeof id != "undefined") {
+    if (typeof listenerId != "undefined") {
       startListeners();
-      let rv = {};
-      if (remotenessChange) {
-        rv.listenerId = id;
-      }
-      sendAsyncMessage("Marionette:listenersAttached", rv);
+      sendAsyncMessage("Marionette:listenersAttached",
+          {"listenerId": listenerId});
     }
   }
 }
 
 // Eventually we will not have a closure for every single command, but
 // use a generic dispatch for all listener commands.
 //
 // Perhaps one could even conceive having a separate instance of
@@ -1100,34 +1094,34 @@ function multiAction(args, maxLen) {
  * current navigation request in case we're interupted by an onbeforeunload
  * handler and navigation doesn't complete.
  */
 function cancelRequest() {
   loadListener.stop();
 }
 
 /**
- * This implements the latter part of a get request (for the case we
- * need to resume one when a remoteness update happens in the middle of a
+ * This implements the latter part of a get request (for the case we need
+ * to resume one when the frame script has been reloaded in the middle of a
  * navigate request). This is most of of the work of a navigate request,
  * but doesn't assume DOMContentLoaded is yet to fire.
  *
  * @param {number} command_id
  *     ID of the currently handled message between the driver and
  *     listener.
  * @param {number} pageTimeout
  *     Timeout in seconds the method has to wait for the page being
  *     finished loading.
  * @param {number} startTime
  *     Unix timestap when the navitation request got triggred.
  */
 function waitForPageLoaded(msg) {
   let {command_id, pageTimeout, startTime} = msg.json;
 
-  loadListener.waitForLoadAfterRemotenessChange(
+  loadListener.waitForLoadAfterFramescriptReload(
       command_id, pageTimeout, startTime);
 }
 
 /**
  * Navigate to the given URL.  The operation will be performed on the
  * current browsing context, which means it handles the case where we
  * navigate within an iframe.  All other navigation is handled by the driver
  * (in chrome space).