Backed out changeset cf7d24fe5cec (bug 1291320)

testing/marionette/driver.js

@@ -961,7 +961,7 @@ GeckoDriver.prototype.executeJSScript = function* (cmd, resp) {
  * @param {string} url
  *     URL to navigate to.
  */
-GeckoDriver.prototype.get = function* (cmd, resp) {
+GeckoDriver.prototype.get = function*(cmd, resp) {
   assert.content(this.context);
   assert.window(this.getCurrentWindow());
 
@@ -980,7 +980,7 @@ GeckoDriver.prototype.get = function* (cmd, resp) {
       startTime: new Date().getTime(),
     };
     this.mm.broadcastAsyncMessage(
-        "Marionette:waitForPageLoaded" + this.curBrowser.curFrameId,
+        "Marionette:pollForReadyState" + this.curBrowser.curFrameId,
         parameters);
   });
 
@@ -1081,7 +1081,9 @@ GeckoDriver.prototype.goBack = function* (cmd, resp) {
       startTime: new Date().getTime(),
     };
     this.mm.broadcastAsyncMessage(
-        "Marionette:waitForPageLoaded" + this.curBrowser.curFrameId,
+        // TODO: combine with
+        // "Marionette:pollForReadyState" + this.curBrowser.curFrameId,
+        "Marionette:pollForReadyState" + this.curBrowser.curFrameId,
         parameters);
   });
 
@@ -1121,7 +1123,9 @@ GeckoDriver.prototype.goForward = function* (cmd, resp) {
       startTime: new Date().getTime(),
     };
     this.mm.broadcastAsyncMessage(
-        "Marionette:waitForPageLoaded" + this.curBrowser.curFrameId,
+        // TODO: combine with
+        // "Marionette:pollForReadyState" + this.curBrowser.curFrameId,
+        "Marionette:pollForReadyState" + this.curBrowser.curFrameId,
         parameters);
   });
 

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

@@ -31,7 +31,7 @@ class BaseNavigationTestCase(WindowManagerMixin, MarionetteTestCase):
         self.test_page_not_remote = "about:robots"
         self.test_page_remote = self.marionette.absolute_url("test.html")
 
-        if self.marionette.session_capabilities["platformName"] == "darwin":
+        if self.marionette.session_capabilities['platformName'] == 'darwin':
             self.mod_key = Keys.META
         else:
             self.mod_key = Keys.CONTROL
@@ -92,15 +92,56 @@ class BaseNavigationTestCase(WindowManagerMixin, MarionetteTestCase):
               return tabBrowser.isRemoteBrowser;
             """)
 
+    def run_bfcache_test(self, test_pages):
+        # Helper method to run simple back and forward testcases.
+        for index, page in enumerate(test_pages):
+            if "error" in page:
+                with self.assertRaises(page["error"]):
+                    self.marionette.navigate(page["url"])
+            else:
+                self.marionette.navigate(page["url"])
+            self.assertEqual(page["url"], self.marionette.get_url())
+            self.assertEqual(self.history_length, index + 1)
+
+            if "is_remote" in page:
+                self.assertEqual(page["is_remote"], self.is_remote_tab,
+                                 "'{}' doesn't match expected remoteness state: {}".format(
+                                     page["url"], page["is_remote"]))
+
+        for page in test_pages[-2::-1]:
+            if "error" in page:
+                with self.assertRaises(page["error"]):
+                    self.marionette.go_back()
+            else:
+                self.marionette.go_back()
+            self.assertEqual(page["url"], self.marionette.get_url())
+
+        if "is_remote" in page:
+            self.assertEqual(page["is_remote"], self.is_remote_tab,
+                             "'{}' doesn't match expected remoteness state: {}".format(
+                                 page["url"], page["is_remote"]))
+
+        for page in test_pages[1::]:
+            if "error" in page:
+                with self.assertRaises(page["error"]):
+                    self.marionette.go_forward()
+            else:
+                self.marionette.go_forward()
+            self.assertEqual(page["url"], self.marionette.get_url())
+
+        if "is_remote" in page:
+            self.assertEqual(page["is_remote"], self.is_remote_tab,
+                             "'{}' doesn't match expected remoteness state: {}".format(
+                                 page["url"], page["is_remote"]))
+
 
 class TestNavigate(BaseNavigationTestCase):
 
     def test_set_location_through_execute_script(self):
         self.marionette.execute_script(
-            "window.location.href = '{}'".format(self.test_page_remote),
+            "window.location.href = '%s'" % self.test_page_remote)
-            sandbox=None)
         Wait(self.marionette).until(
-            lambda mn: self.test_page_remote == mn.get_url())
+            lambda _: self.test_page_remote == self.marionette.get_url())
         self.assertEqual("Marionette Test", self.marionette.title)
 
     def test_navigate_chrome_unsupported_error(self):
@@ -162,13 +203,13 @@ class TestNavigate(BaseNavigationTestCase):
     def test_find_element_state_complete(self):
         self.marionette.navigate(self.test_page_remote)
         state = self.marionette.execute_script(
-            "return window.document.readyState", sandbox=None)
+            "return window.document.readyState")
         self.assertEqual("complete", state)
         self.assertTrue(self.marionette.find_element(By.ID, "mozLink"))
 
     def test_navigate_timeout_error_no_remoteness_change(self):
         is_remote_before_timeout = self.is_remote_tab
-        self.marionette.timeout.page_load = 0.5
+        self.marionette.timeout.page_load = 0.1
         with self.assertRaises(errors.TimeoutException):
             self.marionette.navigate(self.marionette.absolute_url("slow"))
         self.assertEqual(self.is_remote_tab, is_remote_before_timeout)
@@ -179,7 +220,7 @@ class TestNavigate(BaseNavigationTestCase):
         self.marionette.navigate("about:robots")
         self.assertFalse(self.is_remote_tab)
 
-        self.marionette.timeout.page_load = 0.5
+        self.marionette.timeout.page_load = 0.1
         with self.assertRaises(errors.TimeoutException):
             self.marionette.navigate(self.marionette.absolute_url("slow"))
 
@@ -204,7 +245,7 @@ class TestNavigate(BaseNavigationTestCase):
     def test_about_blank_for_new_docshell(self):
         self.assertEqual(self.marionette.get_url(), "about:blank")
 
-        self.marionette.navigate("about:blank")
+        self.marionette.navigate('about:blank')
 
     @run_if_manage_instance("Only runnable if Marionette manages the instance")
     @skip_if_mobile("Bug 1322993 - Missing temporary folder")
@@ -232,10 +273,10 @@ class TestNavigate(BaseNavigationTestCase):
         self.assertFalse(self.is_remote_tab)
 
         with self.marionette.using_context("chrome"):
-            urlbar = self.marionette.find_element(By.ID, "urlbar")
+            urlbar = self.marionette.find_element(By.ID, 'urlbar')
-            urlbar.send_keys(self.mod_key + "a")
+            urlbar.send_keys(self.mod_key + 'a')
-            urlbar.send_keys(self.mod_key + "x")
+            urlbar.send_keys(self.mod_key + 'x')
-            urlbar.send_keys("about:support" + Keys.ENTER)
+            urlbar.send_keys('about:support' + Keys.ENTER)
 
         Wait(self.marionette).until(
             lambda mn: mn.get_url() == "about:support",
@@ -248,9 +289,9 @@ class TestNavigate(BaseNavigationTestCase):
         self.assertTrue(self.is_remote_tab)
 
         with self.marionette.using_context("chrome"):
-            urlbar = self.marionette.find_element(By.ID, "urlbar")
+            urlbar = self.marionette.find_element(By.ID, 'urlbar')
-            urlbar.send_keys(self.mod_key + "a")
+            urlbar.send_keys(self.mod_key + 'a')
-            urlbar.send_keys(self.mod_key + "x")
+            urlbar.send_keys(self.mod_key + 'x')
             urlbar.send_keys(self.test_page_remote + Keys.ENTER)
 
         Wait(self.marionette).until(
@@ -265,7 +306,7 @@ class TestNavigate(BaseNavigationTestCase):
             self.marionette.navigate(self.test_page_remote)
             with self.marionette.using_context("chrome"):
                 main_win = self.marionette.find_element(By.ID, "main-window")
-                main_win.send_keys(self.mod_key, Keys.SHIFT, "a")
+                main_win.send_keys(self.mod_key, Keys.SHIFT, 'a')
 
         new_tab = self.open_tab(trigger=open_with_shortcut)
         self.marionette.switch_to_window(new_tab)
@@ -276,51 +317,6 @@ class TestNavigate(BaseNavigationTestCase):
 
 class TestBackForwardNavigation(BaseNavigationTestCase):
 
-    def run_bfcache_test(self, test_pages):
-        # Helper method to run simple back and forward testcases.
-        for index, page in enumerate(test_pages):
-            if "error" in page:
-                with self.assertRaises(page["error"]):
-                    self.marionette.navigate(page["url"])
-            else:
-                self.marionette.navigate(page["url"])
-            self.assertEqual(page["url"], self.marionette.get_url())
-            self.assertEqual(self.history_length, index + 1)
-
-            if "is_remote" in page:
-                self.assertEqual(page["is_remote"], self.is_remote_tab,
-                                 "'{}' doesn't match expected remoteness state: {}".format(
-                                     page["url"], page["is_remote"]))
-
-        # Now going back in history for all test pages by backward iterating
-        # through the list (-1) and skipping the first entry at the end (-2).
-        for page in test_pages[-2::-1]:
-            if "error" in page:
-                with self.assertRaises(page["error"]):
-                    self.marionette.go_back()
-            else:
-                self.marionette.go_back()
-            self.assertEqual(page["url"], self.marionette.get_url())
-
-        if "is_remote" in page:
-            self.assertEqual(page["is_remote"], self.is_remote_tab,
-                             "'{}' doesn't match expected remoteness state: {}".format(
-                                 page["url"], page["is_remote"]))
-
-        # Now going forward in history by skipping the first entry.
-        for page in test_pages[1::]:
-            if "error" in page:
-                with self.assertRaises(page["error"]):
-                    self.marionette.go_forward()
-            else:
-                self.marionette.go_forward()
-            self.assertEqual(page["url"], self.marionette.get_url())
-
-        if "is_remote" in page:
-            self.assertEqual(page["is_remote"], self.is_remote_tab,
-                             "'{}' doesn't match expected remoteness state: {}".format(
-                                 page["url"], page["is_remote"]))
-
     def test_no_history_items(self):
         # Both methods should not raise a failure if no navigation is possible
         self.marionette.go_back()
@@ -397,18 +393,18 @@ class TestBackForwardNavigation(BaseNavigationTestCase):
         self.marionette.find_element(*test_element_locator)
         self.assertEqual(self.marionette.get_url(), page)
 
-    def test_image_to_html_to_image(self):
+    def test_image_document_to_html(self):
         test_pages = [
-            {"url": self.marionette.absolute_url("black.png")},
+            {"url": self.marionette.absolute_url('black.png')},
             {"url": self.test_page_remote},
-            {"url": self.marionette.absolute_url("white.png")},
+            {"url": self.marionette.absolute_url('white.png')},
         ]
         self.run_bfcache_test(test_pages)
 
-    def test_image_to_image(self):
+    def test_image_document_to_image_document(self):
         test_pages = [
-            {"url": self.marionette.absolute_url("black.png")},
+            {"url": self.marionette.absolute_url('black.png')},
-            {"url": self.marionette.absolute_url("white.png")},
+            {"url": self.marionette.absolute_url('white.png')},
         ]
         self.run_bfcache_test(test_pages)
 
@@ -440,9 +436,9 @@ class TestBackForwardNavigation(BaseNavigationTestCase):
 
     def test_timeout_error(self):
         urls = [
-            self.marionette.absolute_url("slow?delay=3"),
+            self.marionette.absolute_url('slow'),
             self.test_page_remote,
-            self.marionette.absolute_url("slow?delay=4"),
+            self.marionette.absolute_url('slow'),
         ]
 
         # First, load all pages completely to get them added to the cache
@@ -455,29 +451,21 @@ class TestBackForwardNavigation(BaseNavigationTestCase):
         self.assertEqual(urls[1], self.marionette.get_url())
 
         # Force triggering a timeout error
-        self.marionette.timeout.page_load = 0.5
+        self.marionette.timeout.page_load = 0.1
         with self.assertRaises(errors.TimeoutException):
             self.marionette.go_back()
-        self.marionette.timeout.reset()
+        self.assertEqual(urls[0], self.marionette.get_url())
-
+        self.marionette.timeout.page_load = 300000
-        Wait(self.marionette, self.marionette.timeout.page_load).until(
-            lambda mn: urls[0] == mn.get_url(),
-            message="Slow loading page has been successfully loaded after going back")
-        self.assertEqual(self.marionette.find_element(By.ID, "delay").text, "3")
 
         self.marionette.go_forward()
         self.assertEqual(urls[1], self.marionette.get_url())
 
         # Force triggering a timeout error
-        self.marionette.timeout.page_load = 0.5
+        self.marionette.timeout.page_load = 0.1
         with self.assertRaises(errors.TimeoutException):
             self.marionette.go_forward()
-        self.marionette.timeout.reset()
+        self.assertEqual(urls[2], self.marionette.get_url())
-
+        self.marionette.timeout.page_load = 300000
-        Wait(self.marionette, self.marionette.timeout.page_load).until(
-            lambda mn: urls[2] == mn.get_url(),
-            message="Slow loading page has been successfully loaded after going forward")
-        self.assertEqual(self.marionette.find_element(By.ID, "delay").text, "4")
 
     def test_certificate_error(self):
         test_pages = [

testing/marionette/listener.js

@@ -78,6 +78,9 @@ var originalOnError;
 var checkTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
 //timer for readystate
 var readyStateTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+// timer for navigation commands.
+var navTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+var onDOMContentLoaded;
 // Send move events about this often
 var EVENT_INTERVAL = 30; // milliseconds
 // last touch for each fingerId
@@ -109,186 +112,6 @@ var modalHandler = function() {
 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.
- */
-var loadListener = {
-  command_id: null,
-  timeout: null,
-  timer: null,
-
-  /**
-   * Start listening for page unload/load events.
-   *
-   * @param {number} command_id
-   *     ID of the currently handled message between the driver and listener.
-   * @param {number} timeout
-   *     Timeout in seconds the method has to wait for the page being finished loading.
-   * @param {number} startTime
-   *     Unix timestap when the navitation request got triggered.
-   * @param {boolean=} waitForUnloaded
-   *     If `true` wait for page unload events, otherwise only for page load events.
-   */
-  start: function (command_id, timeout, startTime, waitForUnloaded = true) {
-    this.command_id = command_id;
-    this.timeout = timeout;
-
-    // In case of a remoteness change, only wait the remaining time
-    timeout = startTime + timeout - new Date().getTime();
-
-    if (timeout <= 0) {
-      this.notify();
-      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);
-    } else {
-      addEventListener("DOMContentLoaded", loadListener, false);
-      addEventListener("pageshow", loadListener, false);
-    }
-  },
-
-  /**
-   * Stop listening for page unload/load events.
-   */
-  stop: function () {
-    if (this.timer) {
-      this.timer.cancel();
-      this.timer = null;
-    }
-
-    removeEventListener("hashchange", this);
-    removeEventListener("pagehide", this);
-    removeEventListener("DOMContentLoaded", this);
-    removeEventListener("pageshow", this);
-  },
-
-  /**
-   * Callback for registered DOM events.
-   */
-  handleEvent: function (event) {
-    switch (event.type) {
-      case "pagehide":
-        if (event.originalTarget === curContainer.frame.document) {
-          removeEventListener("hashchange", this);
-          removeEventListener("pagehide", this);
-
-          // Now wait until the target page has been loaded
-          addEventListener("DOMContentLoaded", this, false);
-          addEventListener("pageshow", this, false);
-        }
-        break;
-
-      case "hashchange":
-        this.stop();
-        sendOk(this.command_id);
-        break;
-
-      case "DOMContentLoaded":
-        if (event.originalTarget.baseURI.startsWith("about:certerror")) {
-          this.stop();
-          sendError(new InsecureCertificateError(), this.command_id);
-
-        } else if (/about:.*(error)\?/.exec(event.originalTarget.baseURI)) {
-          this.stop();
-          sendError(new UnknownError("Reached error page: " +
-              event.originalTarget.baseURI), this.command_id);
-
-        // Special-case about:blocked pages which should be treated as non-error
-        // pages but do not raise a pageshow event.
-        } else if (/about:blocked\?/.exec(event.originalTarget.baseURI)) {
-          this.stop();
-          sendOk(this.command_id);
-        }
-        break;
-
-      case "pageshow":
-        if (event.originalTarget === curContainer.frame.document) {
-          this.stop();
-          sendOk(this.command_id);
-        }
-        break;
-    }
-  },
-
-  /**
-   * Callback for navigation timeout timer.
-   */
-  notify: function (timer) {
-    this.stop();
-    sendError(new TimeoutError("Timeout loading page after " + this.timeout + "ms"),
-              this.command_id);
-  },
-
-  /**
-   * Continue to listen for page load events after a remoteness change happened.
-   *
-   * @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: function (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
-   *     Callback that triggers the page load.
-   * @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.
-   * @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;
-      }
-    }
-
-    if (loadEventExpected) {
-      let startTime = new Date().getTime();
-      this.start(command_id, timeout, startTime, true);
-    }
-
-    try {
-      trigger();
-    } catch (e) {
-      if (loadEventExpected) {
-        this.stop();
-      }
-      sendError(new UnknownCommandError(e.message), command_id);
-      return;
-    }
-
-    if (!loadEventExpected) {
-      sendOk(command_id);
-    }
-  },
-}
-
 /**
  * Called when listener is first started up.
  * The listener sends its unique window ID and its current URI to the actor.
@@ -360,7 +183,7 @@ function dispatch(fn) {
   return function (msg) {
     let id = msg.json.command_id;
 
-    let req = Task.spawn(function* () {
+    let req = Task.spawn(function*() {
       if (typeof msg.json == "undefined" || msg.json instanceof Array) {
         return yield fn.apply(null, msg.json);
       } else {
@@ -442,7 +265,7 @@ function startListeners() {
   addMessageListenerId("Marionette:actionChain", actionChainFn);
   addMessageListenerId("Marionette:multiAction", multiActionFn);
   addMessageListenerId("Marionette:get", get);
-  addMessageListenerId("Marionette:waitForPageLoaded", waitForPageLoaded);
+  addMessageListenerId("Marionette:pollForReadyState", pollForReadyState);
   addMessageListenerId("Marionette:cancelRequest", cancelRequest);
   addMessageListenerId("Marionette:getCurrentUrl", getCurrentUrlFn);
   addMessageListenerId("Marionette:getTitle", getTitleFn);
@@ -547,7 +370,7 @@ function deleteSession(msg) {
   removeMessageListenerId("Marionette:actionChain", actionChainFn);
   removeMessageListenerId("Marionette:multiAction", multiActionFn);
   removeMessageListenerId("Marionette:get", get);
-  removeMessageListenerId("Marionette:waitForPageLoaded", waitForPageLoaded);
+  removeMessageListenerId("Marionette:pollForReadyState", pollForReadyState);
   removeMessageListenerId("Marionette:cancelRequest", cancelRequest);
   removeMessageListenerId("Marionette:getTitle", getTitleFn);
   removeMessageListenerId("Marionette:getPageSource", getPageSourceFn);
@@ -624,8 +447,8 @@ function sendToServer(uuid, data = undefined) {
  * @param {UUID} uuid
  *     Unique identifier of the request.
  */
-function sendResponse(obj, uuid) {
+function sendResponse(obj, id) {
-  sendToServer(uuid, obj);
+  sendToServer(id, obj);
 }
 
 /**
@@ -1062,31 +885,84 @@ function multiAction(args, maxLen) {
   setDispatch(concurrentEvent, pendingTouches);
 }
 
-/**
- * Cancel the polling and remove the event listener associated with a
- * 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 navigate request). This is most of
  * of the work of a navigate request, but doesn't assume DOMContentLoaded is yet to fire.
  *
+ * @param {function=} cleanupCallback
+ *     Callback to execute when registered event handlers or observer notifications
+ *     have to be cleaned-up.
  * @param {number} command_id
  *     ID of the currently handled message between the driver and listener.
+ * @param {string=} lastSeenURL
+ *     Last URL as seen before the navigation request got triggered.
  * @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) {
+function pollForReadyState(msg) {
-  let {command_id, pageTimeout, startTime} = msg.json;
+  let {cleanupCallback, command_id, lastSeenURL, pageTimeout, startTime} = msg.json;
 
-  loadListener.waitForLoadAfterRemotenessChange(command_id, pageTimeout, startTime);
+  if (typeof startTime == "undefined") {
+    startTime = new Date().getTime();
+  }
+
+  if (typeof cleanupCallback == "undefined") {
+    cleanupCallback = () => {};
+  }
+
+  let endTime = startTime + pageTimeout;
+
+  let checkLoad = () => {
+    navTimer.cancel();
+
+    let doc = curContainer.frame.document;
+
+    if (pageTimeout === null || new Date().getTime() <= endTime) {
+      // Under some conditions (eg. for error pages) the pagehide event is fired
+      // even with a readyState complete for the formerly loaded page.
+      // To prevent race conditition for goBack and goForward we have to wait
+      // until the last seen page has been fully unloaded.
+      // TODO: Bug 1333458 has to improve this.
+      if (!doc.location || lastSeenURL && doc.location.href === lastSeenURL) {
+        navTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);
+
+      // document fully loaded
+      } else if (doc.readyState === "complete") {
+        cleanupCallback();
+        sendOk(command_id);
+
+      // document with an insecure cert
+      } else if (doc.readyState === "interactive" &&
+          doc.baseURI.startsWith("about:certerror")) {
+        cleanupCallback();
+        sendError(new InsecureCertificateError(), command_id);
+
+      // we have reached an error url without requesting it
+      } else if (doc.readyState === "interactive" &&
+          /about:.+(error)\?/.exec(doc.baseURI)) {
+        cleanupCallback();
+        sendError(new UnknownError("Reached error page: " + doc.baseURI), command_id);
+
+      // return early for about: urls
+      } else if (doc.readyState === "interactive" && doc.baseURI.startsWith("about:")) {
+        cleanupCallback();
+        sendOk(command_id);
+
+      // document not fully loaded
+      } else {
+        navTimer.initWithCallback(checkLoad, 100, Ci.nsITimer.TYPE_ONE_SHOT);
+      }
+
+    } else {
+      cleanupCallback();
+      sendError(new TimeoutError("Error loading page, timed out (checkLoad)"), command_id);
+    }
+  };
+
+  checkLoad();
 }
 
 /**
@@ -1096,49 +972,163 @@ function waitForPageLoaded(msg) {
  * driver (in chrome space).
  */
 function get(msg) {
-  let {command_id, pageTimeout, url} = msg.json;
+  let {pageTimeout, url, command_id} = msg.json;
+
+  let startTime = new Date().getTime();
 
   // We need to move to the top frame before navigating
   sendSyncMessage("Marionette:switchedToFrame", {frameValue: null});
   curContainer.frame = content;
 
-  loadListener.navigate(() => {
+  let docShell = curContainer.frame
-    curContainer.frame.location = url;
+      .document
-  }, command_id, pageTimeout, url);
+      .defaultView
+      .QueryInterface(Ci.nsIInterfaceRequestor)
+      .getInterface(Ci.nsIWebNavigation)
+      .QueryInterface(Ci.nsIDocShell);
+  let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
+      .getInterface(Ci.nsIWebProgress);
+  let sawLoad = false;
+
+  let requestedURL;
+  let loadEventExpected = false;
+  try {
+    requestedURL = new URL(url).toString();
+    let curURL = curContainer.frame.location;
+    loadEventExpected = navigate.isLoadEventExpected(curURL, requestedURL);
+  } catch (e) {
+    sendError(new InvalidArgumentError("Malformed URL: " + e.message), command_id);
+    return;
+  }
+
+  // It's possible that a site we're being sent to will end up redirecting
+  // us before we end up on a page that fires DOMContentLoaded. We can ensure
+  // This loadListener ensures that we don't send a success signal back to
+  // the caller until we've seen the load of the requested URL attempted
+  // on this frame.
+  let loadListener = {
+    QueryInterface: XPCOMUtils.generateQI(
+        [Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference]),
+
+    onStateChange(webProgress, request, state, status) {
+      if (!(request instanceof Ci.nsIChannel)) {
+        return;
+      }
+
+      const isDocument = state & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT;
+      const loadedURL = request.URI.spec;
+
+      // We have to look at the originalURL because of about: pages,
+      // the loadedURL is what the about: page resolves to, and is
+      // not the one that was requested.
+      const originalURL = request.originalURI.spec;
+      const isRequestedURL = loadedURL == requestedURL ||
+          originalURL == requestedURL;
+
+      if (!isDocument || !isRequestedURL) {
+        return;
+      }
+
+      // We started loading the requested document. This document
+      // might not be the one that ends up firing DOMContentLoaded
+      // (if it, for example, redirects), but because we've started
+      // loading this URL, we know that any future DOMContentLoaded's
+      // are fair game to tell the Marionette client about.
+      if (state & Ci.nsIWebProgressListener.STATE_START) {
+        sawLoad = true;
+      }
+
+      // This indicates network stop or last request stop outside of
+      // loading the document.  We hit this when DOMContentLoaded is
+      // not triggered, which is the case for image documents.
+      else if (state & Ci.nsIWebProgressListener.STATE_STOP &&
+          content.document instanceof content.ImageDocument) {
+        pollForReadyState({json: {
+          command_id: command_id,
+          pageTimeout: pageTimeout,
+          startTime: startTime,
+          cleanupCallback: () => {
+            webProgress.removeProgressListener(loadListener);
+            removeEventListener("DOMContentLoaded", onDOMContentLoaded, false);
+          }
+        }});
+      }
+    },
+
+    onLocationChange() {},
+    onProgressChange() {},
+    onStatusChange() {},
+    onSecurityChange() {},
+  };
+
+  webProgress.addProgressListener(
+      loadListener, Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
+
+  // Prevent DOMContentLoaded events from frames from invoking this
+  // code, unless the event is coming from the frame associated with
+  // the current window (i.e. someone has used switch_to_frame).
+  onDOMContentLoaded = ev => {
+    let frameEl = ev.originalTarget.defaultView.frameElement;
+    let correctFrame = !frameEl || frameEl == curContainer.frame.frameElement;
+
+    // If the page we're at fired DOMContentLoaded and appears to
+    // be the one we asked to load, then we definitely saw the load
+    // occur. We need this because for error pages, like about:neterror
+    // for unsupported protocols, we don't end up opening a channel that
+    // our WebProgressListener can monitor.
+    if (curContainer.frame.location == requestedURL) {
+      sawLoad = true;
+    }
+
+    // We also need to make sure that if the requested URL is not
+    // about:blank the DOMContentLoaded we saw isn't for the initial
+    // about:blank of a newly created docShell.
+    let loadedRequestedURI = (requestedURL == "about:blank") ||
+        docShell.hasLoadedNonBlankURI;
+
+    if (correctFrame && sawLoad && loadedRequestedURI) {
+      pollForReadyState({json: {
+        command_id: command_id,
+        pageTimeout: pageTimeout,
+        startTime: startTime,
+        cleanupCallback: () => {
+          webProgress.removeProgressListener(loadListener);
+          removeEventListener("DOMContentLoaded", onDOMContentLoaded, false);
+        }
+      }});
+    }
+  };
+
+  if (typeof pageTimeout != "undefined") {
+    let onTimeout = () => {
+      if (loadEventExpected) {
+        removeEventListener("DOMContentLoaded", onDOMContentLoaded, false);
+      }
+      webProgress.removeProgressListener(loadListener);
+      sendError(new TimeoutError("Error loading page, timed out (onDOMContentLoaded)"), command_id);
+    };
+    navTimer.initWithCallback(onTimeout, pageTimeout, Ci.nsITimer.TYPE_ONE_SHOT);
+  }
+
+  if (loadEventExpected) {
+    addEventListener("DOMContentLoaded", onDOMContentLoaded, false);
+  }
+  curContainer.frame.location = requestedURL;
+  if (!loadEventExpected) {
+    sendOk(command_id);
+  }
 }
 
 /**
- * Cause the browser to traverse one step backward in the joint history
+ * Cancel the polling and remove the event listener associated with a
- * of the current browsing context.
+ * current navigation request in case we're interupted by an onbeforeunload
- *
+ * handler and navigation doesn't complete.
- * @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.
  */
-function goBack(msg) {
+function cancelRequest() {
-  let {command_id, pageTimeout} = msg.json;
+  navTimer.cancel();
-
+  if (onDOMContentLoaded) {
-  loadListener.navigate(() => {
+    removeEventListener("DOMContentLoaded", onDOMContentLoaded, false);
-    curContainer.frame.history.back();
+  }
-  }, command_id, pageTimeout);
-}
-
-/**
- * Cause the browser to traverse one step forward in the joint history
- * of the current browsing context.
- *
- * @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.
- */
-function goForward(msg) {
-  let {command_id, pageTimeout} = msg.json;
-
-  loadListener.navigate(() => {
-    curContainer.frame.history.forward();
-  }, command_id, pageTimeout);
 }
 
 /**
@@ -1162,6 +1152,117 @@ function getPageSource() {
   return curContainer.frame.document.documentElement.outerHTML;
 }
 
+/**
+ * Wait for the current page to be unloaded after a navigation got triggered.
+ *
+ * @param {function} trigger
+ *     Callback to execute which triggers a page navigation.
+ * @param {function} doneCallback
+ *     Callback to execute when the current page has been unloaded.
+ *
+ *     It receives a dictionary with the following items as argument:
+ *         loading - Flag if a page load will follow.
+ *         lastSeenURL - Last seen URL before the navigation request.
+ *         startTime - Time when the navigation request has been triggered.
+ */
+function waitForPageUnloaded(trigger, doneCallback) {
+  let currentURL = curContainer.frame.location.href;
+  let start = new Date().getTime();
+
+  function handleEvent(event) {
+    // In case of a remoteness change it can happen that we are no longer able
+    // to access the document's location. In those cases ignore the event,
+    // but keep the code waiting, and assume in the driver that waiting for the
+    // page load is necessary. Bug 1333458 should improve things.
+    if (typeof event.originalTarget.location == "undefined") {
+      return;
+    }
+
+    switch (event.type) {
+      case "hashchange":
+        removeEventListener("hashchange", handleEvent);
+        removeEventListener("pagehide", handleEvent);
+        removeEventListener("unload", handleEvent);
+
+        doneCallback({loading: false, lastSeenURL: currentURL});
+        break;
+
+      case "pagehide":
+      case "unload":
+        if (event.originalTarget === curContainer.frame.document) {
+          removeEventListener("hashchange", handleEvent);
+          removeEventListener("pagehide", handleEvent);
+          removeEventListener("unload", handleEvent);
+
+          doneCallback({loading: true, lastSeenURL: currentURL, startTime: start});
+        }
+        break;
+    }
+  }
+
+  addEventListener("hashchange", handleEvent, false);
+  addEventListener("pagehide", handleEvent, false);
+  addEventListener("unload", handleEvent, false);
+
+  trigger();
+}
+
+/**
+ * Cause the browser to traverse one step backward in the joint history
+ * of the current browsing context.
+ *
+ * @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.
+ */
+function goBack(msg) {
+  let {command_id, pageTimeout} = msg.json;
+
+  waitForPageUnloaded(() => {
+      curContainer.frame.history.back();
+    }, pageLoadStatus => {
+    if (pageLoadStatus.loading) {
+      pollForReadyState({json: {
+        command_id: command_id,
+        lastSeenURL: pageLoadStatus.lastSeenURL,
+        pageTimeout: pageTimeout,
+        startTime: pageLoadStatus.startTime,
+      }});
+    } else {
+      sendOk(command_id);
+    }
+  });
+}
+
+/**
+ * Cause the browser to traverse one step forward in the joint history
+ * of the current browsing context.
+ *
+ * @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.
+ */
+function goForward(msg) {
+  let {command_id, pageTimeout} = msg.json;
+
+  waitForPageUnloaded(() => {
+    curContainer.frame.history.forward();
+  }, pageLoadStatus => {
+    if (pageLoadStatus.loading) {
+      pollForReadyState({json: {
+        command_id: command_id,
+        lastSeenURL: pageLoadStatus.lastSeenURL,
+        pageTimeout: pageTimeout,
+        startTime: pageLoadStatus.startTime,
+      }});
+    } else {
+      sendOk(command_id);
+    }
+  });
+}
+
 /**
  * Refresh the page
  */

testing/marionette/navigate.js

@@ -14,30 +14,106 @@ this.navigate = {};
 
 /**
  * Determines if we expect to get a DOM load event (DOMContentLoaded)
- * on navigating to the |url|.
+ * on navigating to the |future| URL.
  *
- * @param {string} url
+ * @param {string} current
- *     Destination URL
+ *     URL the browser is currently visiting.
+ * @param {string=} future
+ *     Destination URL, if known.
  *
  * @return {boolean}
- *     Full page load would be expected if url gets loaded.
+ *     Full page load would be expected if future is followed.
  *
  * @throws TypeError
- *     If |url| is an invalid URL.
+ *     If |current| is not defined, or any of |current| or |future|
+ *     are invalid URLs.
  */
-navigate.isLoadEventExpected = function (url) {
+navigate.isLoadEventExpected = function (current, future = undefined) {
-  // assume we will go somewhere exciting
+  if (typeof current == "undefined") {
-  if (typeof url == "undefined") {
+    throw TypeError("Expected at least one URL");
-    throw TypeError("Expected destination URL");
   }
 
-  switch (new URL(url).protocol) {
+  // assume we will go somewhere exciting
-    // assume javascript:<whatever> will modify current document
+  if (typeof future == "undefined") {
-    // but this is not an entirely safe assumption to make,
+    return true;
-    // considering it could be used to set window.location
+  }
-    case "javascript:":
+
-      return false;
+  let cur = new navigate.IdempotentURL(current);
+  let fut = new navigate.IdempotentURL(future);
+
+  // assume javascript:<whatever> will modify current document
+  // but this is not an entirely safe assumption to make,
+  // considering it could be used to set window.location
+  if (fut.protocol == "javascript:") {
+    return false;
+  }
+
+  // navigating to same url, but with any hash
+  if (cur.origin == fut.origin &&
+      cur.pathname == fut.pathname &&
+      fut.hash != "") {
+    return false;
   }
 
   return true;
 };
+
+/**
+ * Sane URL implementation that normalises URL fragments (hashes) and
+ * path names for "data:" URLs, and makes them idempotent.
+ *
+ * At the time of writing this, the web is approximately 10 000 days (or
+ * ~27.39 years) old.  One should think that by this point we would have
+ * solved URLs.  The following code is prudent example that we have not.
+ *
+ * When a URL with a fragment identifier but no explicit name for the
+ * fragment is given, i.e. "#", the {@code hash} property a {@code URL}
+ * object computes is an empty string.  This is incidentally the same as
+ * the default value of URLs without fragments, causing a lot of confusion.
+ *
+ * This means that the URL "http://a/#b" produces a hash of "#b", but that
+ * "http://a/#" produces "".  This implementation rectifies this behaviour
+ * by returning the actual full fragment, which is "#".
+ *
+ * "data:" URLs that contain fragments, which if they have the same origin
+ * and path name are not meant to cause a page reload on navigation,
+ * confusingly adds the fragment to the {@code pathname} property.
+ * This implementation remedies this behaviour by trimming it off.
+ *
+ * The practical result of this is that while {@code URL} objects are
+ * not idempotent, the returned URL elements from this implementation
+ * guarantees that |url.hash == url.hash|.
+ *
+ * @param {string|URL} o
+ *     Object to make an URL of.
+ *
+ * @return {navigate.IdempotentURL}
+ *     Considered by some to be a somewhat saner URL.
+ *
+ * @throws TypeError
+ *     If |o| is not a valid type or if is a string that cannot be parsed
+ *     as a URL.
+ */
+navigate.IdempotentURL = function (o) {
+  let url = new URL(o);
+
+  let hash = url.hash;
+  if (hash == "" && url.href[url.href.length - 1] == "#") {
+    hash = "#";
+  }
+
+  return {
+    hash: hash,
+    host: url.host,
+    hostname: url.hostname,
+    href: url.href,
+    origin: url.origin,
+    password: url.password,
+    pathname: url.pathname,
+    port: url.port,
+    protocol: url.protocol,
+    search: url.search,
+    searchParams: url.searchParams,
+    username: url.username,
+  };
+};

testing/marionette/test_navigate.js

@@ -10,10 +10,58 @@ Cu.import("chrome://marionette/content/navigate.js");
 
 add_test(function test_isLoadEventExpected() {
   Assert.throws(() => navigate.isLoadEventExpected(undefined),
-      /Expected destination URL/);
+      /Expected at least one URL/);
 
   equal(true, navigate.isLoadEventExpected("http://a/"));
-  equal(false, navigate.isLoadEventExpected("javascript:whatever"));
+  equal(true, navigate.isLoadEventExpected("http://a/", "http://a/"));
+  equal(true, navigate.isLoadEventExpected("http://a/", "http://a/b"));
+  equal(true, navigate.isLoadEventExpected("http://a/", "http://b"));
+  equal(true, navigate.isLoadEventExpected("http://a/", "data:text/html;charset=utf-8,foo"));
+  equal(true, navigate.isLoadEventExpected("about:blank", "http://a/"));
+  equal(true, navigate.isLoadEventExpected("http://a/", "about:blank"));
+  equal(true, navigate.isLoadEventExpected("http://a/", "https://a/"));
+
+  equal(false, navigate.isLoadEventExpected("http://a/", "javascript:whatever"));
+  equal(false, navigate.isLoadEventExpected("http://a/", "http://a/#"));
+  equal(false, navigate.isLoadEventExpected("http://a/", "http://a/#b"));
+  equal(false, navigate.isLoadEventExpected("http://a/#b", "http://a/#b"));
+  equal(false, navigate.isLoadEventExpected("http://a/#b", "http://a/#c"));
+  equal(false, navigate.isLoadEventExpected("data:text/html;charset=utf-8,foo", "data:text/html;charset=utf-8,foo#bar"));
+  equal(false, navigate.isLoadEventExpected("data:text/html;charset=utf-8,foo", "data:text/html;charset=utf-8,foo#"));
+
+  run_next_test();
+});
+
+add_test(function test_IdempotentURL() {
+  Assert.throws(() => new navigate.IdempotentURL(undefined));
+  Assert.throws(() => new navigate.IdempotentURL(true));
+  Assert.throws(() => new navigate.IdempotentURL({}));
+  Assert.throws(() => new navigate.IdempotentURL(42));
+
+  // propagated URL properties
+  let u1 = new URL("http://a/b");
+  let u2 = new navigate.IdempotentURL(u1);
+  equal(u1.host, u2.host);
+  equal(u1.hostname, u2.hostname);
+  equal(u1.href, u2.href);
+  equal(u1.origin, u2.origin);
+  equal(u1.password, u2.password);
+  equal(u1.port, u2.port);
+  equal(u1.protocol, u2.protocol);
+  equal(u1.search, u2.search);
+  equal(u1.username, u2.username);
+
+  // specialisations
+  equal("#b", new navigate.IdempotentURL("http://a/#b").hash);
+  equal("#", new navigate.IdempotentURL("http://a/#").hash);
+  equal("", new navigate.IdempotentURL("http://a/").hash);
+  equal("#bar", new navigate.IdempotentURL("data:text/html;charset=utf-8,foo#bar").hash);
+  equal("#", new navigate.IdempotentURL("data:text/html;charset=utf-8,foo#").hash);
+  equal("", new navigate.IdempotentURL("data:text/html;charset=utf-8,foo").hash);
+
+  equal("/", new navigate.IdempotentURL("http://a/").pathname);
+  equal("/", new navigate.IdempotentURL("http://a/#b").pathname);
+  equal("text/html;charset=utf-8,foo", new navigate.IdempotentURL("data:text/html;charset=utf-8,foo#bar").pathname);
 
   run_next_test();
 });