"Add a Keyword for this Search…" does not work where using POST method

VERIFIED FIXED in Firefox 39

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: alice0775, Assigned: mak)

Tracking

(Blocks: 1 bug, {regression, ux-trust})

40 Branch
mozilla41
regression, ux-trust
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox38 unaffected, firefox39+ verified, firefox38.0.5 unaffected, firefox40+ fixed, firefox41+ fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
[Tracking Requested - why for this release]: Regression due to Bug 1125115

"Add a Keyword for this Search…" POST method is broken.

Steps to reproduce
1. Open http://forum.mozilla.gr.jp/srch.cgi
2. Right click on input field
3. Choose "Add a Keyword for this Search…"
4. Input keyword (ex. mmm ) and save
5. Input "mmm firefox" and Enter (without quotation marks)

Actual Results:
Search with default seacrh engine

Expected Results:
Should search in http://forum.mozilla.gr.jp/srch.cgi

Regressed by: Bug 1125115
Flags: needinfo?(mak77)
(Reporter)

Updated

4 years ago
Component: General → Places
Product: Firefox → Toolkit
(Reporter)

Comment 1

4 years ago
I disappointed there are no automate test since Bug 260006 :/
Firefox's credibility has hit rock bottom...
Keywords: ux-trust
(Reporter)

Comment 2

4 years ago
[Tracking Requested - why for this release]:
Blocks: 1125113
status-firefox39: unaffected → affected
tracking-firefox39: --- → ?
I tried to repro this on Firefox Dev edition 40.0a2 and I see it working fine in some cases and not in others. If I go to www.yahoo.com and try "add a keyword for this search" and then play with it, it works fine. 

But when I try STR from the description it doesn't work. If I modify step 5 and only enter keyword "mmm" in browser address bar (without any arguments) it works fine.

Anyways marking as tracking so a dev can further investigate what aspects of this are broken. Tracking for firefox40 and firefox41.
tracking-firefox40: ? → +
tracking-firefox41: ? → +
Florian, as Alice said in comment #1, we should have test for this. Is it hard to write?
Flags: needinfo?(florian)
(Assignee)

Comment 5

4 years ago
we do have a test http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_addKeywordSearch.js but doesn't look like it's checking POST forms. Let alone POST forms in foreign languages...
fwiw, we first need to figure out what makes the case in comment 0 special, the problem might be at the API level and then it might require a different kind of test.
Flags: needinfo?(mak77)
(Assignee)

Comment 6

4 years ago
Ah it's related to the charset of the page, strange though, I didnd't touch browser.js at all.
I'll have to investigate this.

Writing a test might be trivial or not, it depends on the bug.
Assignee: nobody → mak77
Flags: needinfo?(florian)
(Assignee)

Comment 7

4 years ago
So far I found a bug on charset that exists from Firefox 25 (bug 570266) and it has just been fixed by Bug 1141350 (I wonder if that happened on purpose or by luck), there is a typo where charSet became charset.
But there's likely more that I'm still investigating.
(Assignee)

Comment 8

4 years ago
I think I found the problem, when creating a keyword we first try to set postData on a bookmark without a keyword, then add the keyword later... bug 1095425 will likely fix this, but we need a solution in the meanwhile.

I must kill PlacesEditBookmarkPostDataTransaction and merge it into the editKeyword transaction.
(Assignee)

Comment 9

4 years ago
Plus I just found loading POST keywords is completely broken in e10s (works in a non-e10s window), I see
"RemoteWebNavigation doesn't accept postdata or headers"

Looks like it's bug 1129957.
(Assignee)

Updated

4 years ago
Depends on: 1129957
(Assignee)

Comment 10

4 years ago
So, I have sort-of a working patch, modulo bug 1129957.
I'll now try to write a test.
(Assignee)

Comment 13

4 years ago
Created attachment 8612872 [details] [diff] [review]
patch v1.2

This one should do (waitForFocus voodoo):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5be7a56b08d5
Attachment #8612765 - Attachment is obsolete: true
Attachment #8612872 - Flags: review?(ttaubert)
(Assignee)

Comment 14

4 years ago
The utils I added should simplify bug 1160326.
(Assignee)

Updated

4 years ago
Blocks: 1160326
Status: NEW → ASSIGNED
Comment on attachment 8612872 [details] [diff] [review]
patch v1.2

Review of attachment 8612872 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/tests/browser/browser_bookmarkProperties_addKeywordForThisSearch.js
@@ +1,1 @@
> +const TEST_URL = "http://mochi.test:8888/browser/browser/components/places/tests/browser/keyword_form.html";

Nit: "use strict";

@@ +29,5 @@
> +      let entry;
> +      yield waitCondition(function* () {
> +          entry = yield PlacesUtils.keywords.fetch("kw");
> +          return !!entry;
> +        },"Unable to find the expected keyword");

Nit: indentation is a little off here.

