Bug 1504807 - [marionette] Don't remove pending command unless it has been completed. r=ato, a=test-only
authorHenrik Skupin <mail@hskupin.info>
Tue, 06 Nov 2018 12:08:55 +0000
changeset 501065 6900e33d73082d4dd29a9efa1737e1b29af52c52
parent 501064 2dba15f946e31610e6f6c07b647d59184866f5f7
child 501066 5b42d14602aacbaf0aa2bbdcbfb7cf2100cb162e
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato, test-only
bugs1504807
milestone64.0
Bug 1504807 - [marionette] Don't remove pending command unless it has been completed. r=ato, a=test-only With the use of multiple content processes in Firefox a navigation command can cause the active framescript to be moved to a different process. This interrupts the currently executed command, and as such needs to be executed again after the framescript has been finished initializing. Currently flushing the pending commands doesn't take into account that the framescript can even be moved multiple times to a different process during a single page navigation. As such all pending commands are getting removed after the first process move. For navigation commands this means that no page load listeners are attached for subsequent process changes, and navigation commands could never return, and cause a hang of Marionette. To solve the problem the pending commands need to be flushed each time the process changes. They will remove themselves from the list once they have finished processing. Depends on D10998 Differential Revision: https://phabricator.services.mozilla.com/D10999
testing/marionette/browser.js
testing/marionette/driver.js
testing/marionette/listener.js
--- a/testing/marionette/browser.js
+++ b/testing/marionette/browser.js
@@ -136,21 +136,21 @@ browser.Context = class {
     // 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.
+    // Commands which trigger a navigation can cause the frame script to be
+    // moved to a different process. 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;
 
     this.frameRegsPending = 0;
 
     this.getIdForBrowser = driver.getIdForBrowser.bind(driver);
     this.updateIdForBrowser = driver.updateIdForBrowser.bind(driver);
   }
