Closed
Bug 1446551
Opened 6 years ago
Closed 6 years ago
"too much recursion" when pressing enter in location bar
Categories
(Firefox :: Address Bar, defect, P1)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Oriol, Assigned: adw)
References
Details
(Whiteboard: [fxsearch])
Attachments
(1 file, 1 obsolete file)
Sometimes I click the location bar and press enter, but the page does not reload. Sorry I don't have more precise steps to reproduce. When it happens, in the browser console I get this stack trace each time I press enter. too much recursion next self-hosted:562:9 from self-hosted:597:9 get_children chrome://global/content/bindings/richlistbox.xml:325:28 getIndexOfItem chrome://global/content/bindings/richlistbox.xml:181:13 get_selectedIndex chrome://global/content/bindings/listbox.xml:100:20 get_selectedIndex chrome://global/content/bindings/autocomplete.xml:747:1 handleEnter chrome://browser/content/urlbarBindings.xml:1505:1 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13 replayAllDeferredKeyEvents chrome://browser/content/urlbarBindings.xml:474:11 _deferKeyEvent chrome://browser/content/urlbarBindings.xml:424:15 handleEnter chrome://browser/content/urlbarBindings.xml:1526:11 _replayKeyEventInstance chrome://browser/content/urlbarBindings.xml:486:13
Reporter | ||
Comment 1•6 years ago
|
||
Permalinks: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/urlbarBindings.xml#473 https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/urlbarBindings.xml#423 https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/urlbarBindings.xml#1525 https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/urlbarBindings.xml#485
Comment 3•6 years ago
|
||
Surely there's something wrong here. handleEnter may invoke _deferKeyEvent, that adds the event to the queue, then it may invoke replayAllDeferredKeyEvents if the timer has elapsed, that goes through all the queue, included this event itself since it has been added... but if the reasons that first caused the event to be delayed are still there (and this is all synchronous), it will try to defer again and again... There are various possibilities to explore: 1. empty the queue before forcibly replaying all the events, so handleEnter just executes * this may be fragile 2. add an artificial 0-delay when invoking replayAllDeferredKeyEvents, so we give the initial condition some time to disappear * maybe we should do this regardless, to avoid very long stacks 3. add a "force" argument to handleEnter and onKeyPress, so they don't defer if it's set * a little bit brute-force, but it may be safer 4. change handleEnter to use _shouldDeferKeyEvent instead of the custom "!this._deferredKeyEventQueue.length" check it currently has * probably nice to do, unclear if it may have downsides, due to complexity I'd probably try 2+4... and eventually fallback to 2+3 if it creates issues.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
I don't think this is right. There's some deeper problem here. I'm trying to think through it.
Comment 6•6 years ago
|
||
fwiw, I didn't notice problems in my manual testing, and surely the infinite recursion comes from having everything in the same tick (indeed it's impossible for the delay cause to go away if we just keep looping). But since you worked more on this deferring code, you surely have a clearer picture in mind
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
Sorry for hijacking the bug with my own patch, but writing a patch was the only way to think through this. I think the "force" idea is the right one. Normally, when a key is pressed, we should queue the press if the queue is non-empty so that events are played in the right order. But when we replay an individual event, we must replay it regardless of the queue, about which we shouldn't make any assumptions. The thing that's hard to think through with this bug is that _deferKeyEvent pushes the event on the queue, but then when replayAllDeferredKeyEvents is immediately called, it should shift that event off the queue, and the queue should become empty, so when the handleEnter is replayed, it should see that the queue is empty and handle the event normally. That's not happening, the queue isn't empty. This makes some other fixes: _shouldDeferKeyEvent should check the queue length, not the timeout, to determine whether "any event has been deferred for this search." The timeout is nulled as soon as it fires, but events in the queue are replayed asyncly -- so the timeout is nulled before all queued events are replayed. _deferKeyEvent should be called at most once for an event. Throw if event.urlbarDeferred has already been set. Simplify _deferKeyEvent by always setting the timeout. If `remaining` <= 0, then set the timeout with a 0ms value instead of calling replayAllDeferredKeyEvents directly. Improve handleEnter: (1) Flip the should-defer conditional so that the early return is the should-defer case. (2) Call _shouldDeferKeyEvent instead of the custom conditional. (3) Add DOM_VK_RETURN to _keyCodesToDefer so that _shouldDeferKeyEvent works right. (4) Modify _safeToPlayDeferredKeyEvent to handle the return/enter key. Regardless of the previous point, _safeToPlayDeferredKeyEvent should handle the return/enter key because replaySafeDeferredKeyEvents, which calls _safeToPlayDeferredKeyEvent, should handle return/enter correctly.
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8969125 [details] Bug 1446551 - "too much recursion" when pressing enter in location bar. https://reviewboard.mozilla.org/r/237840/#review243672 It looks good, thank you for having a deeper look into it. It's unfortunate this is so hard to test :( ::: browser/base/content/urlbarBindings.xml:258 (Diff revision 1) > ]]></body> > </method> > > <method name="onKeyPress"> > <parameter name="aEvent"/> > + <parameter name="aNoDefer"/> Personally, I'd invert this to aCanDefer, because everywhere we do !aNoDefer that is harder to read. Obviously it will require to invert all passed bools. The same for handleEnter. ::: browser/base/content/urlbarBindings.xml:445 (Diff revision 1) > > <!-- 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, not a critical problem to solve now, but I suspect pagedown,pageup should also be deferred.
Attachment #8969125 -
Flags: review?(mak77) → review+
Updated•6 years ago
|
Assignee: mak77 → adw
Updated•6 years ago
|
Attachment #8968172 -
Attachment is obsolete: true
Attachment #8968172 -
Flags: review?(adw)
Assignee | ||
Comment 10•6 years ago
|
||
Thanks Marco. (In reply to Marco Bonardo [::mak] (Away 23 Apr - 1 May) from comment #9) > ::: browser/base/content/urlbarBindings.xml:258 > (Diff revision 1) > > ]]></body> > > </method> > > > > <method name="onKeyPress"> > > <parameter name="aEvent"/> > > + <parameter name="aNoDefer"/> > > Personally, I'd invert this to aCanDefer, I think it's better for the arg to default to false though, because onKeyPress and handleEnter are both called by the base autocomplete binding in autocomplete.xml. It doesn't seem right to modify those calls for the urlbar. So I left this as is. I'll push this to try and then land it.
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20f94582037dc9cd920b0dfd95558542a810b42c
Comment 12•6 years ago
|
||
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2fefab7d7dc1 "too much recursion" when pressing enter in location bar. r=mak
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2fefab7d7dc1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 14•6 years ago
|
||
Are we uplifting this? I just discovered that this is the problem I have all the time where the enter key randomly doesn't work. It happens a lot when Windows is overloaded or when I come back from suspend.
Comment 15•6 years ago
|
||
it's not trivial nor what I'd call 100% safe, but condiered its importance and the fact you can confirm it works as intended, we could surely try an approval request.
Comment 16•6 years ago
|
||
> it's not trivial nor what I'd call 100% safe, but condiered its importance and the fact you can confirm it works as intended, we could surely try an approval request.
I'll do some more testing to confirm the fix. I run beta and I know I hit this a lot.
Assignee | ||
Comment 17•6 years ago
|
||
Thanks Mike, that would be very helpful. I agree with Marco that this isn't 100% safe. In the absence of real-world testing I'd prefer to let it ride the trains especially this late in the cycle.
Comment 18•6 years ago
|
||
Sadly I can't recreate. Ride the trains it shall.
You need to log in
before you can comment on or make changes to this bug.
Description
•