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

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Alice0775 White]

[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

Comment 1

[Alice0775 White]

I disappointed there are no automate test since Bug 260006 :/
Firefox's credibility has hit rock bottom...

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Florian, as Alice said in comment #1, we should have test for this. Is it hard to write?

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Marco Bonardo [:mak]]

So, I have sort-of a working patch, modulo bug 1129957.
I'll now try to write a test.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1

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

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.2

This one should do (waitForFocus voodoo):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5be7a56b08d5

Comment 14

[Marco Bonardo [:mak]]

The utils I added should simplify bug 1160326.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 16

[Kate Glazko]

Because this appears to be a regression of 39, we are tracking it.

Comment 17

[Marco Bonardo [:mak]]

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

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b8ae983d4937

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8ae983d4937

Comment 20

[Marco Bonardo [:mak]]

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

Comment 21

[Marco Bonardo [:mak]]

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

Comment 22

[Marco Bonardo [:mak]]

note: while the patch applies to Aurora, the test times out due to the same weird yield issue I hit in Beta.

Comment 23

[Marco Bonardo [:mak]]

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.

Comment 24

[Marco Bonardo [:mak]]

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

Comment 25

[Marco Bonardo [:mak]]

Attachment: Aurora patch

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

Comment 26

[Marco Bonardo [:mak]]

Attachment: beta patch

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Approved for uplift to aurora. We should ask for some manual testing from QE.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e5be8fc20660

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/eb586f84c8a3

Comment 31

[Marco Bonardo [:mak]]

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

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d810a18a0e0f

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb586f84c8a3

Comment 35

[Andrei Vaida [:avaida]]

Verified fixed on Beta 39.0b6 (20150615125213), using Windows 8.1 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.