@@ -425,37 +425,38 @@ browser.Context = class {
       }
     }
 
     // used to delete sessions
     this.knownFrames.push(uid);
   }
 
   /**
-   * Flushes any queued pending commands after a reload of the frame script.
+   * Flush any queued pending commands.
+   *
+   * Needs to be run after a process change for the frame script.
    */
   flushPendingCommands() {
     if (!this._needsFlushPendingCommands) {
       return;
     }
 
     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.
+    * the new listener script is loaded and registered itself.
     * This occurs when a command whose effect is asynchronous (such
-    * as goBack) results in a reload of the frame script and new commands
-    * are subsequently posted to the server.
+    * as goBack) results in process change of the frame script and new
+    * commands are subsequently posted to the server.
     */
   executeWhenReady(cb) {
     if (this._needsFlushPendingCommands) {
       this.pendingCommands.push(cb);
     } else {
       cb();
     }
   }
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -1059,18 +1059,18 @@ GeckoDriver.prototype.get = async functi
   assert.content(this.context);
   assert.open(this.getCurrentWindow());
   await this._handleUserPrompts();
 
   let url = cmd.parameters.url;
 
   let get = this.listener.get({url, 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
+  // If a process change 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
       commandID: this.listener.activeMessageId,
       pageTimeout: this.timeouts.pageLoad,
       startTime: new Date().getTime(),
     };
@@ -1178,18 +1178,18 @@ GeckoDriver.prototype.goBack = async fun
   // 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 reload of the frame script interrupts our page load, this will
-  // never return. We need to re-issue this request to correctly poll for
+  // If a process change 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
       commandID: this.listener.activeMessageId,
       lastSeenURL: lastURL.toString(),
       pageTimeout: this.timeouts.pageLoad,
       startTime: new Date().getTime(),
@@ -1221,18 +1221,18 @@ GeckoDriver.prototype.goForward = async 
   if (!this.curBrowser.contentBrowser.webNavigation.canGoForward) {
     return;
   }
 
   let lastURL = this.currentURL;
   let goForward = this.listener.goForward(
       {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
+  // If a process change 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
       commandID: this.listener.activeMessageId,
       lastSeenURL: lastURL.toString(),
       pageTimeout: this.timeouts.pageLoad,
       startTime: new Date().getTime(),
@@ -1258,18 +1258,18 @@ GeckoDriver.prototype.goForward = async 
 GeckoDriver.prototype.refresh = async function() {
   assert.content(this.context);
   assert.open(this.getCurrentWindow());
   await this._handleUserPrompts();
 
   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
+  // If a process change 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
       commandID: this.listener.activeMessageId,
       pageTimeout: this.timeouts.pageLoad,
       startTime: new Date().getTime(),
     };
@@ -2140,19 +2140,19 @@ GeckoDriver.prototype.clickElement = asy
       let el = this.curBrowser.seenEls.get(webEl);
       await interaction.clickElement(el, this.a11yChecks);
       break;
 
     case Context.Content:
       let click = this.listener.clickElement(
           {webElRef: webEl.toJSON(), 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.
+      // If a process change 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
           commandID: this.listener.activeMessageId,
           pageTimeout: this.timeouts.pageLoad,
           startTime: new Date().getTime(),
         };
         this.curBrowser.messageManager.sendAsyncMessage(
--- a/testing/marionette/listener.js
+++ b/testing/marionette/listener.js
@@ -86,18 +86,18 @@ let multiLast = {};
 const sandboxes = new Sandboxes(() => curContainer.frame);
 
 const eventObservers = new ContentEventObserverService(
     content, sendAsyncMessage.bind(this));
 
 /**
  * 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.
+ * to happen. In the specific case of a process change of the frame script it
+ * allows to continue observing the current page load.
  */
 const loadListener = {
   commandID: null,
   seenBeforeUnload: false,
   seenUnload: false,
   timeout: null,
   timerPageLoad: null,
   timerPageUnload: null,
@@ -123,17 +123,18 @@ const loadListener = {
 
     this.seenBeforeUnload = false;
     this.seenUnload = false;
 
     this.timerPageLoad = Cc["@mozilla.org/timer;1"]
         .createInstance(Ci.nsITimer);
     this.timerPageUnload = null;
 
-    // In case the frame script has been reloaded, wait the remaining time
+    // In case the frame script has been moved to a differnt process,
+    // wait the remaining time
     timeout = startTime + timeout - new Date().getTime();
 
     if (timeout <= 0) {
       this.notify(this.timerPageLoad);
       return;
     }
 
     if (waitForUnloaded) {
@@ -141,17 +142,17 @@ const loadListener = {
       addEventListener("hashchange", this, true);
       addEventListener("pagehide", this, true);
       addEventListener("popstate", this, true);
       addEventListener("unload", this, true);
 
       Services.obs.addObserver(this, "outer-window-destroyed");
 
     } else {
-      // The frame script got reloaded due to a new content process.
+      // The frame script has been moved to a differnt content process.
       // Due to the time it takes to re-register the browser in Marionette,
       // it can happen that page load events are missed before the listeners
       // are getting attached again. By checking the document readyState the
       // command can return immediately if the page load is already done.
       let readyState = content.document.readyState;
       let documentURI = content.document.documentURI;
       logger.debug(truncate`Check readyState ${readyState} for ${documentURI}`);
       // If the page load has already finished, don't setup listeners and
@@ -183,18 +184,19 @@ const loadListener = {
     removeEventListener("beforeunload", this, true);
     removeEventListener("hashchange", this, true);
     removeEventListener("pagehide", this, true);
     removeEventListener("popstate", this, true);
     removeEventListener("DOMContentLoaded", this, true);
     removeEventListener("pageshow", this, true);
     removeEventListener("unload", this, true);
 
-    // In case the observer was added before the frame script has been
-    // reloaded, it will no longer be available. Exceptions can be ignored.
+    // In case the observer was added before the frame script has been moved
+    // to a different process, 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.
    */
@@ -355,17 +357,17 @@ const loadListener = {
           sendOk(this.commandID);
         }
         break;
     }
   },
 
   /**
    * Continue to listen for page load events after the frame script has been
-   * reloaded.
+   * moved to a different content process.
    *
    * @param {number} commandID
    *     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
@@ -987,28 +989,28 @@ function multiAction(args, maxLen) {
  * 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 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.
+ * to resume one when the frame script has been moved to a different content
+ * process 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} commandID
  *     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.
+ *     Unix timestap when the navitation request got triggered.
  */
 function waitForPageLoaded(msg) {
   let {commandID, pageTimeout, startTime} = msg.json;
   loadListener.waitForLoadAfterFramescriptReload(
       commandID, pageTimeout, startTime);
 }
 
 /**