1215518 - Cannot Pin site with invalid favicon

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect, P2)

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

STR:

STR:
1. Load http://www.bmwusa.com/ in browser window
2. Click context menu and select "Pin"

Expected:
Pinning menu pops up

Actual:
Nothing happens.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1215518-Cannot-Pin-site-with-invalid-favicon > mozilla-b2g:master

Comment 2

[Guillaume Marty [:gmarty]]

Comment on attachment 8675658 [details] [review]
[gaia] gmarty:Bug-1215518-Cannot-Pin-site-with-invalid-favicon > mozilla-b2g:master

Alberto, what do you think of this PR?

Comment 3

[Alberto Pastor [:albertopq]]

Comment on attachment 8675658 [details] [review]
[gaia] gmarty:Bug-1215518-Cannot-Pin-site-with-invalid-favicon > mozilla-b2g:master

I'm not too sure about relying on the content-type for this, but can't think on better solutions. May be a try/catch trying to build an image with the blob?

Anyway, I'm happy to land this with a test for the time being.

Thanks!

Comment 4

[Guillaume Marty [:gmarty]]

Landed in https://github.com/mozilla-b2g/gaia/commit/63c4256bd726b83d06e4935aa7fadfb3f711db67

Comment 5

[Nigel Babu [:nigelb]]

I had to back this out for Gij(27) perma orange. Unfortunately, it has also been merged around. Waiting to see if the backout helps.

Comment 6

[Nigel Babu [:nigelb]]

This back out did help. Please make sure Gij17 is green before re-landing. Another instances of false greens on gaia because of the differences in build.

Comment 7

[Nigel Babu [:nigelb]]

(Er, Gij27, not Gij17)

Comment 8

[Guillaume Marty [:gmarty]]

If the failing test was task_manager_icons_test.js, it's going to be fixed by Bug 1215476. I'll wait till this one lands before retrying the PR here.

Comment 9

[Nigel Babu [:nigelb]]

It is indeed. Argh, I've just realized I didn't link the failure! Sorry about that. This is the failure -> https://treeherder.mozilla.org/logviewer.html#?job_id=3118666&repo=b2g-inbound

Comment 10

[Ben Francis [:benfrancis]]

(In reply to Alberto Pastor [:albertopq] from comment #3)
> I'm not too sure about relying on the content-type for this, but can't think
> on better solutions. May be a try/catch trying to build an image with the
> blob?

FWIW in the browser app we used to check for a valid image by creating an invisible img element, setting the src and checking for a non-zero and not-extremely-large width/height. This helps both filter out invalid images and avoid extremely large image files which could be used to try to exploit bugs in the image decoder (and take up lots of storage).

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1215518-Cannot-Pin-site-with-invalid-favicon-new > mozilla-b2g:master

Comment 12

[Guillaume Marty [:gmarty]]

Comment on attachment 8678108 [details] [review]
[gaia] gmarty:Bug-1215518-Cannot-Pin-site-with-invalid-favicon-new > mozilla-b2g:master

I changed the patch completely as the type approach didn't work with local assets. Alberto, can you take another look?
As Ben said, the blob is loaded into a detached image and the error event wasn't previously listened to in icon.renderBlob(). If it's not an image, it now fails.

Post v2.5 we need to promisify icon.renderBlob and clean IconsHelper. It's getting hard to maintain.

Comment 13

[Alberto Pastor [:albertopq]]

Comment on attachment 8678108 [details] [review]
[gaia] gmarty:Bug-1215518-Cannot-Pin-site-with-invalid-favicon-new > mozilla-b2g:master

LGTM Thanks!

Comment 14

[Guillaume Marty [:gmarty]]

Landed in master in:
https://github.com/mozilla-b2g/gaia/commit/bde2717ffc50b14492413ff38348f2773e1c6fdd

Comment 15

[Alison Shiue]

Verified on

[Flame]
Build ID               20151027150240
Gaia Revision          a26eadc5e1133d5112b6cbc10badbb7670a1090f
Gaia Date              2015-10-27 17:36:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151027.184038
Firmware Date          Tue Oct 27 18:40:49 EDT 2015
Bootloader             L1TC000118D0

[Aries]
Build ID               20151027221526
Gaia Revision          a26eadc5e1133d5112b6cbc10badbb7670a1090f
Gaia Date              2015-10-27 17:36:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151027.213419
Firmware Date          Tue Oct 27 21:34:27 UTC 2015
Bootloader             s1