Keyword bookmarks don't expand keyword query "%S" or POST data

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Bookmarks & History
P1
major
VERIFIED FIXED
7 months ago
5 months ago

People

(Reporter: flod, Assigned: mak)

Tracking

({regression})

Trunk
Firefox 52
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox50 unaffected, firefox51 verified, firefox52 verified)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
I have several keyword bookmarks using %S, and they stopped working in Nightly. I can reproduce in at least 52.0a1 (2016-10-16), on two different machines.

For example I have a bookmark https://bugzilla.mozilla.org/show_bug.cgi?id=%S with keyword "b", "b 123456" takes me to https://bugzilla.mozilla.org/show_bug.cgi?id=%S instead of https://bugzilla.mozilla.org/show_bug.cgi?id=123456
(Assignee)

Comment 1

7 months ago
I suspect bug 1306639, now it's the backend generating the url, and we're doing
replace("%s", queryString); fixing that would be trivial but...

The browser code is far more complex http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/base/content/browser.js#2148

Indeed we likely also broke POST keywords.
It may be the right time to finally move the code to the backend and remove getShortcutOrURIAndPostData from urlbarbindings (and maybe even simplify it for other consumers).
Blocks: 1306639
Keywords: regression
(Assignee)

Updated

7 months ago
Priority: -- → P1
Whiteboard: [fxsearch]
(Assignee)

Updated

7 months ago
Summary: Keyword bookmarks don't expand keyword query "%S" → Keyword bookmarks don't expand keyword query "%S" or POST data
(Assignee)

Updated

7 months ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Updated

7 months ago
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox51: --- → affected
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1310953
(Assignee)

Comment 3

7 months ago
we also need to fix the encoding (see duped bug 1310953) but it should come for free once we port getShortcutOrURI
(Assignee)

Updated

7 months ago
Blocks: 1126143
(Assignee)

Updated

7 months ago
Blocks: 1312018
(Assignee)

Updated

7 months ago
Blocks: 1313620
Comment hidden (mozreview-request)
(Assignee)

Comment 5

7 months ago
Ugh, I forgot bug 1312018. will update the patch and tests for it.
Comment hidden (mozreview-request)
Looks like there were failures on try.  Does that try push on reviewboard correspond to the latest revision?
(Assignee)

Comment 8

7 months ago
the X is fixed, the b-c are not.
The problem is simple though, by removing getShortcutOrURI from handleCommand, it doesn't work anymore to just set .value and press Enter.
There are 2 ways:
1. update these tests to do the "right" thing (simulate input like a normal user)
2. reintroduce getShortcutOrURI in the handleCommand else branch

While I was initially tempted to go path 1, I then thought the most add-ons compatible choice is path 2. Since most of the encoding/decoding code is now shared between GSOU and UC, it should still be a good compromise.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

7 months ago
there are still a couple things I'm thinking about.
1. I'm not a big fan of the fallback I introduced because creating a URL is more expensive than just checking for some boolean... but it should be acceptable. So far I can't think of a better way to understand if the result came from the search component or not.
2. It would be great to provide a way for add-ons to intercept Enter and replace the result, for bug 1313620. I'm wondering something about overrideValue. I'll think about that.

Comment 12

7 months ago
mozreview-review
Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

https://reviewboard.mozilla.org/r/89342/#review88668

This is a pretty big patch but I've looked it over a couple of times now and don't see anything clearly wrong (naturally!)...

::: browser/base/content/browser.js:2074
(Diff revision 2)
> - * @param callback (optional, deprecated)
> - *        The callback function invoked when done. This parameter is
> - *        deprecated, please use the Promise that is returned.
>   *
> - * @return Promise<{ postData, url, mayInheritPrincipal }>
> + * @return {Promise}
> + * @resolves <{ url, postData, mayInheritPrincipal }>. If it's not possible

Nit: I think the < and > here are unnecessary and a little confusing.
Attachment #8805585 - Flags: review?(adw) → review+
(Assignee)

Updated

7 months ago
Blocks: 1232132
Comment hidden (mozreview-request)
(Assignee)

Comment 14

7 months ago
I actually found a bug in browser_urlbarEnter.js test and thus this is likely to fix an intermittent.
Comment hidden (mozreview-request)

Comment 16

7 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/74b268465bc7
Urlbar doesn't properly handle %S and POST keywords. r=adw
(Assignee)

Updated

7 months ago
Blocks: 1314013

Comment 17

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74b268465bc7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hi ::mak,
Since this bug is a regression and also affects 51, do you think it's worth uplifting to 51 if this patch is not too risky??
Flags: needinfo?(mak77)

Updated

7 months ago
No longer blocks: 1312018

Updated

7 months ago
Duplicate of this bug: 1312018

Comment 20

7 months ago
spum
Yep, the bug 1312018 fixed this problem.
(Assignee)

Comment 21

7 months ago
(In reply to Gerry Chang [:gchang] from comment #18)
> Hi ::mak,
> Since this bug is a regression and also affects 51, do you think it's worth
> uplifting to 51 if this patch is not too risky??

Yes, I plan to request approval for an uplift, I was actually trying to understand if bug 1303333 will be uplifted cause it bitrots this patch. I will ask Alessio today.
Flags: needinfo?(mak77)
(Assignee)

Comment 22

7 months ago
OK, we will wait for bug 1303333 data validation and uplift approval, that should happen on Monday, otherwise we should heavily unbitrot both that patch and this one, since they partly touch the same code. We'll queue up after it.
Flags: needinfo?(mak77)
(Assignee)

Updated

7 months ago
Flags: needinfo?(mak77)
See Also: → bug 1315514
(Assignee)

Updated

7 months ago
Depends on: 1315514
(Assignee)

Comment 23

7 months ago
Created attachment 8809058 [details] [diff] [review]
Aurora patch

NOTE: this also includes the typo fix in bug 1315514 (along with 2 additional tests from there)

Approval Request Comment
[Feature/regressing bug #]: Bug 1306639
[User impact if declined]: A large number of bookmark keywords don't work in the location bar
[Describe test coverage new/current, TreeHerder]: automated tests in Nightly
[Risks and why]: The patch is mostly moving existing code to share it across components and we have a ton of tests on autocomplete. The risk looks acceptable.
[String/UUID change made/needed]: none

This is the unbitrotted patch for Aurora, I'm currently running it through Try, I'll take care of the push by myself.
Attachment #8809058 - Flags: approval-mozilla-aurora?
Comment on attachment 8809058 [details] [diff] [review]
Aurora patch

This patch fixes a regression related to keyword bookmarks and also includes tests. Take it in 51 aurora.
Attachment #8809058 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 25

7 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/476ff24916373b77e5cc0b1f2658188e3939406d
status-firefox51: affected → fixed
I have reproduced this bug with Nightly 52.0a1 (2016-10-17) (32-bit)! 

This bug's fix is verified with latest Developer Edition (Aurora) and Beta.

Build ID    :	20170105004018
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0


Build ID    :  	20170105155013
User Agent  :	Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0

 [testday-20170106]
Thanks Almas!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.