Commit dab04b17 authored by Henrik Skupin's avatar Henrik Skupin
Browse files

Bug 1504807 - [marionette] Don't remove pending command unless it has been completed. r=ato 

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

--HG--
extra : moz-landing-system : lando
parent 2e1c43e3

testing/marionette/browser.js

+11 −10
Original line number Diff line number Diff line
@@ -141,11 +141,11 @@ browser.Context = class { 
    // 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;
@@ -430,7 +430,9 @@ browser.Context = class { 
  }



  /**

   * 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) {
@@ -438,7 +440,6 @@ browser.Context = class { 
    }



    this.pendingCommands.forEach(cb => cb());

    this.pendingCommands = [];

    this._needsFlushPendingCommands = false;

  }
@@ -447,10 +448,10 @@ browser.Context = class { 
    * 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) {

testing/marionette/driver.js

+11 −11
Original line number Diff line number Diff line
@@ -1064,8 +1064,8 @@ GeckoDriver.prototype.get = async function(cmd) { 


  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 = {
@@ -1183,8 +1183,8 @@ GeckoDriver.prototype.goBack = async function() { 
  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 = {
@@ -1226,8 +1226,8 @@ GeckoDriver.prototype.goForward = async function() { 
  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 = {
@@ -1263,8 +1263,8 @@ GeckoDriver.prototype.refresh = async function() { 
  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 = {
@@ -2146,9 +2146,9 @@ GeckoDriver.prototype.clickElement = async function(cmd) { 
      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

testing/marionette/listener.js

+13 −11
Original line number Diff line number Diff line
@@ -91,8 +91,8 @@ const eventObservers = new ContentEventObserverService( 
/**

 * 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,
@@ -128,7 +128,8 @@ const loadListener = { 
        .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) {
@@ -146,7 +147,7 @@ const loadListener = { 
      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
@@ -188,8 +189,9 @@ const loadListener = { 
    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) {}
@@ -360,7 +362,7 @@ const loadListener = { 


  /**

   * 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
@@ -992,9 +994,9 @@ function cancelRequest() { 


/**

 * 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
@@ -1003,7 +1005,7 @@ function cancelRequest() { 
 *     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;