Closed
Bug 1438329
Opened 6 years ago
Closed 6 years ago
Problem(s) with delayed key handling in urlbar
Categories
(Firefox :: Address Bar, enhancement, P1)
Firefox
Address Bar
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.
Assignee | ||
Comment 1•6 years ago
|
||
(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.
Comment 2•6 years ago
|
||
why don't we use a monotonic timer here?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
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
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5f116192fa38799923199c2e8718d3a57692309
Comment 6•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Priority: -- → P1
Whiteboard: [fxsearch]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
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
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cbad25b1fee3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in
before you can comment on or make changes to this bug.
Description
•