Blob URLs as favicons don't work

VERIFIED FIXED in Firefox 48

Status

()

Firefox
Tabbed Browser
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: mrbkap, Assigned: blassey)

Tracking

({productwanted})

unspecified
Firefox 48
productwanted
Points:
---

Firefox Tracking Flags

(e10s+, firefox47 wontfix, firefox48 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
The code that deals with favicons doesn't handle "host object URLs" specially [1][2] and simply passes them through. In e10s, this means that the favicon simply doesn't work, which causes some sites that generate them (such as WhatsApp Web) to have a blank favicon.

Fixing this seems a little painful. The easiest way would be to slurp the blob as a data URI and set that in the parent instead, but converting blobs to data URIs is not a terribly clean solution. A more correct solution would be to detect this case and get the blob itself to send to the parent. The parent could then create its own blob URL and set that as the favicon URI. Doing this would mean that the child would have to notify the parent when the blobs in question were revoked, which could be tricky, though.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm?rev=330d323a5043#78
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=83cc3090b16c#3361
(Assignee)

Updated

2 years ago
tracking-e10s: ? → +
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1217439
(Reporter)

Updated

2 years ago
Keywords: productwanted
Product Triage: Favicons in the most basic use cases work. We can't glean how widespread this issue is; can you specify what a blob URL is?
Flags: needinfo?(mrbkap)
It sounds like the only site this happens on is What'sApp but that seems pretty important. Blake, can you provide level of effort to fix this and what the general risk profile looks like?
(Reporter)

Comment 4

2 years ago
This shouldn't be too hard to fix and the fix should be very safe (limited only to cases where sites use this feature).
Flags: needinfo?(mrbkap)
Component: General → Tabbed Browser

Updated

2 years ago
Duplicate of this bug: 1250877

Updated

2 years ago
Duplicate of this bug: 1253087

Updated

2 years ago
Priority: -- → P1
Created attachment 8734598 [details] [diff] [review]
blob_favicons.patch
Assignee: nobody → blassey.bugs
Attachment #8734598 - Flags: review?(felipc)
Created attachment 8734746 [details] [diff] [review]
blob_favicons.patch

This should handle the case where we don't get all the data in one onDataAvailable() call
Attachment #8734598 - Attachment is obsolete: true
Attachment #8734598 - Flags: review?(felipc)
Attachment #8734746 - Flags: review?(felipc)
Comment on attachment 8734746 [details] [diff] [review]
blob_favicons.patch

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

::: browser/modules/ContentLinkHandler.jsm
@@ +122,5 @@
> +                let spec = "data:" + channel.contentType + ";base64," + this.encoded;
> +                chromeGlobal.sendAsyncMessage(
> +                  "Link:SetIcon",
> +                  {url: spec, loadingPrincipal: link.ownerDocument.nodePrincipal});
> +                iconAdded = true;

I think you can immediatelly set iconAdded = true outside of the listener, instead of waiting for when the encoding completes.

Note that bug 1186139 has been backed out and this patch is touching the same code as that one. So it might be better to wait for that to reland.
Attachment #8734746 - Flags: review?(felipc) → review+
(Assignee)

Updated

2 years ago
Depends on: 1186139

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/073bbd96f757

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/073bbd96f757
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8734746 [details] [diff] [review]
blob_favicons.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Some websites, primarily interactive applications like WhatsApp will be missing their favicons
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: worst case would seem to be a different exception throw in chrome js
[String/UUID change made/needed]:
Attachment #8734746 - Flags: approval-mozilla-aurora?
Blake, is this issue fixed on Nightly? I just want to check because the uplift request had nothing filled out for test coverage. Thanks!
Flags: needinfo?(mrbkap)

Updated

2 years ago
status-firefox47: --- → affected
(Reporter)

Comment 14

2 years ago
(In reply to Ritu Kothari (:ritu) from comment #13)
> Blake, is this issue fixed on Nightly? I just want to check because the
> uplift request had nothing filled out for test coverage. Thanks!

Yes, I can confirm that this is fixed on Nightly.
Flags: needinfo?(mrbkap)
Thanks for the verification Blake!
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
Comment on attachment 8734746 [details] [diff] [review]
blob_favicons.patch

Support for blob URL favicons, noticeably Watsapp is impacted, Aurora47+
Attachment #8734746 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems uplifting, can you take a look, thanks!

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 073bbd96f757
grafting 336626:073bbd96f757 "bug 1184739 - Blob URLs as favicons don't work r=felipe"
merging browser/modules/ContentLinkHandler.jsm
warning: conflicts while merging browser/modules/ContentLinkHandler.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(blassey.bugs)
blassey is on PTO for a few weeks - needinfo'ing felipe who reviewed.
Flags: needinfo?(blassey.bugs) → needinfo?(felipc)
(In reply to Carsten Book [:Tomcat] from comment #17)
> has problems uplifting, can you take a look, thanks!
> 


The problem with this uplift is that this code depends on bug 1186139, which got approval- for 47. If we're sure that e10s is not gonna go to release on 47, I think we can also not uplift this one and let it ride for 48.

Ritu, what do you think?
Flags: needinfo?(felipc) → needinfo?(rkothari)

Updated

2 years ago
Flags: needinfo?(rkothari)
(In reply to :Felipe Gomes (needinfo me!) from comment #19)
> (In reply to Carsten Book [:Tomcat] from comment #17)
> > has problems uplifting, can you take a look, thanks!
> > 
> 
> 
> The problem with this uplift is that this code depends on bug 1186139, which
> got approval- for 47. If we're sure that e10s is not gonna go to release on
> 47, I think we can also not uplift this one and let it ride for 48.
> 
> Ritu, what do you think?

Hi Felipe, is this a common scenario? Just the Watsapp association makes me think this is an imp scenario and if e10s is enabled for 20% of the beta population (even though it is not on by default for 47 release), we should uplift this and bug 1186139 as well.
On second thoughts, if the user impact here is only a blank favicon (I read comment 0 again) versus uplifting 3-4 patches, I think we can easily wontfix this for Fx47. Please let me know if there are any concerns.

Updated

2 years ago
status-firefox47: affected → wontfix

Updated

2 years ago
Attachment #8734746 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.