Closed Bug 1167915 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Places, defect)

40 Branch
defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- unaffected
firefox39 + verified
firefox40 + fixed
firefox41 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: alice0775, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: regression, ux-trust)

Attachments

(4 files, 3 obsolete files)

[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)
Component: General → Places
Product: Firefox → Toolkit
I disappointed there are no automate test since Bug 260006 :/
Firefox's credibility has hit rock bottom...
Keywords: ux-trust
[Tracking Requested - why for this release]:
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.
Florian, as Alice said in comment #1, we should have test for this. Is it hard to write?
Flags: needinfo?(florian)
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)
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)
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.
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.
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.
Depends on: 1129957
So, I have sort-of a working patch, modulo bug 1129957.
I'll now try to write a test.
Attached patch patch v1.2Splinter Review
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)
The utils I added should simplify bug 1160326.
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+
Because this appears to be a regression of 39, we are tracking it.
(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.
Points: --- → 3
Flags: qe-verify+
Flags: in-testsuite+
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/b8ae983d4937
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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?
Attached patch beta patch (obsolete) — Splinter Review
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
note: while the patch applies to Aurora, the test times out due to the same weird yield issue I hit in Beta.
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.
Attached patch nightly followupSplinter Review
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)
Attached patch Aurora patchSplinter Review
the 2 merged Nightly patches, plus the typo fix in browser.js that was already fixed in Nightly.
Attached patch beta patchSplinter Review
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)
(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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.