Closed Bug 1184739 Opened 4 years ago Closed 4 years ago

Blob URLs as favicons don't work

Categories

(Firefox :: Tabbed Browser, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox47 --- wontfix
firefox48 --- verified

People

(Reporter: mrbkap, Assigned: blassey)

References

Details

(Keywords: productwanted)

Attachments

(1 file, 1 obsolete file)

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
Duplicate of this bug: 1217439
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?
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
Duplicate of this bug: 1250877
Duplicate of this bug: 1253087
Priority: -- → P1
Attached patch blob_favicons.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #8734598 - Flags: review?(felipc)
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+
Depends on: 1186139
https://hg.mozilla.org/mozilla-central/rev/073bbd96f757
Status: NEW → RESOLVED
Closed: 4 years ago
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)
(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
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)
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.
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.