1246076 - Tab records don't include the page's favicon

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0012-Bug-XXXXXXX-include-the-favicon-in-synced-tabs-recor.patch

The tabs engine attempts to get the favicon via:

| icon: tabState.attributes && tabState.attributes.image || "",

however, the icon is actually stored in the image attribute on the tabstate itself - https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/components/sessionstore/TabState.jsm#187

With the following fix I see the incoming synced tabs get an icon URL (and thus it should automagically appear in the awesomebar.

Over in bug 428378, there are concerns about the privacy implications of this (ie, the URL of the favicon is stored, not the data itself) - which is reasonable - however, the tabs engine *tries* to get the icon, but fails - ie, it appears this patch is fixing an old regression.

An alternative to this might be to use places to fetch the favicon as a data: URL (and I guess with an upper limit on the size) which would fix the "favicon are missing" issue for synced tabs, but not for bookmarks and history while still respecting privacy.

Thoughts?

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 8716177 [details] [diff] [review]
0012-Bug-XXXXXXX-include-the-favicon-in-synced-tabs-recor.patch

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

This seems fine to me.

We can't do anything more invasive without breaking deployed clients (and doing work on at least two other platforms).

If we were going to change the tab record format, there are more valuable things I'd suggest working on in the tabs format, like incremental representation -- Bug 1222594 Comment 4 has some pointers.

Furthermore, syncing icon data is a little more complex than just dumping the Places data into the record -- Bug 428378 Comment 41 and later outline some of the reasons why. Most noticeably in the context of tabs (a single record for all data), this would balloon the size of the tabs record, which would have an impact on bandwidth consumption and also push a larger percentage of users over the maximum record size for their open tabs, breaking sync completely for those users.

Comment 2

[Chris Karlof [:ckarlof]]

Comment on attachment 8716177 [details] [diff] [review]
0012-Bug-XXXXXXX-include-the-favicon-in-synced-tabs-recor.patch

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

SGTM, but does it actually fix any problems, i.e., do any favicons now show up that didn't previously?

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Chris Karlof [:ckarlof] from comment #2)
> SGTM, but does it actually fix any problems, i.e., do any favicons now show
> up that didn't previously?

It will cause favicons shown in all synced-tabs UI to be correct where they aren't currently. Probably also in the awesomebar. What it will *not* fix is favicons for synced bookmarks or history entries.

However, I'm still concerned about the privacy implications  - it means that favicons for all synced-tabs will be fetched as the UI is shown, which is exactly the concern voiced in bug 428378 comment 41. I'll investigate turning the favicon into a data: url to avoid this (Places already has functions for exactly this)

Comment 4

[Richard Newman [:rnewman]]

(In reply to Mark Hammond [:markh] from comment #3)

> However, I'm still concerned about the privacy implications  - it means that
> favicons for all synced-tabs will be fetched as the UI is shown, which is
> exactly the concern voiced in bug 428378 comment 41. I'll investigate
> turning the favicon into a data: url to avoid this (Places already has
> functions for exactly this)

This'll break iOS and Android, and of course all of the other mentioned problems (bandwidth, which sizes to use, etc.) still apply.

Rather than try to hack Places-specific ad hoc favicon syncing in here, perhaps simply don't auto-fetch icons, or follow some more sophisticated policy?

Comment 5

[Chris Karlof [:ckarlof]]

(In reply to Mark Hammond [:markh] from comment #3)
> (In reply to Chris Karlof [:ckarlof] from comment #2)
> > SGTM, but does it actually fix any problems, i.e., do any favicons now show
> > up that didn't previously?
> 
> It will cause favicons shown in all synced-tabs UI to be correct where they
> aren't currently. Probably also in the awesomebar. What it will *not* fix is
> favicons for synced bookmarks or history entries.

That's a pretty good outcome.

> However, I'm still concerned about the privacy implications  - it means that
> favicons for all synced-tabs will be fetched as the UI is shown, which is
> exactly the concern voiced in bug 428378 comment 41.

I'm personally not concerned. If you have a network level attacker tracking you, you have bigger problems than someone seeing what IP addresses these favicon fetches map to. 

My strawman proposal: proceed with the fix as-is and add a (synced?) about:config pref to turn off synced tab favicons for those concerned.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1246076-part-1-include-the-favicon-in-synced-tab.patch

This patch builds on the previous one - the favicon for a tab is always fetched.

This patch adds a new pref |services.sync.syncedTabs.showRemoteIcons| with a default of true, and arranges for this pref to be synced. This pref doesn't change what is synced, but does change whether the synced icon is displayed or not.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1246076-part-2-optionally-display-a-remote-tab-s.patch

Mak, we recently discovered that synced tabs did *not* correctly sync favicons for remote tabs. The previous patch fixes that, but also introduces a new pref that controls whether the favicon is actually shown or not (the default is true, but the pref exists if people have privacy concerns with this - see previous comments.)

This patch does the same as the previous patch, but for the autocomplete provider. If the pref |services.sync.syncedTabs.showRemoteIcons| is true, we show the synced icon, otherwise use the default favicon.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8720673 [details] [diff] [review]
0003-Bug-1246076-part-2-optionally-display-a-remote-tab-s.patch

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

::: toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm
@@ +98,5 @@
>      }
>  
> +    let showIcons = true;
> +    try {
> +      showIcons = Services.prefs.getBoolPref("services.sync.syncedTabs.showRemoteIcons");

I'm not thrilled of having to read a preference for every typed char, but the only alternative would be to observe changes, supposing the user can flip this at any time and we are expected to react immediately for privacy reasons.
OTOH, IIRC reading prefs should be generally very fast.
What do you think, is it worth the complication?

::: toolkit/components/places/tests/unifiedcomplete/test_remotetabmatches.js
@@ +5,5 @@
>  
>  Cu.import("resource://services-sync/main.js");
>  
> +const faviconService = Cc["@mozilla.org/browser/favicon-service;1"]
> +                       .getService(Ci.nsIFaviconService);

Please Use PlacesUtils.favicons instead, PlacesUtils is already in scope.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Marco Bonardo [::mak] from comment #8)
> I'm not thrilled of having to read a preference for every typed char, but
> the only alternative would be to observe changes, supposing the user can
> flip this at any time and we are expected to react immediately for privacy
> reasons.

TBH, I doubt anyone is actually going to flip this pref in practice, so I think it would be fine to require a restart after flipping this pref. I'll change this patch to read the pref once at initialization time, and add a comment to the pref itself indicating a restart is required.

Thanks.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8720671 [details] [diff] [review]
0002-Bug-1246076-part-1-include-the-favicon-in-synced-tab.patch

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

::: services/sync/modules/SyncedTabs.jsm
@@ +63,5 @@
>  
>    /* Make a "tab" record. Returns a promise */
>    _makeTab: Task.async(function* (client, tab, url) {
> +    let icon;
> +    if (Preferences.get("services.sync.syncedTabs.showRemoteIcons", true)) {

I'm not convinced that we need this change here. Just because you don't want to show remote icons doesn't mean they're not a legitimate part of the tab record.

If not, we should check this once, not once per tab.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 8720671 [details] [diff] [review]
> 0002-Bug-1246076-part-1-include-the-favicon-in-synced-tab.patch
> 
> Review of attachment 8720671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/modules/SyncedTabs.jsm
> @@ +63,5 @@
> >  
> >    /* Make a "tab" record. Returns a promise */
> >    _makeTab: Task.async(function* (client, tab, url) {
> > +    let icon;
> > +    if (Preferences.get("services.sync.syncedTabs.showRemoteIcons", true)) {
> 
> I'm not convinced that we need this change here. Just because you don't want
> to show remote icons doesn't mean they're not a legitimate part of the tab
> record.
> 
> If not, we should check this once, not once per tab.

I'm not sure what you mean here - I added the check to SyncedTabs.jsm for exactly that reason - the tabengine concept of a tab *always* has the icon, but the UI parts are what determines whether to show it or not. SyncedTabs.jsm really is just a helper for the UI and it implements the icon policy, just as it already does when there's no icon at all (ie, it supplies the favicon).

Can you please elaborate on your concern?

Comment 12

[Richard Newman [:rnewman]]

(In reply to Mark Hammond [:markh] from comment #11)

Pah, SyncedTabs.jsm confuses me again.

Revised comment: don't check this pref N times.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

FTR:

Making the value "static" and thus forcing Firefox to be restarted before the pref change has any affect made testing painful, so what I did was:

* In SyncedTabs.jsm we read the preference value once each time the list of tabs is requested, not once per tab.
* In PlacesRemoteTabsAutoCompleteProvider.jsm I use a module-level global and added a preference observer.

The end result is that we've reduced the number of times the pref is read, but don't require a restart after toggling it.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dbb6c118f854
https://hg.mozilla.org/integration/fx-team/rev/8a4eed67be3b

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbb6c118f854
https://hg.mozilla.org/mozilla-central/rev/8a4eed67be3b