361923 - Can't use remote search engine icons larger than 10KB

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Alex]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

I tested this with a few OpenSearch plugins (not sure if it's like this with Sherlock). I specified an icon (and used the documentation to make sure I did everything correct) in the plugin, but Firefox still used the favicon of the site I tried to install the plugin from.

Reproducible: Always

Steps to Reproduce:
1. Go to any website with a (OpenSearch) search plugin. (Here's a site with a lot: http://www.searchplugins.net/ )
2. Install a search plugin whose icon is different from the favicon.
3. The icon appears different from what is specified.
Actual Results:  
I found the icon used by Firefox was the favicon of the site I installed the plugin from.

Expected Results:  
I expected to see the icon specified by the plugin, not the favicon.

I have no extensions that involve search plugins installed.

Comment 1

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

Alex, do you have a specific example that I can test?

Comment 2

[Alex]

Hmm... Now that I see it, I think if you specify it via Javascript it works; I just tried one on Mycroft and it worked, but noticed the icon is specified via Javascript.

You can try this OpenSearch plugin: http://www.searchplugins.net/pluginlist.aspx?q=mozilla+addons&mode=title (try any without the favicon- the favicon for the site is orange)

Comment 3

[Alex]

...Never mind, any of those did work. This one I created doesn't work, however.  If you compare the two, they're identical (code-wise). Both icons exist.

http://www.searchplugins.net/createos.aspx?number=1177&view=source
(^doesn't work)
http://www.searchplugins.net/createos.aspx?number=340&view=source
(^does work)

Comment 4

[Mycroft Project (CC)]

Er, this is the javascript for OpenSearch admission at Mycroft:

function addOpenSearch(name,ext,cat,pid,meth)
{
  if ((typeof window.external == "object") && ((typeof window.external.AddSearchProvider == "unknown") || (typeof window.external.AddSearchProvider == "function"))) {
    if ((typeof window.external.AddSearchProvider == "unknown") && meth == "p") {
      alert("This plugin uses POST which is not currently supported by Internet Explorer's implementation of OpenSearch.");
    } else {
      window.external.AddSearchProvider(
        "http://mycroft.mozdev.org/installos.php/" + pid + "/" + name + ".xml");
    }
  } else {
    alert("You will need a browser which supports OpenSearch to install this plugin.");
  }
}

The icon is specified within the OpenSearch description.
(Sherlock icons are specified via Javascript)

Comment 5

[Alex]

Whoops, I meant to take a look into the Javascript at Mycroft, but apparently, forgot.

Anyways, does anyone else see this problem?

Comment 6

[Mycroft Project (CC)]

I see the 'wrong' icon for #1177

It's returning:
iconLoadCallback: load failed, or the icon was too large!
http://search.live.com/favicon.ico is a rather fat 29.22 kB

Seems that:
const MAX_ICON_SIZE   = 10000;
http://lxr.mozilla.org/mozilla/source/browser/components/search/nsSearchService.js#90
http://lxr.mozilla.org/mozilla/source/browser/components/search/nsSearchService.js#1327

It seems that it defaults back to the loading site's favicon but I didn't trace that far...

Comment 7

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

Ah, yes. It will reject large icons, because generating a data: URI for them could take a long time (and potentially hang the UI). The limit I chose is rather arbitrary - of the large (1000+) set of search engines I had, only 6 or so had icons greater than 10kb. We should probably consider increasing it if it proves to be a problem, and we should definitely make it clearer to plugin developers what's happening without making them resort to flipping browser.search.update.log (I'm thinking a message in the Error console would be appropriate).

Comment 8

[Mycroft Project (CC)]

Well given we're talking about 16x16 icons they really shouldn't need to be that big - in fact I don't really see how they can be much bigger than 16*16*4 = 1k (for 32 bit) but I don't know much about images...

As far as I'm concerned from a Mycroft point of view, submitted icons are limited to 5k anyway (well now that I've corrected an omission in the OpenSearch submission form anyway :) ) so a 10k limit is irrelevant.

I don't know you normally do for that sort of error message - browser.search.log works well enough for me.

Comment 9

