633558 - Installation of search engines fails in Firefox < 4.*

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[krupa raj[:krupa]]

steps to reproduce:
1. Load https://addons.mozilla.org/en-US/firefox/search-tools/bookmarks
2. Click on "Add to Firefox" for any of the listed add-ons

observed behavior:
Install Error
This search engine isn't supported by Firefox and can't be installed.

Note: works fine in FF 4

Comment 1

[krupa raj[:krupa]]

Attachment: install error

Comment 2

[Wil Clouser [:clouserw]]

We can't reproduce this in 3.6.13.  What add-on did you try to install?

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

I can reproduce in 3.6.13 (Windows), and I get:

Error: Invalid argument passed to window.sidebar.addSearchEngine: Unsupported search icon URL.
Source File: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsSidebar.js
Line: 112

In nsSidebar.prototype.validateSearchEngine =
function (engineURL, iconURL), the part that's failing is:

// Make sure we're using HTTP, HTTPS, or FTP and refering to a
    // .gif/.jpg/.jpeg/.png/.ico file for the icon.
    if (iconURL &&
        ! /^(https?|ftp):\/\/.+\.(gif|jpg|jpeg|png|ico)$/i.test(iconURL))
      throw "Unsupported search icon URL.";

Mossop, any ideas?

Comment 4

[Stephen Donner [:stephend] Not actively reading bugmail]

OK; works fine from top-level, such as https://addons.mozilla.org/en-US/firefox/search-tools/, (prod/preview/next), and fails with the above error, on 3.6.* with the sub-categories (comment 0).  If I knew how to use Firebug, I'd tell you what the value of iconURL is, and why it's failing i.test

Comment 5

[Wil Clouser [:clouserw]]

The most recent thing messing with this was the switch to putting icons on the static.amn CDN.  I don't know why this would work for top level pages, and fx4 though.

Sending a cache busting string (?blah on the end of the URL) wouldn't match that regex - that could be what's happening.  I don't have 3.6 here to test.

Comment 6

[Justin Scott [:fligtar]]

This seems like a critical bug that should have been fixed Friday, unless I'm misunderstanding something?

Comment 7

[Wil Clouser [:clouserw]]

(In reply to comment #6)
> This seems like a critical bug that should have been fixed Friday, unless I'm
> misunderstanding something?

It's important.  Mossop - can you tell us why this works in FF4 but not 3.6?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

We loosened the iconURL validity check in 4.0, in bug 550290. If comment 5 is correct (?blah causing the failure), then I guess this is invalid?

Comment 9

[Wil Clouser [:clouserw]]

Thanks Gavin.  It's still valid, since we still support 3.6.  It makes an unfortunately specific case in our caching strategy though.

Comment 10

[Justin Scott [:fligtar]]

This isn't the first time we've run into the question mark breaking InstallTrigger either, unfortunately. Though last time it was extensions I think.

Comment 11

[Wil Clouser [:clouserw]]

Actually, I don't know how this would be our fault.  Search engines either use data URLs or href's in their opensearch XML, but neither of them should be coming from AMO.

QA:  What add-on are you failing to install, specifically?

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #11)

> QA:  What add-on are you failing to install, specifically?

All/any of them in a sub-category of /search-tools/.  Another data point is that when I try to install from /search-tools/ on preview/allizom, I get the requisite "Firefox prevented this site (addons.allizom.org) from asking you to install software on your computer" info-bar, while I don't on /search-tools/bookmarks, e.g.

Try: SearchGeek for Firefox or Google.com.tr, or any of those; again, other categories fail too, like News & Blogs (so, Bing engine fails to install).

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

in-testsuite? and in-litmus? -- we shouldn't regress this again.  Not sure if it's best-addressed by a Django unittest or if we could whip something up in Selenium, at least.

Comment 14

[Wil Clouser [:clouserw]]

It looks like it doesn't even request the icon in the opensearch package, it just loads directly off the page (doesn't make any more http requests after clicking).

So yeah, it's the cache buster.

Comment 15

[Jeff Balogh (:jbalogh)]

We call this to add a search engine:

        window.external.AddSearchProvider(url);

That url is something like https://addons.mozilla.org/firefox/downloads/latest/10772/addon-10772-latest.xml?src=category (whatever the install button's href is).

I don't see how any image URLs from AMO would get in there.

Comment 16

[Andy McKay]

Its using the favicon for the tab at the time.

http://hg.mozilla.org/mozilla-central/file/d843b4e06f1b/browser/components/sidebar/src/nsSidebar.js#l195

https://developer.mozilla.org/en/XUL/Method/getIcon

The favicon is something like:

https://static.addons.mozilla.net/media/img/favicon.ico?b=be18645

Comment 17

[Andy McKay]

I think the change was here:

https://github.com/jbalogh/zamboni/commit/c865421e#L18L11

Comment 18

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

We have the same problem in our Mozmill test for the SearchGeek search engine. See bug 634429.

Comment 19

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

And this happens also in Firefox 3.5.

Comment 20

[Andy McKay]

https://github.com/jbalogh/zamboni/commit/66920051

Comment 21

[krupa raj[:krupa]]

verified on 3.6.13 and 3.5.

Comment 22

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Has this already been pushed live? Given our latest Mozmill tests for 3.6.14 build 3 it's still broken:

http://mozmill-release.brasstacks.mozilla.com/#/general/report/cf236ad19b037e87fe0fe25cddfccbef

Comment 23

[krupa raj[:krupa]]

This is fixed in preview and was pushed live in 5.12.9.4. But, I can reproduce the issue on prod.

Comment 24

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Thanks Krupa => Reopening because it is still an issue on AMO.

Comment 25

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

What I forgot to mention, I can't see this behavior on allizom. Do we simply have forgotten to push this patch to production?

Comment 26

[Wil Clouser [:clouserw]]

Yep, this was fixed last night.

Comment 27

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Works now on AMO. Marking as verified fixed.