1562585 - Quantum Bar is storing full urls in inputhistory

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Marco Bonardo [:mak]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1559686#c13 and below, in some cases we are not storing the user typed string, but a full url.
We should not store urls as inputhistory input strings. That means we may also want to skip pasted strings, if they look like urls.

Additionally, ensure we don't store empty strings, and don't execute the adaptive query for empty search strings.

Comment 1

[Marco Bonardo [:mak]]

To keep this focused, I filed bug 1562823 about a series of remaining defects and improvements we could apply to input history.

Comment 2

[Marco Bonardo [:mak]]

This bug is about what happens when opening the address bar popup on an empty tab, or opening it from the dropmarker on any tab, then picking an entry that is not the first one, either with the mouse or the keyboard.
The currently expected behavior is that we store an input history row with empty "input" field. In the future this may change, we may not want to store a row at all then.

STR a:

1. open a new tab
2. click on the dropmarker to open the panel
3. pick something with mouse / pick something with keyboard

STR b:

1. open a new tab
2. press down to open the panel
3. pick something with mouse / pick something with keyboard

STR c:

1. load a page
2. click on the dropmarker to open the panel
3. pick something with mouse / pick something with keyboard

EXPECTED RESULT:
an empty string is stored in moz_inputhistory input column
ACTUAL RESULT:
a url is stored in moz_inputhistory input column

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao

When the search string is supposed to be "empty", we should not store a url in
inputhistory's input column, instead we should store an empty input. This happens
for example when clicking the dropmarker and picking something, or when pressing
DOWN on a valid pageproxystate (no user input).
In the future we may evaluate not storing an empty string at all (Bug 1562823).

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2ed47577d89
Don't store urls in input history for dropmarker empty queries. r=adw,dao

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76d6cbd3793b
Backed out changeset a2ed47577d89 for causing perma bc failures in browser/components/urlbar/tests/browser/browser_autoFill_preserve.js CLOSED TREE

Comment 6

[Marco Bonardo [:mak]]

oops, it's just a typo: this._textValueOnLastSearche :(

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a48314ea7f02
Don't store urls in input history for dropmarker empty queries. r=adw,dao

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

Please request uplift to release/esr68 today if this ought to be in 68.0.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 9075308 [details]
Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao

Beta/Release Uplift Approval Request

• User impact if declined: We may be storing urls instead of empty strings for urlbar input history, that means some searches may return unexpected/surprising top results.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 2 has steps to reproduce, we have an automated test covering those anyway.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The patch is simple and has a test, but didn't get nightly testing. We are confident enough, because there are so many automated tests for the urlbar, but clearly we can't exclude edge cases from not being tested. Our additional manual testing didn't show problems anyway.
• String changes made/needed: none

Comment 10

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9075308 [details]
Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao

I did say release/esr68, not beta :)

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9075308 [details]
Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao

fix a recent regression in quantumbar, approved for 68 rc2.

Comment 12

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-release/rev/6a1981c7a257

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/6a1981c7a257

Comment 14

[Marco Bonardo [:mak]]

Attachment: test_release_fix.diff

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-release/rev/aed34f2ed95d

Comment 16

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/aed34f2ed95d

Comment 17

[Razvan Maries]

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

Comment 18

[Adrian Florinescu [:aflorinescu]]

I've verified the fix for this issue on Ubuntu 16.04, Windows 10 and a very quick look on Mac 10.13.6, since I don't feel like the fix is OS dependent. Mainly I've followed the scenarios described in comment 2, but also tried to make heads and tails on how the data looks or should look like in places.sqlite in relationship to input history.

Given the fact that I don't have extensive knowledge in this area, the most I could do was to look around to see if I spot anything obviously wrong, which I didn't.

Taking into account the above and adding to it that this fix is also covered by automated tests and have been previously checked manually @ implementation time, I'll move to mark this issue as verified on:

Release 68.0 2019-07-05
ESR 68.0esr 2019-07-05
Beta 69.0b3 2019-07-08
Nightly 70.0a1 2019-07-08