1222107 - Yahoo.com offering a Yahoo Search engine that doesn't install

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Mike Kaply [:mkaply]]

If you visit yahoo.com, you see the + sign on the search box.

Clicking on it shows an offer to install "Yahoo Search"

If you click that, nothing happens and you get this error on the console:

Error adding search engine: 2 urlbarBindings.xml:1481:0

1. Our search engine should match Yahoo's so we don't get this offer.

2. Error shouldn't be happening.

Comment 1

[Alice0775 White]

The error is caused by Bug 937870.

Before landing Bug 937870, the following alert box pops up.

"Nightly could not install the search plugin from "https://search.yahoo.com/opensearch.xml" because an engine with the same name already exists."

But now, It silently fails and an error is shown on the browser console.

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5d40a8f1b00b&tochange=992194d6631f

Regressed by:bd3fb665acfc	Chenxia Liu — Bug 937870 - nsSearchService displays duplicate engine prompt regardless of confirmation set by caller. r=gavin

Comment 2

[Mike Kaply [:mkaply]]

There's a second problem at play here, though.

If the engine already exists, the offer shouldn't even be made to install the engine, should it?

I believe we're smart enough to do that (we do that on other pages).

Comment 3

[Mike Kaply [:mkaply]]

And thank you for tracking that down.

Comment 4

[Alice0775 White]

(In reply to Mike Kaply [:mkaply] from comment #2)
> There's a second problem at play here, though.
> 
> If the engine already exists, the offer shouldn't even be made to install
> the engine, should it?
> 
> I believe we're smart enough to do that (we do that on other pages).

maybe dup of Bug 351441

Comment 5

[Florian Quèze [:florian]]

The problem here is that the name of the engine offered in the HTML page (<link rel="search" type="application/opensearchdescription+xml" href="https://fr.search.yahoo.com/opensearch.xml" title="Yahoo Search" />) doesn't match the actual name of the engine once we download it: 	<ShortName>Yahoo</ShortName>

So yes, it's a duplicate of bug 351441 for the issue within Firefox. But given Yahoo is a partner, maybe we can convince them to fix it on their website?

Comment 6

[Kev Needham [:kev]]

Ask sent to Yahoo product and marketing teams. They usually correct this kind of thing pretty quickly.

Comment 7

[Andrei Vaida [:avaida]]

Any update on this? It's still reproducible across versions and platforms, although the error is thrown now from a different location:
> Error adding search engine: 2 search.xml:1439:0

The Desktop Release QA stumbled over this again, while testing 45.0b10 -- perhaps our results might be useful:

[Affected versions]:
- 47.0a1 (2016-02-25)
- 46.0a2 (2016-02-25)
- 45.0b10-build1 (20160225145837)
- 44.0.2-build3 (20160210153822)
- 39.0.3-build2 (20150806001005)

[Affected platforms]:
- Windows 7 x86
- Ubuntu 12.04 x64
- Mac OS X 10.11
- Windows 8.1 x64

[Steps to reproduce]:
1. Launch Firefox using a clean profile.
2. Open this page: https://ro.yahoo.com/?p=us
3. Click the magnifying glass button from the Search Bar.
4. Click the [Add "Yahoo Cautare"] button.

[Screenshot]:
- http://i.imgur.com/8DdDb1g.png

Comment 8

[Mike Kaply [:mkaply]]

Kev:

Can we reping the Yahoo teams?

In the meantime, I'm going to fix this on the Firefox side. We shouldn't be swallowing the error.

Comment 9

[Mike Kaply [:mkaply]]

Attachment: Add error message in caller

This brings back the error message and puts it in the caller which is where it should be now since we added the new onError code for addEngine.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8724113 [details] [diff] [review]
Add error message in caller

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

::: browser/components/search/content/search.xml
@@ +1432,5 @@
>                event.target.hidePopup();
>                BrowserSearch.searchBar.openSuggestionsPanel();
>              },
>              onError: function(errorCode) {
> +              if (errorCode != 2) {

r- for hardcoding some random number without any details where it came from. This should use whatever constant this really is, or at least have a comment about what it is.

@@ +1438,5 @@
> +                return;
> +              }
> +              const kBrandBundleURI = "chrome://branding/locale/brand.properties";
> +              const kSearchBundleURI = "chrome://global/locale/search/search.properties";
> +              let brandBundle = Services.strings.createBundle(kBrandBundleURI);

This binding already depends on being loaded in a browser window scope (it relies on gBrowser being defined), so just steal the brand bundle from there instead of creating a new instance.

@@ +1439,5 @@
> +              }
> +              const kBrandBundleURI = "chrome://branding/locale/brand.properties";
> +              const kSearchBundleURI = "chrome://global/locale/search/search.properties";
> +              let brandBundle = Services.strings.createBundle(kBrandBundleURI);
> +              let searchBundle = Services.strings.createBundle(kSearchBundleURI);

