Closed Bug 1186393 Opened 4 years ago Closed 4 years ago

Some search add-ons don't perform searches from awesome bar

Categories

(Firefox :: Search, defect, P1)

42 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox42 --- affected
firefox43 --- verified

People

(Reporter: petruta.rasa, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [unifiedautocomplete][suggestions][fxsearch])

Attachments

(1 file, 1 obsolete file)

Reproduced with Firefox 42.0a1 2015-07-21 under Win 7 x64 and Mac OSX 10.9.5.

Steps to reproduce:
1. Open Search Preferences and select the option to add more search engines
2. From https://addons.mozilla.org/en-US/firefox/search/?atype=4 page select the first engine (Google Default) and make it the default search engine
3. Go to url bar and start typing
4. Select any of the search suggestions with mouse click
5. Delete everything from location bar and start typing again
6. Select an entry with Down arrow and press Enter

Expected results:
After steps 4 and 6, search results of the entered terms should open

Actual results:
The following error is thrown in the browser's console and the search is not performed:
TypeError: engine is null urlbarBindings.xml:341:19

Notes: 1. Typing a search term and then pressing Enter doesn't reproduce the issue.

2. Not every AMO add-on reproduce this. Here are some others that can be used to reproduce:
 - https://addons.mozilla.org/en-US/firefox/addon/ixquick-https-privacy-search-e/?src=search
 - https://addons.mozilla.org/en-US/firefox/addon/internet-search-facebook/?src=search
 - Firefox Add-ons offered while on AMO page
 - YouTube Video Search that can be added using the "+" icon on youtube.com page

3. This seems to be regressed by bug 1168811 (cc'ing :mak). I didn't find any other possible culprits in the pushlog:

Last good revision: f986e55c4e0b (2015-05-29)
First bad revision: 45a4d6336c73 (2015-05-30)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f986e55c4e0b&tocha
nge=45a4d6336c73

Last good revision: bea6758824d9
First bad revision: 6b20dd2e20cc
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bea675
8824d9&tochange=6b20dd2e20cc
Blocks: 958204
Flags: firefox-backlog+
Priority: -- → P2
Whiteboard: [unifiedautocomplete][suggestions][fxsearch]
Intermittently, the same error is shown and the search is not made, when using search keywords and clicking / selecting the suggestions with arrow keys and pressing Enter.

Marco, should this case be covered with another bug?
if it's about the first pre-selected entry in the popup, it might be covered by bug 1172937. You might retest after that lands.
(In reply to Marco Bonardo [::mak] from comment #2)
> if it's about the first pre-selected entry in the popup, it might be covered
> by bug 1172937. You might retest after that lands.
I will, thanks!

This issue also reproduces when Wikipedia (en) is set as the default search engine.
P2 - we will check again after bug 1172937 lands.
Rank: 25
Depends on: 1172937
This issue still reproduces after bug 1172937 landed.

Simple STR:
1. Make Wikipedia (en) the default search engine (or https://addons.mozilla.org/en-US/firefox/addon/google-default/?src=search add-on)
2. Start typing and select the search suggestions results using either mouse click or keyboard arrows + Enter

AR: Search is not performed
Duplicate of this bug: 1192594
Duplicate of this bug: 1180447
This is pretty bad, deserves to be a P1.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Rank: 25
Priority: P2 → P1
Attached patch patch (obsolete) — Splinter Review
The problem happens for engines whose name encodings in moz-action URIs are not equal to the engines' actual names.  Wikipedia's name has a space in it, "Wikipedia (en)" for example, which becomes "Wikipedia%20(en)", which urlbarBindings passes directly to the search service without decoding it.

This adds a front-end test.  We could use one anyway.  The reason it tests this bug is because the searchSuggestionEngine.xml engine name happens to have a space in it.  The test times out without decoding engineName because the search results page never loads.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb808a045287
Attachment #8660951 - Flags: review?(mak77)
Comment on attachment 8660951 [details] [diff] [review]
patch

There were try failures, canceling review for now.
Attachment #8660951 - Flags: review?(mak77)
Attached patch patch v2Splinter Review
OK, the problem was that the new test clicks a suggestion to make sure it loads correctly, but that adds the search results page to history, and then when browser_urlbarSearchSuggestionsNotification.js ran after the new test, it assumed that all urlbar results would be search suggestions, but now there was this results page in history.

So this patch removes that assumption, which is fragile and bad and unnecessary.  Passes locally for me when running the new test before browser_urlbarSearchSuggestionsNotification.js (and fails without the browser_urlbarSearchSuggestionsNotification.js fix).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01739b5460e
Attachment #8660951 - Attachment is obsolete: true
Attachment #8661354 - Flags: review?(mak77)
Comment on attachment 8661354 [details] [diff] [review]
patch v2

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

::: browser/base/content/test/general/browser_urlbarSearchSuggestions.js
@@ +13,5 @@
> +
> +    // Make sure the popup is closed for the next test.
> +    gURLBar.blur();
> +    Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
> +  });

nit: Since the test ends up adding history, even if you fixed the following test doing wrong assumptions, maybe we should yield PlacesTestUtils.clearHistory();
it's not that uncommon for tests to make assumptions on a clean history status.

@@ +20,5 @@
> +add_task(function* clickSuggestion() {
> +  gURLBar.focus();
> +  yield promiseAutocompleteResultPopup("foo");
> +  let [idx, suggestion] = yield promiseFirstSuggestion();
> +  let item = gURLBar.popup.richlistbox.getItemAtIndex(idx);

nit: there's no great value in returning the index, you could make promiseFirstSuggestion return [item, suggestion]

@@ +25,5 @@
> +  let loadPromise = promiseTabLoaded(gBrowser.selectedTab);
> +  item.click();
> +  yield loadPromise;
> +  let uri = Services.search.currentEngine.getSubmission(suggestion).uri;
> +  Assert.ok(uri.equals(gBrowser.currentURI));

please add a message

@@ +40,5 @@
> +      let [, type, paramStr] = mozActionMatch;
> +      let params = {};
> +      try {
> +        params = JSON.parse(paramStr);
> +      } catch (err) {}

why do we swallow this error? sounds like it may cover a coding mistake the test harness should eventually report.

::: browser/base/content/test/general/browser_urlbarSearchSuggestionsNotification.js
@@ +219,5 @@
> +      let [, type, paramStr] = mozActionMatch;
> +      let params = {};
> +      try {
> +        params = JSON.parse(paramStr);
> +      } catch (err) {}

ditto
Attachment #8661354 - Flags: review?(mak77) → review+
Rank: 5
https://hg.mozilla.org/mozilla-central/rev/1ab7fb448f60
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Verified as fixed using Developer Edition 43.0a2 under Win 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.