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 : 2c4d2a19a006645ecd44e08a28309367bf4f8d32

testing/marionette/driver.js

@@ -1897,7 +1897,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

@@ -8,7 +8,7 @@ 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 +97,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
+        self.marionette.find_element(By.ID, "username")
-        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.assertEqual(self.marionette.title, "XHTML Test Page")
 
     def test_clicking_an_element_that_is_not_displayed_raises(self):
@@ -115,11 +111,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
+        self.marionette.find_element(By.ID, "username")
-        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.assertEqual(self.marionette.title, "XHTML Test Page")
 
     def test_scroll_into_view_near_end(self):
@@ -260,3 +252,45 @@ class TestClick(TestLegacyClick):
         with self.assertRaises(errors.ElementClickInterceptedException):
             obscured.click()
         self.assertFalse(self.marionette.execute_script("return window.clicked", sandbox=None))
+
+
+class TestClickNavigation(MarionetteTestCase):
+
+    def setUp(self):
+        super(TestClickNavigation, self).setUp()
+
+    def test_click_link_page_load(self):
+        test_html = self.marionette.absolute_url("clicks.html")
+        self.marionette.navigate(test_html)
+        self.marionette.find_element(By.LINK_TEXT, "333333").click()
+        self.assertNotEqual(self.marionette.get_url(), test_html)
+        self.assertEqual(self.marionette.title, "XHTML Test Page")
+
+    def test_click_link_anchor(self):
+        test_html = self.marionette.absolute_url("clicks.html")
+        self.marionette.navigate(test_html)
+        self.marionette.find_element(By.ID, "anchor").click()
+        self.assertEqual(self.marionette.get_url(), "{}#".format(test_html))
+
+    def test_click_no_link(self):
+        test_html = self.marionette.absolute_url("clicks.html")
+        self.marionette.navigate(test_html)
+        self.marionette.find_element(By.ID, "showbutton").click()
+        self.assertEqual(self.marionette.get_url(), test_html)
+
+    @run_if_e10s("Requires e10s mode enabled")
+    def test_click_remoteness_change(self):
+        test_html = self.marionette.absolute_url("clicks.html")
+
+        self.marionette.navigate("about:robots")
+        self.marionette.navigate(test_html)
+        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.expected import element_present
 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):
@@ -194,7 +194,6 @@ class TestTabModalAlerts(BaseAlertTestCase):
         alert.accept()
         self.wait_for_condition(lambda mn: mn.get_url() == "about:blank")
 
-    @skip_if_e10s("Bug 1325044")
     def test_unrelated_command_when_alert_present(self):
         click_handler = self.marionette.find_element(By.ID, "click-handler")
         text = self.marionette.find_element(By.ID, "click-result").text

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

@@ -17,6 +17,8 @@
 </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="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>

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,35 +138,44 @@ 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("beforeunload", this, false);
       addEventListener("hashchange", this, false);
       addEventListener("pagehide", this, false);
     } 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) {
+    if (this.timerPageLoad) {
-      this.timer.cancel();
+      this.timerPageLoad.cancel();
-      this.timer = null;
     }
 
+    if (this.timerPageUnload) {
+      this.timerPageUnload.cancel();
+    }
+
+    removeEventListener("beforeunload", this);
     removeEventListener("hashchange", this);
     removeEventListener("pagehide", this);
     removeEventListener("DOMContentLoaded", this);
@@ -176,8 +187,13 @@ var loadListener = {
    */
   handleEvent: function (event) {
     switch (event.type) {
+      case "beforeunload":
+        this.seenUnload = true;
+        break;
+
       case "pagehide":
         if (event.originalTarget === curContainer.frame.document) {
+          removeEventListener("beforeunload", this);
           removeEventListener("hashchange", this);
           removeEventListener("pagehide", this);
 
@@ -223,9 +239,22 @@ var loadListener = {
    * Callback for navigation timeout timer.
    */
   notify: function (timer) {
+    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) {
           this.stop();
-    sendError(new TimeoutError("Timeout loading page after " + this.timeout + "ms"),
+          sendOk(this.command_id);
+        }
+        break;
+
+    case this.timerPageLoad:
+      this.stop();
+      sendError(new TimeoutError(`Timeout loading page after ${this.timeout}ms`),
           this.command_id);
+      break;
+    }
   },
 
   /**
@@ -252,22 +281,13 @@ var loadListener = {
    *     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.
+   * @param {boolean=} loadEventExpected
+   *     TODO
    * @param {string=} url
    *     Optional URL, which is used to check if a page load is expected.
    */
-  navigate: function (trigger, command_id, timeout, url = undefined) {
+  navigate: function (trigger, command_id, timeout, loadEventExpected = true,
-    let loadEventExpected = true;
+      useUnloadTimer = false) {
-
-    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;
-      }
-    }
-
     if (loadEventExpected) {
       let startTime = new Date().getTime();
       this.start(command_id, timeout, startTime, true);
@@ -277,7 +297,13 @@ var loadListener = {
       yield trigger();
 
     }).then(val => {
-      if (!loadEventExpected) {
+      if (loadEventExpected) {
+        // Setup timer to detect a possible page load
+        // TODO: Make it optional to wait + time with multiplier
+        if (useUnloadTimer) {
+          this.timerPageUnload.initWithCallback(this, "200", Ci.nsITimer.TYPE_ONE_SHOT);
+        }
+      } else {
         sendOk(command_id);
       }
 
@@ -401,7 +427,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);
@@ -456,7 +481,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);
@@ -561,7 +586,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);
@@ -1100,15 +1125,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);
@@ -1254,15 +1290,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) {
+function clickElement(msg) {
-  let el = seenEls.get(id, curContainer);
+  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(
-      el,
+        seenEls.get(id, curContainer),
         capabilities.get("moz:accessibilityChecks"),
-      capabilities.get("specificationLevel") >= 1);
+        capabilities.get("specificationLevel") >= 1
+      );
+    }, command_id, pageTimeout, loadEventExpected, true);
+
+  } catch (e) {
+    sendError(e, command_id);
+  }
 }
 
 function getElementAttribute(id, name) {