[Jorrit Schippers]

It's not 10 KB, it's 10000 bytes. For instance, http://www.vbulletin.com/forum/favicon.ico does not work, while it's 9.9 KB (it's a bad icon anyway, but I thought it would be useful to mention what the exact maximum is)

Comment 10

[Jezz]

Is there any chance this 10000-byte limit could be increased? Even just to 20000 bytes?

I'd like to use http://www.amazon.com/favicon.ico with an OpenSearch plugin I'm creating for myself, however it won't work because it's 17542 bytes in size. The reason it's so "big" is because this .ico file contains several different sizes of the icon, including 48x48, 32x32, 24x24 and 16x16 - each of them being 32-bit.

I dunno why Amazon has done that, but nevertheless that's how it is, and I'd love to be able to use their file as it is. I can't imagine there would be a huge burden on Firefox to increase this limit, especially as there are only a small handful of sites that do have such large favicon files (as someone wrote above, most sites use just a single 16x16x4 icon file, which is very small).

Comment 11

[Mycroft Project (CC)]

A few examples of big favicon.ico:
(NB  View Image Info reports the 16x16 page size not the actual file size)
http://www.biblegateway.com/favicon.ico (13 046 bytes)
http://rae.es/favicon.ico (10 862 bytes)
http://nashape.com/favicon.ico (32 988 bytes)

The above were memorable (and slightly weird) recent examples /
(I went looking for one with a 64x64 page for the last.)

But Google / Amazon are getting bigger...
Google's favicon now includes 16 and 32 square icons
and as Jezz says Amazon's includes 24 and 48 square icons as well.

My approach in the past has been just to chop the 16x16 out but as mobile now wants (if not fully supports as far as I can tell) 32x32 I'm less inclined to do this. And I've seen sites with multiple bit depth icons so that the file size is greater than 10 000 bytes even with just 16 and 32 square icons.

Not sure what the best maximum would be though...

Comment 12

[Mycroft Project (CC)]

Because I found it while looking for something else and mostly as a reminder to myself in the future:
https://github.com/audreyr/favicon-cheat-sheet

Comment 15

[alex_mayorga]

¡Hola Marco!

Coming from https://bugzilla.mozilla.org/show_bug.cgi?id=1248038 Thanks Alice!

What's the "right way" to fix this bug?

Do outreach to the sites when this is found?

A few sites where I've found this bug FWIW:

https://es.wiktionary.org/wiki/
https://fr.wiktionary.org/wiki/
http://www.wolframalpha.com/

¡Gracias!
Alex

Comment 16

[Florian Quèze [:florian]]

