Bug 1446551 - "too much recursion" when pressing enter in location bar. r=mak
authorDrew Willcoxon <adw@mozilla.com>
Wed, 18 Apr 2018 16:21:18 -0700
changeset 468041 2fefab7d7dc192cdaf66d88a90b76e763b762465
parent 468040 d42b55d603032606bc40ae6b24a0aa27e6d30c72
child 468042 96867a07e1a37081062f2f4adbae2b20f46b48fa
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1446551
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1446551 - "too much recursion" when pressing enter in location bar. r=mak MozReview-Commit-ID: C8xhlyXOVC2
browser/base/content/urlbarBindings.xml
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -250,16 +250,17 @@ file, You can obtain one at http://mozil
             this.removeAttribute("actiontype");
           }
           return returnValue;
         ]]></body>
       </method>
 
       <method name="onKeyPress">
         <parameter name="aEvent"/>
+        <parameter name="aNoDefer"/>
         <body><![CDATA[
           switch (aEvent.keyCode) {
             case KeyEvent.DOM_VK_LEFT:
             case KeyEvent.DOM_VK_RIGHT:
             case KeyEvent.DOM_VK_HOME:
               // Reset the selected index so that nsAutoCompleteController
               // simply closes the popup without trying to fill anything.
               this.popup.selectedIndex = -1;
@@ -271,17 +272,17 @@ file, You can obtain one at http://mozil
             case KeyEvent.DOM_VK_DOWN:
             case KeyEvent.DOM_VK_PAGE_UP:
             case KeyEvent.DOM_VK_PAGE_DOWN:
               if (this.userSelectionBehavior != "tab")
                 this.userSelectionBehavior = "arrow";
               break;
           }
           if (!this.popup.disableKeyNavigation) {
-            if (this._shouldDeferKeyEvent(aEvent)) {
+            if (!aNoDefer && this._shouldDeferKeyEvent(aEvent)) {
               this._deferKeyEvent(aEvent, "onKeyPress");
               return false;
             }
             if (this.popup.popupOpen && this.popup.handleKeyPress(aEvent)) {
               return true;
             }
           }
           return this.handleKeyPress(aEvent);
@@ -304,19 +305,19 @@ file, You can obtain one at http://mozil
                 The key event that should maybe be deferred.
         @return True if the event should be deferred, false if not.
        -->
       <method name="_shouldDeferKeyEvent">
         <parameter name="event"/>
         <body><![CDATA[
           // If any event has been deferred for this search, then defer all
           // subsequent events so that the user does not experience any
-          // keypresses out of order.  All events will be replayed when this
-          // timeout fires.
-          if (this._deferredKeyEventTimeout) {
+          // keypresses out of order.  All events will be replayed when
+          // _deferredKeyEventTimeout fires.
+          if (this._deferredKeyEventQueue.length) {
             return true;
           }
 
           // At this point, no events have been deferred for this search, and we
           // need to decide whether `event` is the first one that should be.
 
           if (!this._keyCodesToDefer.has(event.keyCode)) {
             // Not a key that should trigger deferring.
@@ -351,16 +352,21 @@ file, You can obtain one at http://mozil
 
         @param  event
                 The key event.
         @return True if the event can be played, false if not.
       -->
       <method name="_safeToPlayDeferredKeyEvent">
         <parameter name="event"/>
         <body><![CDATA[
+          if (event.keyCode == KeyEvent.DOM_VK_RETURN) {
+            return this.popup.selectedIndex != 0 ||
+                   this.gotResultForCurrentQuery;
+          }
+
           if (!this.gotResultForCurrentQuery || !this.popupOpen) {
             // We're still waiting on the first result, or the popup hasn't
             // opened yet, so not safe.
             return false;
           }
 
           let maxResultsRemaining =
             this.popup.maxResults - this.popup.matchCount;
@@ -385,28 +391,33 @@ file, You can obtain one at http://mozil
 
       <!--
         Adds a key event to the deferred event queue.
 
         @param event
                The key event to defer.
         @param methodName
                The name of the method on `this` to call.  It's expected to take
-               a single argument, the event.
+               two arguments: the event, and a noDefer bool.  If the bool is
+               true, then the event is being replayed and it should not be
+               deferred.
       -->
       <method name="_deferKeyEvent">
         <parameter name="event"/>
         <parameter name="methodName"/>
         <body><![CDATA[
           // Somehow event.defaultPrevented ends up true for deferred events.
           // autocomplete ignores defaultPrevented events, which means it would
           // ignore replayed deferred events if we didn't tell it to bypass
           // defaultPrevented.  That's the purpose of this expando.  If we could
           // figure out what's setting defaultPrevented and prevent it, then we
           // could get rid of this.
+          if (event.urlbarDeferred) {
+            throw new Error("Key event already deferred!");
+          }
           event.urlbarDeferred = true;
 
           this._deferredKeyEventQueue.push({
             methodName,
             event,
             searchString: this.mController.searchString,
           });
 
@@ -414,30 +425,27 @@ file, You can obtain one at http://mozil
             // Start the timeout that will unconditionally replay all deferred
             // events when it fires so that, after a certain point, we don't
             // keep blocking the user's keypresses when nothing else has caused
             // the events to be replayed.  Do not check whether it's safe to
             // replay the events because otherwise it may look like we ignored
             // the user's input.
             let elapsed = Cu.now() - this._searchStartDate;
             let remaining = this._deferredKeyEventTimeoutMs - elapsed;
-            if (remaining <= 0) {
+            this._deferredKeyEventTimeout = setTimeout(() => {
               this.replayAllDeferredKeyEvents();
-            } else {
-              this._deferredKeyEventTimeout = setTimeout(() => {
-                this.replayAllDeferredKeyEvents();
-                this._deferredKeyEventTimeout = null;
-              }, remaining);
-            }
+              this._deferredKeyEventTimeout = null;
+            }, Math.max(0, remaining));
           }
         ]]></body>
       </method>
 
       <!-- The enter key is always deferred, so it's not included here. -->
       <field name="_keyCodesToDefer">new Set([
+        KeyboardEvent.DOM_VK_RETURN,
         KeyboardEvent.DOM_VK_DOWN,
         KeyboardEvent.DOM_VK_TAB,
       ])</field>
       <field name="_deferredKeyEventQueue">[]</field>
       <field name="_deferredKeyEventTimeout">null</field>
       <field name="_deferredKeyEventTimeoutMs">200</field>
       <field name="_searchStartDate">0</field>
 
@@ -477,17 +485,17 @@ file, You can obtain one at http://mozil
         ]]></body>
       </method>
 
       <method name="_replayKeyEventInstance">
         <parameter name="instance"/>
         <body><![CDATA[
           // Safety check: handle only if the search string didn't change.
           if (this.mController.searchString == instance.searchString) {
-            this[instance.methodName](instance.event);
+            this[instance.methodName](instance.event, true);
           }
         ]]></body>
       </method>
 
       <field name="_mayTrimURLs">true</field>
       <method name="trimValue">
         <parameter name="aURL"/>
         <body><![CDATA[
@@ -1476,16 +1484,17 @@ file, You can obtain one at http://mozil
             }
           }
           this.resetActionType();
         ]]></body>
       </method>
 
       <method name="handleEnter">
         <parameter name="event"/>
+        <parameter name="noDefer"/>
         <body><![CDATA[
           // We need to ensure we're using a selected autocomplete result.
           // A result should automatically be selected by default,
           // however autocomplete is async and therefore we may not
           // have a result set relating to the current input yet. If that
           // happens, we need to mark that when the first result does get added,
           // it needs to be handled as if enter was pressed with that first
           // result selected.
@@ -1495,40 +1504,39 @@ file, You can obtain one at http://mozil
           // input.
           // However, if the default result is automatically selected, we
           // ensure that it corresponds to the current input.
 
           // Store the current search string so it can be used in handleCommand,
           // which will be called as a result of mController.handleEnter().
           this.handleEnterSearchString = this.mController.searchString;
 
-          if (!this._deferredKeyEventQueue.length &&
-              (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery)) {
-            let canonizeValue = this.value;
-            if (event.shiftKey || (AppConstants.platform === "macosx" ?
-                                   event.metaKey :
-                                   event.ctrlKey)) {
-              let action = this._parseActionUrl(canonizeValue);
-              if (action && "searchSuggestion" in action.params) {
-                canonizeValue = action.params.searchSuggestion;
-              } else if (this.popup.selectedIndex === 0 &&
-                         this.mController.getStyleAt(0).includes("autofill")) {
-                canonizeValue = this.handleEnterSearchString;
-              }
-            }
-            this.maybeCanonizeURL(event, canonizeValue);
-            let handled = this.mController.handleEnter(false, event);
-            this.handleEnterSearchString = null;
-            this.popup.overrideValue = null;
-            return handled;
+          if (!noDefer && this._shouldDeferKeyEvent(event)) {
+            // Defer the event until the first non-heuristic result comes in.
+            this._deferKeyEvent(event, "handleEnter");
+            return false;
           }
 
-          // Defer the event until the first non-heuristic result comes in.
-          this._deferKeyEvent(event, "handleEnter");
-          return false;
+          let canonizeValue = this.value;
+          if (event.shiftKey || (AppConstants.platform === "macosx" ?
+                                 event.metaKey :
+                                 event.ctrlKey)) {
+            let action = this._parseActionUrl(canonizeValue);
+            if (action && "searchSuggestion" in action.params) {
+              canonizeValue = action.params.searchSuggestion;
+            } else if (this.popup.selectedIndex === 0 &&
+                       this.mController.getStyleAt(0).includes("autofill")) {
+              canonizeValue = this.handleEnterSearchString;
+            }
+          }
+          this.maybeCanonizeURL(event, canonizeValue);
+          let handled = this.mController.handleEnter(false, event);
+          this.handleEnterSearchString = null;
+          this.popup.overrideValue = null;
+          return handled;
         ]]></body>
       </method>
 
       <method name="handleDelete">
         <body><![CDATA[
           // If the heuristic result is selected, then the autocomplete
           // controller's handleDelete implementation will remove it, which is
           // not what we want.  So in that case, call handleText so it acts as