There's a string bundle property on the binding, please use that instead.

@@ +1444,5 @@
> +              let brandName = brandBundle.GetStringFromName("brandShortName");
> +              let title = searchBundle.GetStringFromName("error_invalid_engine_title");
> +              let text = searchBundle.formatStringFromName("error_duplicate_engine_msg",
> +                                                           [brandName, target.getAttribute("uri")],
> +                                                           2);

Nit: Please make this not end up with the 2 on its own on a line.

@@ +1445,5 @@
> +              let title = searchBundle.GetStringFromName("error_invalid_engine_title");
> +              let text = searchBundle.formatStringFromName("error_duplicate_engine_msg",
> +                                                           [brandName, target.getAttribute("uri")],
> +                                                           2);
> +              Services.prompt.alert(window, title, text);

r- again for using an app-modal prompt.


Why aren't we fixing this properly so we don't offer to install the engine in the first place? If you don't know how to do that, ask Florian for help.

Comment 11

[Mike Kaply [:mkaply]]

> There's a string bundle property on the binding, please use that instead.

That's actually a different string.properties:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/search.properties

Unfortunately they are named the same.

> r- again for using an app-modal prompt.

Would tab modal make sense here? We're going to get an app modal prompt in the other failure case (download error). What are the rules for app modal versus tab modal, especially since this error is coming from chrome?

> Why aren't we fixing this properly so we don't offer to install the engine in the first place? If you don't know how to do that, ask Florian for help.

There are two issues here.

