Bug 1361983 - Extend page unload timer if flushEventLoop returns after beforeunload event. r=ato, a=test-only
authorHenrik Skupin <mail@hskupin.info>
Thu, 04 May 2017 11:53:30 +0200
changeset 396339 e2018fdeb7fa5853d556ea1a864ccb54dcf4e2f4
parent 396338 b99a30853f5325560cf3daf91d2b981d7e002990
child 396340 eedfc71718b4abd80fabcecf37eb122ef8aaef1a
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato, test-only
bugs1361983
milestone54.0
Bug 1361983 - Extend page unload timer if flushEventLoop returns after beforeunload event. r=ato, a=test-only 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
@@ -297,16 +297,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: