Closed Bug 1438329 Opened 2 years ago Closed 2 years ago

Problem(s) with delayed key handling in urlbar

Categories

(Firefox :: Address Bar, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: adw, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

I'm investigating why browser/base/content/test/urlbar/browser_autocomplete_enter_race.js fails when the Gecko timer precision changed to 2ms.  Info here: https://docs.google.com/document/d/1CqND7I0Z_C8k8qx6hQNIDxbHRq3lBP6e1BKTrI3e3yk/edit#heading=h.a3ayihnys1gi

There are failures in two of the test tasks.  I haven't looked at the second in depth yet, but the first is a real problem in Firefox, not a test problem.  (My initial impression is that the second is, too.)  It's that test_keyword hangs because we end up never replaying a deferred keypress.  It looks like it's because when the replay timeout fires, the age calculation used to tell whether a deferred keypress is old enough to replay is slightly off.  i.e., at the point that the timeout fires, the age should be <= the period of the timeout, but it's > instead.  The timeout is never restarted, so the deferred keypress is never handled.

This kind of failure -- where we're using Date.now() to calculate an age and we're using timeouts -- is definitely plausible given this change in Gecko timers.  It might be enough to make sure the replay timeout keeps going as long as there are deferred keypresses.  But I'd like to go back and re-evaluate why we have this ferring mechanism at all so that we could ideally get rid of it.  The general deferred-keypress mechanism grew out of the older deferred-enter-key mechansim, which looks like was introduced here: https://hg.mozilla.org/mozilla-central/rev/6c9f72338190

Incidentally we have bug 1408398 (and maybe others?  Seems like there were one or two more) and this test failure probably confirms that they're real, and that users are seeing this.
Blocks: 1427166
(In reply to Drew Willcoxon :adw from comment #0)
> i.e., at the point that the timeout fires, the age should be <= the period
> of the timeout, but it's > instead.

Actually we check <, not <=: https://dxr.mozilla.org/mozilla-central/rev/e43f2f6ea111c2d059d95fa9a71516b869a69698/browser/base/content/urlbarBindings.xml#304

When I change it to <=, the whole test passes for me locally (also commenting out the reduceTimerPrecision parts).  :-O

I still think this code is fragile though...  If the difference here is only ~1 ms, seems like it could still easily fail on some machines.  So I'll continue to look at this.

> But I'd like to go back and re-evaluate why we have this [de]ferring mechanism
> at all so that we could ideally get rid of it.

I looked at it, and I guess it really is necessary.  I think we have only a couple of options if we wanted to remove it.  I'm not advocating them, only thinking through how we would do it.

(1) Return an initial/heuristic result syncly instead of asyncly.  Sub-options:

  (1a) Query the Places DB (among other things) syncly instead of asyncly, but we'd maybe jank the UI.

  (1b) Redesign things so we kept everything in memory that we needed to generate the first result, but that would probably never be feasible.

  (1c) Syncly return an initial result based only on info that is available syncly, and then go ahead and do the remainder async part and replace the result when the async part finishes.  But that still has the problem of sending the user to a URL they didn't expect.

(2) Bypass the search results entirely if the user hits the enter key before the first result comes in.  i.e., call URI fixup or whatever on what the user managed to type and do a search or visit a URL.  But that has the same problem of sending the user to a URL they didn't expect.
why don't we use a monotonic timer here?
I've tried to make this saner...  Summary:

* Move the _keyCodesToDefer check into _shouldDeferKeyEvent, where it belongs

* If we've deferred any key event, then defer all subsequent events for as long as we need to keep deferring.  As it is, it's possible for events to happen out of order.

* Split _shouldDeferKeyEvent into two methods, one that's for determining whether we should start deferring (_shouldDeferKeyEvent), one that's for determining whether we can (re)play an event given the current state of the popup (_safeToPlayDeferredKeyEvent).  Call the latter when results come in to tell whether we can replay deferred events.

* Make it clear that when the timeout fires, we must unconditionally replay all deferred key events.  No date comparisons.

* Remove the workarounds from the test in question
Comment on attachment 8951498 [details]
Bug 1438329 - Problem(s) with delayed key handling in urlbar.

https://reviewboard.mozilla.org/r/220802/#review226862

This is scary, but the original code is not better.
Let's land this asap so we get longer Nightly testing.

::: browser/base/content/urlbarBindings.xml:407
(Diff revision 1)
>  
>            if (!this._deferredKeyEventTimeout) {
> +            // 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.

maybe this comment could clarify that we avoid checking safety of the full replay because otherwise it may look like we ignored the user's input.
For a while I was trying to understand why we couldn't check if we CAN replay all the events, then I remembered we discussed it and it's better to do somethign rather than completely ignoring the user.

::: browser/base/content/urlbarBindings.xml:456
(Diff revision 1)
>            let instance = this._deferredKeyEventQueue.shift();
> +          if (!instance) {
> +            return;
> +          }
> +          this._replayKeyEventInstance(instance);
> +          setTimeout(() => {

timeouts may be clamped and make this slower than needed, if it's just to move to the next tick maybe better Services.tm.dispatchToMainThread
Attachment #8951498 - Flags: review?(mak77) → review+
Priority: -- → P1
Whiteboard: [fxsearch]
Thanks, fixed
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cbad25b1fee3
Problem(s) with delayed key handling in urlbar. r=mak
https://hg.mozilla.org/mozilla-central/rev/cbad25b1fee3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Blocks: 1435990
See Also: → 1547826
You need to log in before you can comment on or make changes to this bug.