1184739 - Blob URLs as favicons don't work

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 2

[Erin Lancaster [:elan]]

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?

Comment 3

[Erin Lancaster [:elan]]

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?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This shouldn't be too hard to fix and the fix should be very safe (limited only to cases where sites use this feature).

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: blob_favicons.patch

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: blob_favicons.patch

This should handle the case where we don't get all the data in one onDataAvailable() call

Comment 9

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 10

[Pulsebot]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/073bbd96f757

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

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]:

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Blake, is this issue fixed on Nightly? I just want to check because the uplift request had nothing filled out for test coverage. Thanks!

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks for the verification Blake!

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8734746 [details] [diff] [review]
blob_favicons.patch

Support for blob URL favicons, noticeably Watsapp is impacted, Aurora47+

Comment 17

[Carsten Book [:Tomcat]]

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)

Comment 18

[Mike Conley (:mconley) (:⚙️)]

blassey is on PTO for a few weeks - needinfo'ing felipe who reviewed.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

(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?

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.