Bug 1335778 - Make element click command check for page load and wait. r=ato

If the click command triggered a page load, it should not return before
the page has been fully loaded. With this patch we allow an opt-in for
commands to make use of an unload check. It's set to 200ms right now, and
will cancel an ongoing waitForPageLoad() if no page activity is detected.

MozReview-Commit-ID: DWV53sckBS2

--HG--
extra : rebase_source : 1b7905c101b7ebf406e88c73be5d0e069b7342c0

testing/marionette/driver.js

@@ -2026,7 +2026,25 @@ GeckoDriver.prototype.clickElement = function* (cmd, resp) {
       // listen for it and then just send an error back. The person making the
       // call should be aware something isnt right and handle accordingly
       this.addFrameCloseListener("click");
-      yield this.listener.clickElement(id);
+
+      let click = this.listener.clickElement({id: 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.
+      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 click;
       break;
   }
 };

testing/marionette/harness/marionette_harness/tests/unit/test_click.py

@@ -6,9 +6,8 @@ import urllib
 
 from marionette_driver.by import By
 from marionette_driver import errors
-from marionette_driver.wait import Wait
 
-from marionette_harness import MarionetteTestCase
+from marionette_harness import MarionetteTestCase, run_if_e10s
 
 
 def inline(doc):
@@ -97,11 +96,7 @@ class TestLegacyClick(MarionetteTestCase):
         test_html = self.marionette.absolute_url("clicks.html")
         self.marionette.navigate(test_html)
         self.marionette.find_element(By.LINK_TEXT, "333333").click()
-        # Bug 1335778 - Missing implicit wait for page being loaded
-        Wait(self.marionette, timeout=self.marionette.timeout.page_load,
-             ignored_exceptions=errors.NoSuchElementException).until(
-            lambda m: m.find_element(By.ID, "username"),
-            message="Username field hasn't been found")
+        self.marionette.find_element(By.ID, "username")
         self.assertEqual(self.marionette.title, "XHTML Test Page")
 
     def test_clicking_an_element_that_is_not_displayed_raises(self):
@@ -115,11 +110,7 @@ class TestLegacyClick(MarionetteTestCase):
         test_html = self.marionette.absolute_url("clicks.html")
         self.marionette.navigate(test_html)
         self.marionette.find_element(By.ID, "overflowLink").click()
-        # Bug 1335778 - Missing implicit wait for page being loaded
-        Wait(self.marionette, timeout=self.marionette.timeout.page_load,
-             ignored_exceptions=errors.NoSuchElementException).until(
-            lambda m: m.find_element(By.ID, "username"),
-            message="Username field hasn't been found")
+        self.marionette.find_element(By.ID, "username")
         self.assertEqual(self.marionette.title, "XHTML Test Page")
 
     def test_scroll_into_view_near_end(self):
@@ -277,3 +268,58 @@ class TestClick(TestLegacyClick):
                                      "does not have pointer events enabled"):
             button.click()
         self.assertFalse(self.marionette.execute_script("return window.clicked", sandbox=None))
+
+
+class TestClickNavigation(MarionetteTestCase):
+
+    def setUp(self):
+        super(TestClickNavigation, self).setUp()
+
+        self.test_page = self.marionette.absolute_url("clicks.html")
+        self.marionette.navigate(self.test_page)
+
+    def close_notification(self):
+        try:
+            with self.marionette.using_context("chrome"):
+                popup = self.marionette.find_element(
+                    By.CSS_SELECTOR, "#notification-popup popupnotification")
+                popup.find_element(By.ANON_ATTRIBUTE,
+                                   {"anonid": "closebutton"}).click()
+        except errors.NoSuchElementException:
+            pass
+
+    def test_click_link_page_load(self):
+        self.marionette.find_element(By.LINK_TEXT, "333333").click()
+        self.assertNotEqual(self.marionette.get_url(), self.test_page)
+        self.assertEqual(self.marionette.title, "XHTML Test Page")
+
+    def test_click_link_anchor(self):
+        self.marionette.find_element(By.ID, "anchor").click()
+        self.assertEqual(self.marionette.get_url(), "{}#".format(self.test_page))
+
+    def test_click_link_install_addon(self):
+        try:
+            self.marionette.find_element(By.ID, "install-addon").click()
+            self.assertEqual(self.marionette.get_url(), self.test_page)
+        finally:
+            self.close_notification()
+
+    def test_click_no_link(self):
+        self.marionette.find_element(By.ID, "showbutton").click()
+        self.assertEqual(self.marionette.get_url(), self.test_page)
+
+    @run_if_e10s("Requires e10s mode enabled")
+    def test_click_remoteness_change(self):
+        self.marionette.navigate("about:robots")
+        self.marionette.navigate(self.test_page)
+        self.marionette.find_element(By.ID, "anchor")
+
+        self.marionette.navigate("about:robots")
+        with self.assertRaises(errors.NoSuchElementException):
+            self.marionette.find_element(By.ID, "anchor")
+
+        self.marionette.go_back()
+        self.marionette.find_element(By.ID, "anchor")
+        self.marionette.find_element(By.ID, "history-back").click()
+        with self.assertRaises(errors.NoSuchElementException):
+            self.marionette.find_element(By.ID, "anchor")

testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py

@@ -8,7 +8,7 @@ from marionette_driver import errors
 from marionette_driver.marionette import Alert
 from marionette_driver.wait import Wait
 
-from marionette_harness import MarionetteTestCase, skip_if_e10s, WindowManagerMixin
+from marionette_harness import MarionetteTestCase, WindowManagerMixin
 
 
 class BaseAlertTestCase(WindowManagerMixin, MarionetteTestCase):

testing/marionette/harness/marionette_harness/www/clicks.html

@@ -17,7 +17,10 @@
 </iframe>
 
 <a href="xhtmlTest.html" id="normal">I'm a normal link</a>
+<a href="#" onclick="history.back();" id="history-back">Back in browser history</a>
+<a href="#" onclick="history.forward();" id="history-forward">Forward in browser history</a>
 <a href="#" id="anchor">I go to an anchor</a>
+<a href="addons/restartless_addon_signed.xpi" id="install-addon">Install Add-on</a>
 <a href="javascript:window.open('xhtmlTest.html', '_blank')" id="new-window">I open a window with javascript</a>
 <a href="xhtmlTest.html" id="twoClientRects"><span></span><span>Click me</span></a>
 <a href="xhtmlTest.html" id="link-with-enclosed-image"><img id="enclosed-image" src="./icon.gif"/></a>

testing/marionette/interaction.js

@@ -178,8 +178,8 @@ function* webdriverClickElement (el, a11y) {
   yield interaction.flushEventLoop(win);
 
   // step 10
-  // TODO(ato): if the click causes navigation,
-  // run post-navigation checks
+  // if the click causes navigation, the post-navigation checks are
+  // handled by the load listener in listener.js
 }
 
 function* chromeClick (el, a11y) {
@@ -291,20 +291,19 @@ interaction.selectOption = function (el) {
  *     |win| has closed or been unloaded before the queue can be flushed.
  */
 interaction.flushEventLoop = function* (win) {
-  let unloadEv;
+  return new Promise(resolve => {
+    let handleEvent = event => {
+      win.removeEventListener("beforeunload", this);
+      resolve();
+    };
 
-  return new Promise((resolve, reject) => {
     if (win.closed) {
-      reject();
+      resolve();
       return;
     }
 
-    unloadEv = reject;
-    win.addEventListener("unload", unloadEv, {once: true});
-
-    win.requestAnimationFrame(resolve);
-  }).then(() => {
-    win.removeEventListener("unload", unloadEv);
+    win.addEventListener("beforeunload", handleEvent);
+    win.requestAnimationFrame(handleEvent);
   });
 };
 

testing/marionette/listener.js

@@ -117,8 +117,10 @@ var sandboxName = "default";
  */
 var loadListener = {
   command_id: null,
+  seenUnload: null,
   timeout: null,
-  timer: null,
+  timerPageLoad: null,
+  timerPageUnload: null,
 
   /**
    * Start listening for page unload/load events.
@@ -136,48 +138,81 @@ var loadListener = {
     this.command_id = command_id;
     this.timeout = timeout;
 
+    this.seenUnload = false;
+
+    this.timerPageLoad = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+    this.timerPageUnload = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+
     // In case of a remoteness change, only wait the remaining time
     timeout = startTime + timeout - new Date().getTime();
 
     if (timeout <= 0) {
-      this.notify();
+      this.notify(this.timerPageLoad);
       return;
     }
 
-    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
-    this.timer.initWithCallback(this, timeout, Ci.nsITimer.TYPE_ONE_SHOT);
-
     if (waitForUnloaded) {
       addEventListener("hashchange", this, false);
       addEventListener("pagehide", this, false);
+
+      // The event can only be received if the listener gets added to the
+      // currently selected frame.
+      curContainer.frame.addEventListener("unload", this, false);
+
+      Services.obs.addObserver(this, "outer-window-destroyed");
     } else {
       addEventListener("DOMContentLoaded", loadListener, false);
       addEventListener("pageshow", loadListener, false);
     }
+
+    this.timerPageLoad.initWithCallback(this, timeout, Ci.nsITimer.TYPE_ONE_SHOT);
   },
 
   /**
    * Stop listening for page unload/load events.
    */
   stop: function () {
-    if (this.timer) {
-      this.timer.cancel();
-      this.timer = null;
+    if (this.timerPageLoad) {
+      this.timerPageLoad.cancel();
+    }
+
+    if (this.timerPageUnload) {
+      this.timerPageUnload.cancel();
     }
 
     removeEventListener("hashchange", this);
     removeEventListener("pagehide", this);
     removeEventListener("DOMContentLoaded", this);
     removeEventListener("pageshow", this);
+
+    // If the original content window, where the navigation was triggered,
+    // doesn't exist anymore, exceptions can be silently ignored.
+    try {
+      curContainer.frame.removeEventListener("unload", this);
+    } catch (e if e.name == "TypeError") {}
+
+    // In the case when the observer was added before a remoteness change,
+    // it will no longer be available. Exceptions can be silently ignored.
+    try {
+      Services.obs.removeObserver(this, "outer-window-destroyed");
+    } catch (e) {}
   },
 
   /**
    * Callback for registered DOM events.
    */
   handleEvent: function (event) {
+    logger.debug(`Received DOM event "${event.type}" for "${event.originalTarget.baseURI}"`);
+
     switch (event.type) {
+      case "unload":
+        this.seenUnload = true;
+        break;
+
       case "pagehide":
         if (event.originalTarget === curContainer.frame.document) {
+          this.seenUnload = true;
+
           removeEventListener("hashchange", this);
           removeEventListener("pagehide", this);
 
@@ -223,9 +258,43 @@ var loadListener = {
    * Callback for navigation timeout timer.
    */
   notify: function (timer) {
-    this.stop();
-    sendError(new TimeoutError("Timeout loading page after " + this.timeout + "ms"),
-              this.command_id);
+    switch (timer) {
+      // If the page unload timer is raised, ensure to properly stop the load
+      // listener, and return from the currently active command.
+      case this.timerPageUnload:
+        if (!this.seenUnload) {
+          logger.debug("Canceled page load listener because no navigation " +
+              "has been detected");
+          this.stop();
+          sendOk(this.command_id);
+        }
+        break;
+
+    case this.timerPageLoad:
+      this.stop();
+      sendError(new TimeoutError(`Timeout loading page after ${this.timeout}ms`),
+          this.command_id);
+      break;
+    }
+  },
+
+  observe: function (subject, topic, data) {
+    const winID = subject.QueryInterface(Components.interfaces.nsISupportsPRUint64).data;
+    const curWinID = curContainer.frame.QueryInterface(Ci.nsIInterfaceRequestor)
+        .getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
+
+    logger.debug(`Received observer notification "${topic}" for "${winID}"`);
+
+    switch (topic) {
+      // In the case when the currently selected frame is about to close,
+      // there will be no further load events. Stop listening immediately.
+      case "outer-window-destroyed":
+        if (curWinID === winID) {
+          this.stop();
+          sendOk(this.command_id);
+        }
+        break;
+    }
   },
 
   /**
@@ -251,23 +320,15 @@ var loadListener = {
    * @param {number} command_id
    *     ID of the currently handled message between the driver and listener.
    * @param {number} pageTimeout
-   *     Timeout in milliseconds the method has to wait for the page being finished loading.
+   *     Timeout in milliseconds the method has to wait for the page finished loading.
+   * @param {boolean=} loadEventExpected
+   *     Optional flag, which indicates that navigate has to wait for the page
+   *     finished loading.
    * @param {string=} url
    *     Optional URL, which is used to check if a page load is expected.
    */
-  navigate: function (trigger, command_id, timeout, url = undefined) {
-    let loadEventExpected = true;
-
-    if (typeof url == "string") {
-      try {
-        let requestedURL = new URL(url).toString();
-        loadEventExpected = navigate.isLoadEventExpected(requestedURL);
-      } catch (e) {
-        sendError(new InvalidArgumentError("Malformed URL: " + e.message), command_id);
-        return;
-      }
-    }
-
+  navigate: function (trigger, command_id, timeout, loadEventExpected = true,
+      useUnloadTimer = false) {
     if (loadEventExpected) {
       let startTime = new Date().getTime();
       this.start(command_id, timeout, startTime, true);
@@ -277,8 +338,14 @@ var loadListener = {
       yield trigger();
 
     }).then(val => {
-      if (!loadEventExpected) {
+     if (!loadEventExpected) {
         sendOk(command_id);
+        return;
+      }
+
+      // If requested setup a timer to detect a possible page load
+      if (useUnloadTimer) {
+        this.timerPageUnload.initWithCallback(this, 200, Ci.nsITimer.TYPE_ONE_SHOT);
       }
 
     }).catch(err => {
@@ -286,7 +353,6 @@ var loadListener = {
         this.stop();
       }
 
-      // Check why we do not raise an error if err is of type Event
       sendError(err, command_id);
       return;
     });
@@ -402,7 +468,6 @@ function removeMessageListenerId(messageName, handler) {
 var getTitleFn = dispatch(getTitle);
 var getPageSourceFn = dispatch(getPageSource);
 var getActiveElementFn = dispatch(getActiveElement);
-var clickElementFn = dispatch(clickElement);
 var getElementAttributeFn = dispatch(getElementAttribute);
 var getElementPropertyFn = dispatch(getElementProperty);
 var getElementTextFn = dispatch(getElementText);
@@ -457,7 +522,7 @@ function startListeners() {
   addMessageListenerId("Marionette:findElementContent", findElementContentFn);
   addMessageListenerId("Marionette:findElementsContent", findElementsContentFn);
   addMessageListenerId("Marionette:getActiveElement", getActiveElementFn);
-  addMessageListenerId("Marionette:clickElement", clickElementFn);
+  addMessageListenerId("Marionette:clickElement", clickElement);
   addMessageListenerId("Marionette:getElementAttribute", getElementAttributeFn);
   addMessageListenerId("Marionette:getElementProperty", getElementPropertyFn);
   addMessageListenerId("Marionette:getElementText", getElementTextFn);
@@ -562,7 +627,7 @@ function deleteSession(msg) {
   removeMessageListenerId("Marionette:findElementContent", findElementContentFn);
   removeMessageListenerId("Marionette:findElementsContent", findElementsContentFn);
   removeMessageListenerId("Marionette:getActiveElement", getActiveElementFn);
-  removeMessageListenerId("Marionette:clickElement", clickElementFn);
+  removeMessageListenerId("Marionette:clickElement", clickElement);
   removeMessageListenerId("Marionette:getElementAttribute", getElementAttributeFn);
   removeMessageListenerId("Marionette:getElementProperty", getElementPropertyFn);
   removeMessageListenerId("Marionette:getElementText", getElementTextFn);
@@ -1101,15 +1166,26 @@ function waitForPageLoaded(msg) {
  */
 function get(msg) {
   let {command_id, pageTimeout, url} = msg.json;
+  let loadEventExpected = true;
 
   try {
+    if (typeof url == "string") {
+      try {
+        let requestedURL = new URL(url).toString();
+        loadEventExpected = navigate.isLoadEventExpected(requestedURL);
+      } catch (e) {
+        sendError(new InvalidArgumentError("Malformed URL: " + e.message), command_id);
+        return;
+      }
+    }
+
     // We need to move to the top frame before navigating
     sendSyncMessage("Marionette:switchedToFrame", {frameValue: null});
     curContainer.frame = content;
 
     loadListener.navigate(() => {
       curContainer.frame.location = url;
-    }, command_id, pageTimeout, url);
+    }, command_id, pageTimeout, loadEventExpected);
 
   } catch (e) {
     sendError(e, command_id);
@@ -1255,15 +1331,36 @@ function getActiveElement() {
 /**
  * Send click event to element.
  *
+ * @param {number} command_id
+ *     ID of the currently handled message between the driver and listener.
  * @param {WebElement} id
  *     Reference to the web element to click.
+ * @param {number} pageTimeout
+ *     Timeout in milliseconds the method has to wait for the page being finished loading.
  */
-function clickElement(id) {
-  let el = seenEls.get(id, curContainer);
-  return interaction.clickElement(
-      el,
-      capabilities.get("moz:accessibilityChecks"),
-      capabilities.get("specificationLevel") >= 1);
+function clickElement(msg) {
+  let {command_id, id, pageTimeout} = msg.json;
+
+  try {
+    let loadEventExpected = true;
+
+    let target = getElementAttribute(id, "target");
+
+    if (target === "_blank") {
+      loadEventExpected = false;
+    }
+
+    loadListener.navigate(() => {
+      return interaction.clickElement(
+        seenEls.get(id, curContainer),
+        capabilities.get("moz:accessibilityChecks"),
+        capabilities.get("specificationLevel") >= 1
+      );
+    }, command_id, pageTimeout, loadEventExpected, true);
+
+  } catch (e) {
+    sendError(e, command_id);
+  }
 }
 
 function getElementAttribute(id, name) {