::: browser/components/places/tests/browser/head.js
@@ +356,5 @@
> +
> +  return gContextMenuContentData.popupNode;
> +});
> +
> +let waitCondition = Task.async(function* (conditionFn, errorMsg) {

Nit: maybe waitForCondition()?

@@ +362,5 @@
> +    if ((yield conditionFn()))
> +      return;
> +    yield new Promise(resolve => {
> +      this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +      this.timer.init(resolve, 100, Ci.nsITimer.TYPE_ONE_SHOT);

What's |this| in this context? Could we end up overwriting |this.timer| with multiple waitForCondition() calls? In m-bc tests we could just use setTimeout(), no?
Attachment #8612872 - Flags: review?(ttaubert) → review+

Comment 16

4 years ago
Because this appears to be a regression of 39, we are tracking it.
tracking-firefox39: ? → +
(Assignee)

Comment 17

4 years ago
(In reply to Tim Taubert [:ttaubert] from comment #15)
> @@ +362,5 @@
> > +    if ((yield conditionFn()))
> > +      return;
> > +    yield new Promise(resolve => {
> > +      this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +      this.timer.init(resolve, 100, Ci.nsITimer.TYPE_ONE_SHOT);
> 
> What's |this| in this context? Could we end up overwriting |this.timer| with
> multiple waitForCondition() calls? In m-bc tests we could just use
> setTimeout(), no?

We can't use setTimeout cause we have the flaky timeout detector (any timeout > 0 is considered flaky and disallowed), and I don't want to opt-in any test using waitForCondition to flaky timeouts.
(Assignee)

Updated

4 years ago
Points: --- → 3
Flags: qe-verify+
Flags: in-testsuite+
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/b8ae983d4937
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 20

4 years ago
Comment on attachment 8612872 [details] [diff] [review]
patch v1.2

NOTE: the patch doesn't apply to Beta, I'm unbitrotting it and will attach a beta patch soon. The current m-c changeset applies cleanly to Aurora instead.

Approval Request Comment
[Feature/regressing bug #]: bug 1125113
[User impact if declined]: keywords with POST data don't work
[Describe test coverage new/current, TreeHerder]: automated test
[Risks and why]: The change is limited to keywords, other functionality should be untouched, the test covers most of the changes
[String/UUID change made/needed]: none
Attachment #8612872 - Flags: approval-mozilla-beta?
Attachment #8612872 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

4 years ago
Created attachment 8615981 [details] [diff] [review]
beta patch

This required some changes cause looks like beta has some bug in the promises walker loop, plus other bugs (charser) were unfixed and the bookmarks UI code differs.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b7cf6fccb2
(Assignee)

Comment 22

4 years ago
note: while the patch applies to Aurora, the test times out due to the same weird yield issue I hit in Beta.
(Assignee)

Comment 23

4 years ago
I figured the thing, the dialog is modal and blocks the events loop. Since I directly invoke AddKeywordForSearchField() it's blocking the test events loop. In Nightly the function is using message manager and it's the onmessage handler that opens the modal dialog.
(Assignee)

Comment 24

4 years ago
Created attachment 8616201 [details] [diff] [review]
nightly followup

So, I prefer to keep the trees in-sync, let's land this follow-up to nightly and it should also solve the problem in Aurora and Beta, thus making the patches closer.
Attachment #8615981 - Attachment is obsolete: true
Attachment #8616201 - Flags: review?(ttaubert)
(Assignee)

Comment 25

4 years ago
Created attachment 8616245 [details] [diff] [review]
Aurora patch

the 2 merged Nightly patches, plus the typo fix in browser.js that was already fixed in Nightly.
(Assignee)

Comment 26

4 years ago
Created attachment 8616276 [details] [diff] [review]
beta patch
Attachment #8616201 - Flags: review?(ttaubert) → review+
Comment on attachment 8612872 [details] [diff] [review]
patch v1.2

Approved for uplift to aurora. We should ask for some manual testing from QE.
Attachment #8612872 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Looks like I just approved the wrong patch. Please uplift the one marked "Aurora" to aurora.
 
I would love to have this in 39, but we are about to build beta 4 (of 7).  This seems complicated enough that I'm reluctant to uplift it to beta without more testing time. mak, what do you think about that?
Flags: needinfo?(mak77)
(Assignee)

Comment 31

4 years ago
(In reply to Liz Henry (:lizzard) from comment #28)
> I would love to have this in 39, but we are about to build beta 4 (of 7). 
> This seems complicated enough that I'm reluctant to uplift it to beta
> without more testing time. mak, what do you think about that?

It sounds like the uplift itself will give us more testing. I know this is not a great answer, but I likely don't have a better one. Keywords (and even more keywords with POST data) are a feature used by a 5% of our users, the patch only touches that feature, and the automated test covers the most important operation (adding a keyword). So I think if we are evaluating fixing this bug, it would likely be better sooner than later.
Flags: needinfo?(mak77)
Comment on attachment 8612872 [details] [diff] [review]
patch v1.2

OK, thanks Marco, let's try this on beta 4 and give it some extra manual testing.
Attachment #8612872 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified fixed on Beta 39.0b6 (20150615125213), using Windows 8.1 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.