Bug 1361983 - Extend page unload timer if flushEventLoop returns after beforeunload event. draft
authorHenrik Skupin <mail@hskupin.info>
Thu, 04 May 2017 11:53:30 +0200
changeset 574186 3a9ba089e80d7b727acf1be6c9ad52e59b15c9b7
parent 574065 1fda52a1f3b81cf1a821155998dca637bb64e3d9
child 627496 3c4a4edd1f3d1cb6182d275d8f7c2898876ae34e
push id57600
push userbmo:hskupin@gmail.com
push dateMon, 08 May 2017 12:35:10 +0000
bugs1361983
milestone55.0a1
Bug 1361983 - Extend page unload timer if flushEventLoop returns after beforeunload event. In cases when the navigation trigger function returns too late and the beforeunload event has already been fired, the page load currently gets canceled. This happens because the page unload timer hasn't been set at this time and the unbeforeunload handler doesn't extend its time. With this patch a flag is used which indicates if an beforeunload event already occurred, so when the pageunload timer notifies the page listener the unload timer can be extended to 5s as expected. MozReview-Commit-ID: FKK0oGEWijU
testing/marionette/harness/marionette_harness/tests/unit/test_click.py
testing/marionette/harness/marionette_harness/www/clicks.html
testing/marionette/listener.js
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_click.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_click.py
@@ -332,16 +332,20 @@ class TestClickNavigation(MarionetteTest
             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)
 
+    def test_click_option_navigate(self):
+        self.marionette.find_element(By.ID, "option").click()
+        self.marionette.find_element(By.ID, "delay")
+
     @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):
--- a/testing/marionette/harness/marionette_harness/www/clicks.html
+++ b/testing/marionette/harness/marionette_harness/www/clicks.html
@@ -33,10 +33,13 @@
 <div id="addbuttonlistener" onclick="var el = document.getElementById('showbutton');el.addEventListener('mousedown', function (evt) { document.getElementById('showbutton').innerHTML = evt.button; }, false);">
   click to add an event listener
 </div>
 <div id="showbutton">
   this will show the button clicked
 </div>
 <a href="xhtmlTest.html">333333</a>
 <p><a href="xhtmlTest.html" id="embeddedBlock"><span style="display: block;">I have a span</span><div>And a div</div><span>And another span</span></a></p>
+<select id="option" onclick="window.location = '/slow?delay=1'">
+  <option>Click to navigate</option>
+</select>
 </body>
 </html>
--- a/testing/marionette/listener.js
+++ b/testing/marionette/listener.js
@@ -116,17 +116,18 @@ 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,
-  seenUnload: null,
+  seenBeforeUnload: false,
+  seenUnload: false,
   timeout: null,
   timerPageLoad: null,
   timerPageUnload: null,
 
   /**
    * Start listening for page unload/load events.
    *
    * @param {number} command_id
@@ -137,16 +138,17 @@ var loadListener = {
    *     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;
 
+    this.seenBeforeUnload = false;
     this.seenUnload = false;
 
     this.timerPageLoad = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
     this.timerPageUnload = null;
 
     // In case of a remoteness change, only wait the remaining time
     timeout = startTime + timeout - new Date().getTime();
 
@@ -208,27 +210,17 @@ var loadListener = {
    * Callback for registered DOM events.
    */
   handleEvent: function (event) {
     let location = event.target.baseURI || event.target.location.href;
     logger.debug(`Received DOM event "${event.type}" for "${location}"`);
 
     switch (event.type) {
       case "beforeunload":
-        if (this.timerPageUnload) {
-          // In the case when a document has a beforeunload handler registered,
-          // the currently active command will return immediately due to the
-          // modal dialog observer in proxy.js.
-          // Otherwise the timeout waiting for the document to start navigating
-          // is increased by 5000 ms to ensure a possible load event is not missed.
-          // In the common case such an event should occur pretty soon after
-          // beforeunload, and we optimise for this.
-          this.timerPageUnload.cancel();
-          this.timerPageUnload.initWithCallback(this, 5000, Ci.nsITimer.TYPE_ONE_SHOT)
-        }
+        this.seenBeforeUnload = true;
         break;
 
       case "unload":
         this.seenUnload = true;
         break;
 
       case "pagehide":
         if (event.target === curContainer.frame.document) {
@@ -277,20 +269,31 @@ 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) {
+        // In the case when a document has a beforeunload handler registered,
+        // the currently active command will return immediately due to the
+        // modal dialog observer in proxy.js.
+        // Otherwise the timeout waiting for the document to start navigating
+        // is increased by 5000 ms to ensure a possible load event is not missed.
+        // In the common case such an event should occur pretty soon after
+        // beforeunload, and we optimise for this.
+        if (this.seenBeforeUnload) {
+          this.seenBeforeUnload = null;
+          this.timerPageUnload.initWithCallback(this, 5000, Ci.nsITimer.TYPE_ONE_SHOT)
+
+        // If no page unload has been detected, ensure to properly stop the load
+        // listener, and return from the currently active command.
+        } else 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: