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

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Matt Brubeck (:mbrubeck)]

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

Comment 1

[Matt Brubeck (:mbrubeck)]

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

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1481319 - Include prefix when matching bookmark keywords. r?mak

Comment 3

[Matt Brubeck (:mbrubeck)]

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 ":".

Comment 4

[Alice0775 White]

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

Comment 5

[Marco Bonardo [:mak]]

Thank you for checking the range.

Comment 6

[Phabricator Automation]

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

Comment 7

[Matt Brubeck (:mbrubeck)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3176857bbc04a937cebc63c2b9ec0d5514113d3

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7645fd7dc1a4
Include prefix when matching bookmark keywords. r=mak

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7645fd7dc1a4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Given how old the regression is, doesn't seem worth backporting to ESR60. Did you want to nominate for Beta approval?

Comment 11

[Matt Brubeck (:mbrubeck)]

I don't think this is critical to backport to beta, but I'll defer to mak if thinks we should.

Comment 12

[Marco Bonardo [:mak]]

yep, not critical, it's an edge case.