1. Yahoo has named it improperly (we're trying to fix that).

2. We would have to pre download the engine and parse it which no other browser does (see bug 351441).

We can at least get back to the previous behavior instead of just failing silently.

Comment 12

[:Gijs (he/him)]

So I just tried adding a non-busted thing, and nothing really happens in that case, except we re-focus the search bar and reopen the popup.

Can't we do that here? Seems to me that if the engine is already installed, that is nicer than either a tabmodal/appmodal dialog.

(In reply to Mike Kaply [:mkaply] from comment #11)
> > r- again for using an app-modal prompt.
> 
> Would tab modal make sense here?

It would make more sense than app-modal, IMO.

> We're going to get an app modal prompt in
> the other failure case (download error).

Yes, but only because AIUI the code in question (nsSearchService, right?) has no way to actually create a tabmodal prompt from where it is, because it has no idea who / which tab initiated the issue.

> What are the rules for app modal
> versus tab modal, especially since this error is coming from chrome?

Arguably the issue is with the website, no?

I don't think there are hard and fast rules except that tab-modal is less annoying to the user, so that seems preferable for pretty much anything that is related to the tab.

Really, the best way to signal an error here would be within the search dropdown (e.g. replace the "install foo" blob with a red-background-ish "installing foo failed!" blob), or with a separate error panel pointing there or whatever, but we have no such UI right now and I don't want to get too bogged down trying to design something for this edgecase.

> There are two issues here.
> 
> 1. Yahoo has named it improperly (we're trying to fix that).

For 4 months now? :-(

> 2. We would have to pre download the engine and parse it which no other
> browser does (see bug 351441).

Feels like we could plausibly keep the button but not signal the [+] thing in the icon iff the TLD for the xml file matches the TLD for an existing search engine. Yes, that would break signalling for different searches on the same domain. I don't think that's particularly worrisome. You'd probably need to check if that wasn't a terrible idea with Florian and/or Kev or whoever knows about this stuff, though.

Comment 13

[Mike Kaply [:mkaply]]

(In reply to :Gijs Kruitbosch from comment #12)
> So I just tried adding a non-busted thing, and nothing really happens in
> that case, except we re-focus the search bar and reopen the popup.
> 
> Can't we do that here? Seems to me that if the engine is already installed,
> that is nicer than either a tabmodal/appmodal dialog.

It's not that the engine is already installed necessarily, it's that an engine of the same name already exists.

So the user is not getting the result they wanted - they should be made aware of that. If two engines are completely different but have the same name, we need to inform the user that the choice they made didn't suceed.

Again, my goal here is to at least go back to the behavior before (which is better than the behavior we have now - silently erroring out on the console).sign something for this edgecase.

> > There are two issues here.
> > 
> > 1. Yahoo has named it improperly (we're trying to fix that).
> 
> For 4 months now? :-(

Sadly. But even so, this could be a problem with other sites as well.

> > 2. We would have to pre download the engine and parse it which no other
> > browser does (see bug 351441).
> 
> Feels like we could plausibly keep the button but not signal the [+] thing
> in the icon iff the TLD for the xml file matches the TLD for an existing
> search engine. Yes, that would break signalling for different searches on
> the same domain. I don't think that's particularly worrisome. You'd probably
> need to check if that wasn't a terrible idea with Florian and/or Kev or
> whoever knows about this stuff, though.

That wouldn't work for thinks like yahoo because the TLD for the engine is search.yahoo.com while the TLD for the icon yahoo.com.

Plus many search engines don't use web URLS for icons, they use data URLs.

Comment 14

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > Feels like we could plausibly keep the button but not signal the [+] thing
> > in the icon iff the TLD for the xml file matches the TLD for an existing
> > search engine. Yes, that would break signalling for different searches on
> > the same domain. I don't think that's particularly worrisome. You'd probably
> > need to check if that wasn't a terrible idea with Florian and/or Kev or
> > whoever knows about this stuff, though.
> 
> That wouldn't work for thinks like yahoo because the TLD for the engine is
> search.yahoo.com while the TLD for the icon yahoo.com.

Sorry, I meant the TLD+1; the TLD is ".com", which obviously isn't helpful. The TLD+1 (which some bits of our DNS interfaces can give us for any domain) is "yahoo.com" for both.

> Plus many search engines don't use web URLS for icons, they use data URLs.

I'm not talking about the icon, but the opensearch XML file. If they use a data URI for that, we could actually parse it sync without fetching anything else from the server and avoid this situation altogether.

Comment 15

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > So I just tried adding a non-busted thing, and nothing really happens in
> > that case, except we re-focus the search bar and reopen the popup.
> > 
> > Can't we do that here? Seems to me that if the engine is already installed,
> > that is nicer than either a tabmodal/appmodal dialog.
> 
> It's not that the engine is already installed necessarily, it's that an
> engine of the same name already exists.
> 
> So the user is not getting the result they wanted - they should be made
> aware of that. If two engines are completely different but have the same
> name, we need to inform the user that the choice they made didn't suceed.

I am not convinced that this is the case, except where names are wildly disrespectful of what is "in the box", so to speak. So yes, it's totally possible that some website serves you an opensearch file that is named "Yahoo" and actually searches on Google, or vice versa, and that the user really wants this identically-named-but-subtly-different file. Somewhat less bizarrely, it is possible that Google exposes, say, 5 open search things for web, image, news, groups and maps search.

I don't want to take user choice away, so I'm saying we should leave the button there. But we could avoid the [+] signal in the search bar itself so the user doesn't think there is something "new" to be had on this site, and that will limit the impact of the yahoo issue here, as well as disappointment in other cases.

Comment 16

[Mike Kaply [:mkaply]]

Attachment: Updated patch

I think your idea is a good one and probably warrants a separate bug, but we would still be left with this bug which is that when the user clicked on the menuitem to install the engine, they would get no error and have no idea what happened. I want to fix that regression specifically.

This patch addresses all your comments except that after e10s, the prompt will become app modal. Hopefully by the time e10s is ready, nsPrompter will have been updated to properly support e10s.

Comment 17

[Mike Kaply [:mkaply]]

Attachment: Accidentally used CPOW

I accidentally left in a CPOW reference I was using for testing.

Comment 18

[Florian Quèze [:florian]]

(In reply to Mike Kaply [:mkaply] from comment #13)

> So the user is not getting the result they wanted - they should be made
> aware of that.

Or can we give the user the result they wanted?

If an engine with the same name is already installed, pretending to have succeeded and just letting the user use the existing plugin seems fine. It would be nice to avoid showing the (+) repeatedly in that case of course. Maybe we could achieve this by storing in the metadata of the existing engine the alternative name that was used on the webpage (we may want to limit this behavior to plugins offered from a page with a matching eTLD+1)?

Also, I think there are reasonable cases for users wanting to re-install plugins that already exist:
- a built-in engine has been "removed" (which just sets hidden=true on it)
- an existing engine has been unchecked in the search preferences, causing it to not be visible as a one-off button in the search panel.

So... I think whenever we are attempting to install a duplicate of an engine that's somehow hidden, we should just unhide the existing engine and not surface any error.

Comment 19

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> there are reasonable cases for users wanting to re-install
> plugins that already exist:
> - a built-in engine has been "removed" (which just sets hidden=true on it)

See also bug 363069.

> - an existing engine has been unchecked in the search preferences, causing
> it to not be visible as a one-off button in the search panel.

See also bug 1106924.

Comment 20

[Jim Mathies [:jimm]]

hey Mike, what's the status here?

Comment 21

[Mike Kaply [:mkaply]]

I think it's up to Gijs if he is OK with the patch knowing that with e10s it will switch to a modal prompt.

All his other issues were addressed.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8724192 [details] [diff] [review]
Accidentally used CPOW

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

::: browser/components/search/content/search.xml
@@ +1444,5 @@
> +              let title = searchBundle.GetStringFromName("error_invalid_engine_title");
> +              let text = searchBundle.formatStringFromName("error_duplicate_engine_msg",
> +                                                           [brandName, target.getAttribute("uri")], 2);
> +              let prompt = Cc["@mozilla.org/prompter;1"]
> +                           .getService(Ci.nsIPromptFactory)

Services.prompt.QueryInterface(Ci.nsIPromptFactory);
let prompt = Services.prompt.getPrompt(gBrowser.contentWindow, Ci.nsIPrompt);

@@ +1446,5 @@
> +                                                           [brandName, target.getAttribute("uri")], 2);
> +              let prompt = Cc["@mozilla.org/prompter;1"]
> +                           .getService(Ci.nsIPromptFactory)
> +                           .getPrompt(gBrowser.contentWindow, Ci.nsIPrompt);
> +              let bag = prompt.QueryInterface(Ci.nsIWritablePropertyBag2);

You don't need to assign to a separate variable when you QI, so just do: prompt.QueryInterface(...); without assigning to 'bag', and then prompt.setPropertyAsBool(...);

Comment 23

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #21)
> I think it's up to Gijs if he is OK with the patch knowing that with e10s it
> will switch to a modal prompt.
> 
> All his other issues were addressed.

Well, it's sad, but if we're not willing to invest energy in a better solution, it's better than nothing...

Comment 24

[Mike Kaply [:mkaply]]

Attachment: Final patch address Gijs comments.

Final patch addressing comments.

> Well, it's sad, but if we're not willing to invest energy in a better solution, it's better than nothing...

Agreed. Per our discussion, though, this error case would be the only consumer of any work that allowed chrome to display a tab specific prompt. So at this point, it's not worth the work.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8738733 [details] [diff] [review]
Final patch address Gijs comments.

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

Why is there no commit message? How did you generate this patch? rs=me assuming you set an appropriate commit message, I guess. Please use mozreview or bzexport or hg export in future.

Comment 26

[Mike Kaply [:mkaply]]

Attachment: Proper hg export with commit message

Comment 27

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/603e3444222259d97127198c5e3371fc087f7888
Bug 1222107 - Display error message when installing a duplicate search engine; r=Gijs

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/603e34442222

Comment 29

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

Wontfix for 46 and 47, since we are late in beta 47.

Comment 30

[Camelia Badau [:cbadau], Desktop QA]

I've tested on Windows 7 64bit, Mac OSX 10.11.1 and Ubuntu 14.04 64bit using Firefox 48 Beta 1 (buildID: 20160606200529) and the error message is displayed when trying to add "Yahoo" search engine from Search Bar. Also, after the default "Yahoo" search engine is "removed" or unchecked in about:preferences#search, the error is still displayed - this is what was mentioned in comment 18, comment 19. 

Is anything else that should be tested/covered here?

Comment 31

[Mike Kaply [:mkaply]]

> Is anything else that should be tested/covered here?

I don't believe so.

Comment 32

[Camelia Badau [:cbadau], Desktop QA]

Marking VERIFIED FIXED based on the above comment.