The possible ways forward:
- do outreach to the sites (but this won't ever fix every possible site with the issue...)
- slightly increase the size limit and add an explicit error message in the error console when an image is above the limit (as suggested in comment 7).
- resize the image before saving it. I'm increasingly thinking this is what we should do. See bug 389273 for how this was implemented for favicons.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> - slightly increase the size limit and add an explicit error message in the
> error console when an image is above the limit (as suggested in comment 7).

A bump would make sense considering how common is hi-dpi today.

> - resize the image before saving it. I'm increasingly thinking this is what
> we should do. See bug 389273 for how this was implemented for favicons.

Favicons decision has a few downsides to consider:
1. the chosen limit was too strict, it doesn't play well with hi-dpi. But that had a reason, since all icons are stored in the same file as visits, that is also the same file powering the awesomebar. In the search egines case the limit can be larger, since it's unlikely the user has thousands of engines, but still sounds like everything ends up in the search.json file, so the limit should not be large enough to cause performance problems when parsing a file with (let's guess) 100 engines. Requires some measurement of the impact on low-end hardware.
2. Resizing clearly doesn't play well with some formats, for example animated images will become still, multi-image formats (ico) may become single image, and so on.

What I'd suggest is half-way between the 2 approaches:
1. slightly enlarge the current limit
2. resize anything above the limit
3. if the resize has losses (from animated/multi to single image) notify the problem in the error console so it can get noticed by web devs.
4. Add telemetry for how often we have to resize an icon, how much we take to go from plain icon to the final encoded data uris (included resize) and, if possible, how much time we take to encode/decode the search.json file (but maybe we already have this measurement?).

Comment 18

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #17)

> What I'd suggest is half-way between the 2 approaches:
> 1. slightly enlarge the current limit
> 2. resize anything above the limit
> 3. if the resize has losses (from animated/multi to single image) notify the
> problem in the error console so it can get noticed by web devs.

Seems good to me.

> 4. Add telemetry for how often we have to resize an icon, how much we take
> to go from plain icon to the final encoded data uris (included resize) and,
> if possible, how much time we take to encode/decode the search.json file
> (but maybe we already have this measurement?).

Isn't this a bit excessive for a rare edge case? That case is about someone installing a custom search engine (already not very common) with a large icon (uncommon, as this currently causes the icon to be rejected), on low-end hardware (probably not very common for users who like to customize stuff).

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> Isn't this a bit excessive for a rare edge case?

How we can tell it's rare without data? If we already have such data then I agree it's not worth adding more probes.
Though, the fact this bug exists with various examples, means sites in the wild are doing that. Knowing how often it happens could be useful to tweak the size limit. The search.json measurement is something I expect we should already have, if it impacts startup in any way. The encode time could be overzealous, I agree.
Adding a probe is extremely cheap and it has an expire date, so we can check if the initially selected limit works fine in the wild, and then just remove the probe once expired.

Comment 20

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #19)
> (In reply to Florian Quèze [:florian] [:flo] from comment #18)
> > Isn't this a bit excessive for a rare edge case?
> 
> How we can tell it's rare without data? If we already have such data then I
> agree it's not worth adding more probes.

I have data about default engines that are installed from open search (and I know it's rare). I don't have data about non-default engines installed from open search though, and I guess it would be interesting to collect to better understand how many engines people install.

> Though, the fact this bug exists with various examples, means sites in the
> wild are doing that.

We only have a handful of examples, for a bug that's been around for almost a decade; that makes me think it's rare.

> The search.json measurement is something I expect we should
> already have, if it impacts startup in any way.

We have probes recording the time it takes to initialize the search service (which happens asynchronously), not for the time it takes to parse the json file.

Comment 21

[julien.darthenay]

Hello.

Does the Wiktionnaries icons record as "rare"?

Comment 22

[julien.darthenay]

If I understand clearly the problem, you are intentionality putting a limit to the icon file to void the search.json file to turn too heavy. Actually I agree a lot with that. I would even say the 10k limit  could even be seen as too large. But, why to keep the whole icon in the search.json? Wouldn't it be better to keep only the 16x16 resolution icons and throw away the rest of the icon file? It is that much difficult to keep only the interesting icons in the file of each search engine?

Here are some a few other too heavy icons (most of them were not a few years ago):
- Wiktionary
- wordreference.com
- start.me

Comment 23

[Thomas Bertels]

Only a few examples exist of search engines favicons larger than 10 000 bytes and all of them seem to be below 20 000 bytes. So it may be a good idea to fix this bug by increasing the limit to 20 000 bytes instead of a lengthier and larger approach.

Optionally, a probe could be added to check how often that new limit is large enough for search engines.

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 361923 - Can't use remote search engine icons larger than 10KB.

Review commit: https://reviewboard.mozilla.org/r/195774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195774/

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8924496 [details]
Bug 361923 - Can't use remote search engine icons larger than 10KB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195774/diff/1-2/

Comment 26

[Florian Quèze [:florian]]

Comment on attachment 8924496 [details]
Bug 361923 - Can't use remote search engine icons larger than 10KB.

https://reviewboard.mozilla.org/r/195774/#review200926

Looks good, thanks!

::: toolkit/components/search/nsSearchService.js:354
(Diff revision 2)
> + * Tries to rescale an icon to a given size.
> + *
> + * @param aByteArray Byte array containing the icon payload.
> + * @param aContentType Mime type of the payload.
> + * @param [optional] aSize desired icon size.
> + * @throws if the icon canno be rescaled or the rescaled icon is too big.

typo: 'canno'

::: toolkit/components/search/nsSearchService.js:357
(Diff revision 2)
> + * @param aContentType Mime type of the payload.
> + * @param [optional] aSize desired icon size.
> + * @throws if the icon canno be rescaled or the rescaled icon is too big.
> + */
> +function rescaleIcon(aByteArray, aContentType, aSize = 32) {
> +  if (!aContentType || aContentType == "image/svg+xml")

When do we expect aContentType to be null? The code before the patch didn't null check it. And if it does turn out to sometimes be null, "SVG image" is a misleading error.

::: toolkit/components/search/nsSearchService.js:368
(Diff revision 2)
> +  let stream = imgTools.encodeScaledImage(container, "image/png", aSize, aSize);
> +  let size = stream.available();
> +  if (size > MAX_ICON_SIZE)
> +    throw new Error("Icon is too big");
> +  let bis = Cc["@mozilla.org/binaryinputstream;1"]
> +              .createInstance(Ci.nsIBinaryInputStream);

It seems inconsistent to use Components.Constructor for ArrayBufferInputStream but not for BinaryInputStream in the same function. Could you please pick one style and stick to it? :-)

::: toolkit/components/search/nsSearchService.js:1784
(Diff revision 2)
> +            try {
> +              LOG("iconLoadCallback: rescaling icon");
> +              [aByteArray, contentType] = rescaleIcon(aByteArray, contentType);
> +            } catch (ex) {
> +              LOG("iconLoadCallback: icon too big");
> +              Cu.reportError("Unable to set an icon for the search engine because it is too big.");

Could we report the actual exception 'ex' here?

::: toolkit/components/search/tests/xpcshell/test_big_icon.js:12
(Diff revision 2)
> +  let srv = useHttpServer();
> +  srv.registerContentType("ico", "image/x-icon");
> +  await asyncInit();
> +
> +  let promiseChanged = new Promise(resolve => {
> +    Services.obs.addObserver( function observe(engine, topic, verb) {

I'm surprised eslint doesn't complain about the space between '(' and 'function' here.

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8924496 [details]
Bug 361923 - Can't use remote search engine icons larger than 10KB.

https://reviewboard.mozilla.org/r/195774/#review200926

> When do we expect aContentType to be null? The code before the patch didn't null check it. And if it does turn out to sometimes be null, "SVG image" is a misleading error.

the !aContentType was a leftover from a previous version, good catch.

> I'm surprised eslint doesn't complain about the space between '(' and 'function' here.

Eslint doesn't seem to complain for that case.

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8924496 [details]
Bug 361923 - Can't use remote search engine icons larger than 10KB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195774/diff/2-3/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c551c669a7f
Can't use remote search engine icons larger than 10KB. r=florian

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8c551c669a7f

Comment 31

[jodyirawan]

If I understand clearly the problem, you are intentionality putting a limit to the icon file to void the search.json file to turn too heavy. Actually I agree a lot with that. I would even say the 10k limit could even be seen as too large. But, why to keep the whole icon in the search.json? Wouldn't it be better to keep only the 16x16 resolution icons and throw away the rest of the icon file? It is that much difficult to keep only the interesting icons in the file of each search engine?

Here are some a few other too heavy icons (most of them were not a few years ago):

https://situsjudionlineresmi.com/

Comment 32

[dikta wijaya]

Welcome to my page! I can't tell you how excited I am to have found this place. My favorite TV shows are Curb Your Enthusiasm, Rick and Morty, Twin Peaks. I was born in Cleveland but grew up in Indonesia. I believe in living life to the fullest and following your dreams, no matter how hard they may be. At this time, I'm learning how to play the Double Bass. Feel free to private message me.

Offcial Website : https://qqcuan.com/

microsoft : https://social.msdn.microsoft.com/Profile/Dikta%20Wijaya

Comment 33

[harrysogar]

When do we expect aContentType to be null? The code before the patch didn't null check it. And if it does turn out to sometimes be null, "SVG image" is a misleading error.
Link : https://18.142.63.79/

Comment 34

[Mark Banner (:standard8)]

Restricting comments as this is now gathering spam.