1653436 - Some characters getting clobbered if I type too fast

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Andrew Overholt [:overholt]]

I thought this was a dupe of bug 1652024 (or its dupes) but I'm not getting truncated results and I can still see it in the 2020-07-16 nightly build.

STR

1. have a profile with a large PlacesDB
2. quickly type in a few characters of a history item

Actual

• some characters of what you type remain, sometimes with them half-completing one of your history/bookmarks (like if I type "Wennie" to go my notes for my discussions with Wennie I often end up with wfastmail.com (I used fastmail))

Expected

• no lost characters

Comment 1

[Harry Twyford]

I'll take a look at this, thank you. It's probably a regression from bug 1648468.

Comment 2

[Harry Twyford]

[Tracking Requested - why for this release]:

Interferes with users typing in the Urlbar. Appears to be significantly less severe/widespread than bug 1652024, but fixing it is still important.

Comment 4

[Harry Twyford]

Attachment: Bug 1653436 - Cancel heuristic timer on cancelQuery. r?mak!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fbe7765a2d3
Cancel heuristic timer on cancelQuery. r=mak

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5fbe7765a2d3

Comment 8

[Harry Twyford]

I got a report that this bug is still an issue. Chris and Markus, I'm setting ni? since you opened duped bugs. Are you still seeing your filed issues on the latest Nightly?

Comment 9

[Markus Stange [:mstange]]

Yes, I still see this bug in the latest nightly ("built from https://hg.mozilla.org/mozilla-central/rev/138e7b575614cbfc1e45576a15825f51cb6e6614").

Comment 10

[Harry Twyford]

Thanks, I'll take a look now.

Comment 11

[Marco Bonardo [:mak]]

there is one possible mistake that I'm fixing in https://phabricator.services.mozilla.com/D84657, related to clearing this._autofillResult, but I'm honestly not sure whether it's related with chars loss at all.

Comment 12

[Harry Twyford]

That could be it. If _autofillResult is still valid when a new query is fired, Autofill will return an outdated result, which would overwrite the newer heuristic result from UrlbarProviderHeuristicFallback. Do you mind if I pull out the lines related to clearing this._autofillResult into a patch for this bug so it can land ASAP?

Comment 13

[Marco Bonardo [:mak]]

rebasing should not be a problem.

Comment 14

[Harry Twyford]

Attachment: Bug 1653436 - Clear _autofillResult more reliably. r?mak

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f02ec7a68ff1
Clear _autofillResult more reliably. r=mak

Comment 16

[Markus Stange [:mstange]]

Can we please add a test for this case, too?

Comment 17

[Marco Bonardo [:mak]]

let's first check it really solves the problem for you, then we can brainstorm over a test.

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f02ec7a68ff1

Comment 19

[Marco Bonardo [:mak]]

Markus, please let us know once you're confident about status of this issue.

Comment 20

[Markus Stange [:mstange]]

This is fixed now. Thanks!

Comment 21

[Andrew Overholt [:overholt]]

FWIW I'm still seeing this problem in the 2020-07-28 build of nightly on Windows 10. I'm happy to try debugging locally if it'll help.

Comment 22

[Andrew Overholt [:overholt]]

