"http://x" incorrectly loads search bookmark with keyword "x"

RESOLVED FIXED in Firefox 63

Status

()

defect
P2
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

({regression})

Trunk
Firefox 63
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

Assignee

Description

10 months ago
Steps to reproduce:

1. Create a bookmark with URL "http://google.com/?q=%s" and keyword "foobarbaz".
2. Type "http://foobarbaz" into the location bar and press Enter.

Expected result:

* Firefox navigates to the URL "http://foobarbaz".

Actual result:

* Firefox navigates to the URL "http://google.com/?q=barbaz" (note that "foo" is cut off).
Assignee

Comment 1

10 months ago
This is happening at this line:

https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/toolkit/components/places/UnifiedComplete.js#1539

`this._searchTokens[0]` contains the URL after its prefix was stripped ("foobarbaz"), while `this._trimmedOriginalSearchString` contains the whole URL including the prefix ("http://foobarbaz").  The line above attempts to strip off the keyword, but doesn't account for the length of the prefix, so it strips the first 10 characters ("http://foo") leaving "barbaz".

Updated

10 months ago
Component: Places → Address Bar
Priority: -- → P2
Product: Toolkit → Firefox
Whiteboard: [fxsearch]
Assignee

Comment 3

10 months ago
This patch restores the stripped prefix before matching against bookmark keywords.  This means that entering "foo:bar" will no longer activate a bookmark with keyword "bar", but it will now match a bookmark with keyword "foo:bar".

Alternately, this function could simply return false if the original search string contains any keyword.  This would make it impossible to use any keyword that contains ":".
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED

Comment 4

10 months ago
It was gradually broken.
Before landing of  Bug 1306639, Firefox navigates to "http://foobarbaz" as expected.
Between landing of Bug 1306639 and Bug 1310737, Firefox navigates to "https://www.google.com/?q=".
After landing of  Bug 1310737, Firefox navigates to "http://google.com/?q=barbaz"


#1 regression window(Firefox navigates to "https://www.google.com/?q="):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=64cd7d87e78cf2e4b919aad10fb9f4962834b004&tochange=70d19ed29c980ba3d6825e5286bda08fa7dc676f

Regressed by: 70d19ed29c98	Marco Bonardo — Bug 1306639 - Searching in locationbar by typing something and pressing enter is not accounted in telemetry. r=adw

#2 regression window(Firefox navigates to "http://google.com/?q=barbaz"):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fc6a63bedaec6e34bb3b2491b497b4f131387dab&tochange=74b268465bc724c303f44de6fd3bb939c56f52de

Regressed by: 74b268465bc7	Marco Bonardo — Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords. r=adw
Thank you for checking the range.
Comment on attachment 8998037 [details]
Bug 1481319 - Include prefix when matching bookmark keywords. r?mak

Marco Bonardo [::mak] (Away 9-26 Aug) has approved the revision.

https://phabricator.services.mozilla.com/D2804
Attachment #8998037 - Flags: review+

Comment 8

10 months ago
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7645fd7dc1a4
Include prefix when matching bookmark keywords. r=mak

Comment 9

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7645fd7dc1a4
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Given how old the regression is, doesn't seem worth backporting to ESR60. Did you want to nominate for Beta approval?
Flags: needinfo?(mbrubeck)
Flags: in-testsuite+
Assignee

Comment 11

10 months ago
I don't think this is critical to backport to beta, but I'll defer to mak if thinks we should.
Flags: needinfo?(mbrubeck)
yep, not critical, it's an edge case.

Updated

9 months ago
Duplicate of this bug: 1490651
You need to log in before you can comment on or make changes to this bug.