Closed Bug 1562585 Opened 4 months ago Closed 3 months ago

Quantum Bar is storing full urls in inputhistory

Categories

(Firefox :: Address Bar, defect, P1)

68 Branch
Desktop
All
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 69
Iteration:
69.4 - Jun 24 - Jul 7
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 68+ verified
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 + verified
firefox69 + verified
firefox70 --- verified

People

(Reporter: mak, Assigned: mak)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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.

Keywords: regression
Regressed by: 1559686
Flags: qe-verify+

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

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

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).

Attachment #9075308 - Attachment description: Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw → Bug 1562585 - Don't store urls in input history for dropmarker empty queries. r=adw,dao
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/a2ed47577d89
Don't store urls in input history for dropmarker empty queries. r=adw,dao
Backout by shindli@mozilla.com:
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

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

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/a48314ea7f02
Don't store urls in input history for dropmarker empty queries. r=adw,dao

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

Flags: needinfo?(mak77)

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
Flags: needinfo?(mak77)
Attachment #9075308 - Flags: approval-mozilla-beta?

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 :)

Attachment #9075308 - Flags: approval-mozilla-release?
Attachment #9075308 - Flags: approval-mozilla-esr68?
Attachment #9075308 - Flags: approval-mozilla-beta?

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.

Attachment #9075308 - Flags: approval-mozilla-release?
Attachment #9075308 - Flags: approval-mozilla-release+
Attachment #9075308 - Flags: approval-mozilla-esr68?
Attachment #9075308 - Flags: approval-mozilla-esr68+
Attachment #9075995 - Flags: checkin?(jcristau)
Attachment #9075995 - Flags: checkin?(jcristau) → checkin+
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
QA Whiteboard: [qa-triaged]

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.