Closed Bug 1215518 Opened 6 years ago Closed 6 years ago

Cannot Pin site with invalid favicon

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+)

VERIFIED FIXED
blocking-b2g 2.5+

People

(Reporter: mikehenrty, Assigned: gmarty)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 1 obsolete file)

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.
QA Whiteboard: [COM=Pin the Web]
Assignee: nobody → 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?
Attachment #8675658 - Flags: review?(apastor)
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!
Attachment #8675658 - Flags: review?(apastor) → review+
Priority: -- → P2
Landed in https://github.com/mozilla-b2g/gaia/commit/63c4256bd726b83d06e4935aa7fadfb3f711db67
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Er, Gij27, not Gij17)
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.
Flags: needinfo?(nigelbabu)
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
Flags: needinfo?(nigelbabu)
(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 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.
Attachment #8678108 - Flags: review?(apastor)
Depends on: 1215476
Attachment #8675658 - Attachment is obsolete: true
Comment on attachment 8678108 [details] [review]
[gaia] gmarty:Bug-1215518-Cannot-Pin-site-with-invalid-favicon-new > mozilla-b2g:master

LGTM Thanks!
Attachment #8678108 - Flags: review?(apastor) → review+
Landed in master in:
https://github.com/mozilla-b2g/gaia/commit/bde2717ffc50b14492413ff38348f2773e1c6fdd
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.