(In reply to Andrew Overholt [:overholt] from comment #21)

FWIW I'm still seeing this problem in the 2020-07-28 build of nightly on Windows 10. I'm happy to try debugging locally if it'll help.

Harry told me that I'm actually likely seeing bug 1655363.

Comment 23

[Andrew Overholt [:overholt]]

It turns out I can still see this for about: pages and also this very bug (quickly typing in "clobber" sometimes gets me "cobber", etc.).

Comment 24

[Marco Bonardo [:mak]]

I wonder if there's a relation with bug 1655034, doing more I/O may definitely mess up things here. I can't reproduce the most recent problems. Please let us know if things improve once that bug is in Nightly.

Comment 25

[Chris Peterson [:cpeterson]]

FWIW, I can reproduce this bug in 81 Nightly, but not (yet?) in 80 Beta. So perhaps this bug's fix did fix the bug in 80 Nightly but more recent change regressed it in 81 Nightly?

Comment 26

[Marco Bonardo [:mak]]

Which Nightly build exactly? Please test the absolutely latest one available, that's the only one that matters for now.

Comment 28

[Shane Hughes [:aminomancer]]

:mak I submitted the above bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1655936
after testing on: https://hg.mozilla.org/mozilla-central/rev/eba7e3ce93822075543bcc764cef7dbc8e9fc5f1

Comment 29

[Shane Hughes [:aminomancer]]

if you read my bug (the one I just linked) it goes into a lot more detail on STR and potential factors at least from my perspective

Comment 30

[Shane Hughes [:aminomancer]]

I also just tested on the latest available nightly build: https://hg.mozilla.org/mozilla-central/rev/4c1c82402c09bd9b6a6bf57f96edc719ece2b2f3
and immediately experienced the bug within 30 seconds of startup, and I'm getting the bug repeatedly right now, not more frequently or to a higher degree of severity than the previous build but definitely not less.

Comment 31

[Marco Bonardo [:mak]]

The bug you filed imo is different, because it's about losing characters at the beginning of the string when switching tabs. That looks more like a problem of async tab switching and the urlbar reset happening at an unfortunate time. I don't think it's the same thing reported here.

Here the 2 problems I see reported are:

1. losing characters at the end of the string: this was effectively a regression caused by heuristic changes we made in Firefox 80, various fixes landed, I could reproduce the bug easily, but I can't on current Nightly
2. losing characters in the middle of the string: comment 23. My only suspects are bug 1655034, that is fixed in Nightly, and maybe autofill being more reactive while the user types and ending up clobbering something.

Could anyone reproducing this tell us which of these cases you are seing? I think Andrew is seeing 2, maybe also Chris?
Is anyone still seeing 1?

Comment 32

[Andrew Overholt [:overholt]]

(In reply to Marco Bonardo [:mak] from comment #31)

2. losing characters in the middle of the string: comment 23. My only suspects are bug 1655034, that is fixed in Nightly, and maybe autofill being more reactive while the user types and ending up clobbering something.

Could anyone reproducing this tell us which of these cases you are seing? I think Andrew is seeing 2, maybe also Chris?

Yes, I am seeing 2 with at least the 2020-07-29 build (have an update pending).

Comment 33

[Andrew Overholt [:overholt]]

Still seeing it ("about" -> "aout" is my most common reproducer) in the 2020-07-30 nightly.

Comment 34

[Marco Bonardo [:mak]]

It looks like UrlbarProviderTokenAliasEngines.jsm has a bug similar to the one fixed in UrlbarProviderAutofill.jsm, again I'm not sure it's "the" bug (probably not since nobody is typing @ here), but it should be fixed.

Comment 35

[Harry Twyford]

Marco found a STR in bug 1656152 comment 4 that I can reproduce: typing "wc"
very quickly. Here, Q1 is the query for "w" that has an autofill result and Q2
is the query for "wc" that does not.

The problem is produced when Q1 enters this._getAutofillResult but

1. Q1 is cancelled while that function is awaiting and
2. Q2 also enters this._getAutofillResult while Q1 is still inside it.

Q1 sets this._autofillResult inside this._getAutofillResult. The
instance != this.queryInstance check after Q1 returns from
this._getAutofillResult is true since Q1 is cancelled.
UrlbarProviderAutofill.isActive returns false for Q1, but this._autofillResult
is never cleared.

Q2 then finishes this._getAutofillResult without ever finding an autofill
result. However, this._autofillResult still exists when Q2 gets to this point,
since it wasn't cleared after Q1 was cancelled. The autofill result partially
overwrites the typed string.

Clearing this._autofillResult when instance != this.queryInstance is enough
to fix this bug on my machine. UrlbarProviderTokenAliasEngines has this same
issue because it also fetches a result in isActive.

Comment 36

[Harry Twyford]

Attachment: Bug 1653436 - Store the queryInstance in autofillData and check it when startQuery is ready to display a result. r?mak

Comment 37

[Chris Peterson [:cpeterson]]

(In reply to Marco Bonardo [:mak] from comment #31)

Here the 2 problems I see reported are:

1. losing characters at the end of the string: this was effectively a regression caused by heuristic changes we made in Firefox 80, various fixes landed, I could reproduce the bug easily, but I can't on current Nightly
2. losing characters in the middle of the string: comment 23. My only suspects are bug 1655034, that is fixed in Nightly, and maybe autofill being more reactive while the user types and ending up clobbering something.

Could anyone reproducing this tell us which of these cases you are seing? I think Andrew is seeing 2, maybe also Chris?
Is anyone still seeing 1?

When I quickly type "about:config", the actual string in the address bar is "aout:config". I can still reproduce that problem in today's Nightly 80.0a1 build 2020-07-30 but not in Beta 80.0b1. I don't know if that problem counts as your #1 or #2 (as the missing "b" character is in the middle of "about" but it disappears when "b" is at the end of "ab" while I am typing "abo").

Comment 38

[Marco Bonardo [:mak]]

Attachment: Bug 1653436 - Some urlbar providers should check whether the query was canceled in isActive. r=adw

Comment 40

[Dustin L. Howett]

mozregression confirms it as a regression from 1648468 :)

app_name: firefox
build_date: 2020-07-13 01:02:06.113000
build_type: integration
changeset: 51a157c5fe34d465fcd08a8f4d5276ae13c06f1c
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=52aae6784eb346ee86af8d1f13cdb8965c3a47be&tochange=51a157c5fe34d465fcd08a8f4d5276ae13c06f1c
repo_name: autoland
repo_url: https://hg.mozilla.org/integration/autoland
task_id: JUJU1HxZQI2nsqkrNYnqFg

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70651770977f
Some urlbar providers should check whether the query was canceled in isActive. r=adw

Comment 43

[Dustin L. Howett]

Can no longer repro on autoland changeset 70651770977f. Nicely done!

Comment 44

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/70651770977f

Comment 45

[Mathew Hodson]

Was this actually fixed on Firefox 80? Or does the new patch need to be uplifted?

Comment 46

[Marco Bonardo [:mak]]

the last patch must be uplifted.

Comment 47

[Marco Bonardo [:mak]]

Comment on attachment 9167309 [details]
Bug 1653436 - Some urlbar providers should check whether the query was canceled in isActive. r=adw

Beta/Release Uplift Approval Request

• User impact if declined: Autofill may happen unexpectedly, clobbering some user typed chars.
  Additionally now part of this bug is in 80 and part is not, so it'd be better to unify the code and clarify that. We should not reopen this, even if we should discover new edge cases.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: I found that if "w" autofills to www.something.com, then typing "wc" really quickly will intermittently autofill to ww.something.com instead of searching for "wc". (see bug 1648468)
  A few people could reproduce easily on their profile and may help verifying.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I don't think it's particularly scary, we're checking if some cached data is valid before using it. We'll look into writing an automated test in bug 1655403.
• String changes made/needed:

Comment 48

[Marco Bonardo [:mak]]

If you find some cases where this still happens, please don't reopen this bug. Tracking its status is becoming problematic, so we'd prefer new bugs filed apart.
If you could reproduce the problem please test latest Nightly and let us know if it solves your problem.

Comment 49

[Julien Cristau [:jcristau]]

Comment on attachment 9167309 [details]
Bug 1653436 - Some urlbar providers should check whether the query was canceled in isActive. r=adw

urlbar fix, approved for 80.0b4

Comment 50

[Harry Twyford]

Chris, are you still seeing this bug?

Comment 51

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f450a3bda8d8

Comment 52

[Chris Peterson [:cpeterson]]

(In reply to Harry Twyford [:harry] from comment #50)

Chris, are you still seeing this bug?

Nope. I can no longer reproduce this bug in 81 Nightly. 👍🏻

Comment 53

[Markus Stange [:mstange]]

Same here. I was able to reproduce this with the letters "work" 4 days ago and it is working correctly now.

Comment 54

[Petruta Horea [:phorea]]

Reproduced the issue as described in comment 47 on Nightly 81.0a1 2020-08-01 with a dirty profile.
Verified as fixed using Firefox 80 beta 7 and latest Nightly 81.0a1 2020-08-12 under Win 10 64-bit and Ubuntu 18.04 64-bit.