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)

defect

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
We should investigate this soon.
Priority: -- → P1
Whiteboard: [fxsearch]
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
I don't think this is right.  There's some deeper problem here.  I'm trying to think through it.
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
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 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+
Assignee: mak77 → adw
Attachment #8968172 - Attachment is obsolete: true
Attachment #8968172 - Flags: review?(adw)
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.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2fefab7d7dc1
"too much recursion" when pressing enter in location bar. r=mak
https://hg.mozilla.org/mozilla-central/rev/2fefab7d7dc1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.
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.
> 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.
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.
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.