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

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Petruta Horea [:phorea], Desktop Test Engineering]

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

Comment 1

[Petruta Horea [:phorea], Desktop Test Engineering]

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?

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Petruta Horea [:phorea], Desktop Test Engineering]

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

Comment 4

[:shell escalante]

P2 - we will check again after bug 1172937 lands.

Comment 5

[Petruta Horea [:phorea], Desktop Test Engineering]

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

Comment 8

[Drew Willcoxon :adw]

This is pretty bad, deserves to be a P1.

Comment 9

[Drew Willcoxon :adw]

Attachment: patch

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

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8660951 [details] [diff] [review]
patch

There were try failures, canceling review for now.

Comment 11

[Drew Willcoxon :adw]

Attachment: patch v2

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

Comment 12

[Marco Bonardo [:mak]]

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

Comment 13

[Drew Willcoxon :adw]

Landed with comments addressed:

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

Comment 14

[Drew Willcoxon :adw]

Oh and here's a newer try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a7e49b7b26f

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ab7fb448f60

Comment 16

[Petruta Horea [:phorea], Desktop Test Engineering]

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.