Konark Modi - @konarkmodi Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

1. Open Firefox for Android
2. Open new tab
3. Visit sites:
    a. https://xvideos.com
    b. http://spiegel.de
4. Close the browser
5. Restart the browser


Actual results:

Domains with touch-icon in HTML seem to leave an entry in `cache/icons` folder.

Furthermore, it also reveals the number of page loads, because new unique page seems to save a new entry for the touch-icon.

As in the screenshot you can see, I visited three different pages on spiegel.de, and it saved touch icon three times.

Screenshots attached.


Expected results:

Expectation is: Private browsing does not leave any footprint on the device about domains visited.

But in this case, domains can be known. Also, I did not check, but the cache file name looks like a hash, did not confirm if it's hash based on a URL or not.

	Konark Modi - @konarkmodi Reporter
	Comment 1 • 6 years ago

Correction steps to reproduce:

Steps to reproduce:

1. Open Firefox for Android
2. Open new PRIVATE tab
3. Visit sites:
    a. https://xvideos.com
    b. http://spiegel.de
4. Close the browser
5. Restart the browser

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 4 • 6 years ago

[triage] Critical: PB leaks user data.

	:sdaswani
	Comment 5 • 6 years ago

David, I'm not sure if this is a platform or Front End issue - NI'ing you in case you know or can check with the team, NI me back if it's the latter.

	:sdaswani
	Comment 6 • 6 years ago

Also NI'ing Andreas for prioritization.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 7 • 6 years ago

Thanks Susheel over to James for deeper triage.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 8 • 6 years ago

Favicon code is firmly front-end stuff on Fennec, back to Susheel!

	:sdaswani
	Comment 9 • 6 years ago

Andreas, Barbara - should we fix this? If so, when?

	Andreas Bovens [:abovens]
	Comment 10 • 6 years ago

I think this is important to fix for an upcoming release.

	:sdaswani
	Comment 11 • 6 years ago

Tagging for 61.

	Kevin Brosnan [Ex-Mozilla]
	Comment 12 • 6 years ago

Jan might be interested in this.

	Jan Henning [:JanH]
	Comment 13 • 6 years ago

Some notes of what would have to be done after briefly looking at how icons seem to work:
- IconRequests already have an option to completely skip the disk cache [1], although maybe using that might be a little too blunt an instrument? If we want to still allow *reading* from the cache and only block writes of fresh content while in private mode, we'd have to add a new attribute along the same lines and handle it in the IconRequestBuilder and DiskProcessor.
- Review our usage of "Icons.with(...)" [2] and skip the disk cache (at least for writes) where necessary.
At a first glance, the mainly relevant pieces are the Tab (self-evident), TwoLinePageRow (used for display of recently closed tabs, *including in private mode*), TabHistoryItemRow (session history display when long-pressing back/forward button) and TabsLayoutItemView (tabs tray screen) classes.
- Anything that currently uses both skipNetwork() and skipMemory() and therefore relies only on the disk cache (plus generated fallback icons) needs to be reviewed. One example is HomeScreenPrompt and GeckoApplication.createBrowserShortcut. While I think that the former currently isn't used in private browsing, the latter is used for "Add Page Shortcut" in the "Page" menu. If the disk cache is no longer updated in private browsing and usage of the memory cache is prohibited, adding a launcher shortcut while in private browsing may not work properly. All other call sites from [2] need to be checked for this usage pattern, too.
- Looking briefly, anything else is probably okay as it's either helper functions for reader mode (which only resolves the URL, but doesn't store anything [3]), search engine icons, or used (in the home panels or similar displays) for URLs that have already entered history, have been bookmarked or have saved logins, in which case private browsing won't really matter.
- This bit [4] needs moving into the sanitizer, so it is executed for users that are clearing private data on exit as well. This can be done similar to our existing flow for clearing history [5], i.e. add a Sanitize:Icons request to the offlineApps handler in Sanitizer.jsm and then clear icons in Java only in response to that message instead of directly hooking Settings -> Clear private data.

[1] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java#131-136
[2] https://dxr.mozilla.org/mozilla-central/search?q=Icons.with+-path%3Atest&redirect=false
[3] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java#88
[4] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/preferences/PrivateDataPreference.java#55
[5] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/modules/Sanitizer.jsm#169

	Jan Henning [:JanH]
	Comment 14 • 6 years ago

Sebastian, can you remember why we're also skipping the memory cache when retrieving an icon for adding a home screen shortcut (https://hg.mozilla.org/mozilla-central/annotate/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/promotion/HomeScreenPrompt.java#l138)?

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 15 • 6 years ago

I think the reason for that was that the in-memory icon is resized to the ideal size for displaying in the UI and for the launcher icon we may want to have a larger version from disk.

	Rob Wu [:robwu] Assignee
	Comment 17 • 6 years ago

(In reply to Jan Henning [:JanH] from comment #13)
> Some notes of what would have to be done after briefly looking at how icons
> seem to work:
> - IconRequests already have an option to completely skip the disk cache [1],
> although maybe using that might be a little too blunt an instrument? If we
> want to still allow *reading* from the cache and only block writes of fresh
> content while in private mode, we'd have to add a new attribute along the
> same lines and handle it in the IconRequestBuilder and DiskProcessor.
> - Review our usage of "Icons.with(...)" [2] and skip the disk cache (at
> least for writes) where necessary.
> At a first glance, the mainly relevant pieces are the Tab (self-evident),
> TwoLinePageRow (used for display of recently closed tabs, *including in
> private mode*), TabHistoryItemRow (session history display when
> long-pressing back/forward button) and TabsLayoutItemView (tabs tray screen)
> classes.

I audited the uses of Icons.with in bug 1468116 and reached the same conclusions.
I also noted an icon persistence of AS in PBM: "activitystream (StreamOverridablePageIconLayout.java and TopSitesCard.java ) : It is based on the browsing history, so I am inclined to not change anything for now and keep caching their icons."

> - Anything that currently uses both skipNetwork() and skipMemory() and
> therefore relies only on the disk cache (plus generated fallback icons)
> needs to be reviewed. One example is HomeScreenPrompt and
> GeckoApplication.createBrowserShortcut. While I think that the former
> currently isn't used in private browsing, the latter is used for "Add Page
> Shortcut" in the "Page" menu. If the disk cache is no longer updated in
> private browsing and usage of the memory cache is prohibited, adding a
> launcher shortcut while in private browsing may not work properly. All other
> call sites from [2] need to be checked for this usage pattern, too.

I guess that the network is skipped for shortcuts because of unpredictable latency.
I think that a fall back to the in-memory cache is better than not having anything at all.


> - This bit [4] needs moving into the sanitizer, so it is executed for users
> that are clearing private data on exit as well. This can be done similar to
> our existing flow for clearing history [5], i.e. add a Sanitize:Icons
> request to the offlineApps handler in Sanitizer.jsm and then clear icons in
> Java only in response to that message instead of directly hooking Settings
> -> Clear private data.

Do we need to add any migration logic to sanitize the existing cache? E.g. remove all icons that do not appear in the history or bookmarks.

	Jan Henning [:JanH]
	Comment 18 • 6 years ago

(In reply to Rob Wu [:robwu] from comment #17)
> I guess that the network is skipped for shortcuts because of unpredictable
> latency.
Yes, probably. I also see that the network is skipped as well for tabs, about:home, etc. as well, so it might also have been part of the design to only load favicons from the network when actually loading a page, though.

> I think that a fall back to the in-memory cache is better than not having
> anything at all.

I'd agree, plus potentially the new read-only disk cache option if you choose to implement it that way.

> Do we need to add any migration logic to sanitize the existing cache? E.g.
> remove all icons that do not appear in the history or bookmarks.

Hmm, icons are (mostly?) only fetched from the network when actually loading a page (see above), so dropping the whole icon cache would be the easiest route, but also rather annoying to our users.
So yes, if we don't want to rely on natural wastage then that leaves only an explicit sanitization of all unused favicons.

	Rob Wu [:robwu] Assignee
	Comment 19 • 6 years ago

(In reply to Jan Henning (on vacation in June) [:JanH] from comment #13)
> - This bit [4] needs moving into the sanitizer, so it is executed for users
> that are clearing private data on exit as well. This can be done similar to
> our existing flow for clearing history [5], i.e. add a Sanitize:Icons
> request to the offlineApps handler in Sanitizer.jsm and then clear icons in
> Java only in response to that message instead of directly hooking Settings
> -> Clear private data.
> 
> [4] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/preferences/PrivateDataPreference.java#55
> [5] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/modules/Sanitizer.jsm#169

I have opened bug 1470174 for this (and not fixing it now because the code is being refactored by bug 1466130).


I will also defer the one-time clean-up logic (i.e. removing icons that potentially originate from private browsing mode), because removing old favicons while preserving favicons from history / bookmarks is complicated, because DiskLruCache.java does not support a way to enumerate keys, and the keys are hashes of the page and favicon URLs. Therefore, to remove unknown icons, something like this would be needed:

1. First we need to determine all bookmarked and history URLs.
2. Then we calculate the key: SHA256("mapping:" + pageUrl)
3. Then we find the iconUrl in the DiskLruCache.
4. If iconUrl exists, calculate the key: SHA256("icon:" + iconUrl)
5. All icons except for those referenced by the above keys should be removed.

There are simpler alternatives such as reducing DISK_CACHE_SIZE to shrink the cache size (it is currently set to 50MB, but in practice my cache is twice as much...):
https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java#36-40,70-74

This alternative won't work if the user rarely uses normal browsing mode.
Yet another approach (other than completely nuking the icon cache) is to store icons at a new location (e.g. cache/icons2/) and use the original cache/icons/ in a read-only fashion. Then the directory can be removed after a few releases.


With these parts moved out of the scope of this bug, my patches that fix the actual leak are ready for review.

	Rob Wu [:robwu] Assignee
	Comment 20 • 6 years ago

Test case:
1. Load a new URL in a private tab.
2. Check that the page's icon is not in cache/icons/

	Rob Wu [:robwu] Assignee
	Comment 21 • 6 years ago

Test case:
1. Press triple-dots
2. Long-press the back button (left arrow).
3. Check that there the icon is not saved to the disk.

	Rob Wu [:robwu] Assignee
	Comment 22 • 6 years ago

Test case:
1. Open a private tab (thanks to a previous patch the icon won't be
   saved to cache/icons/ ).
2. Tab on the "[1]" to show all private tabs.
3. Check that the icon is not saved to cache/icons/ .

	Rob Wu [:robwu] Assignee
	Comment 23 • 6 years ago

Test case:
1. Open a URL in normal browsing mode (so it appears in the history).
2. Open a private tab, and type the first three letters of the URL's
   domain. Now Fennec will show a suggestion bar with the item from
   the first step.
3. Check that the icon from step 2 is not stored in cache/icons/.

	Rob Wu [:robwu] Assignee
	Comment 24 • 6 years ago

Try to fetch an icon from the memory cache before definitely falling
back to an auto-generated icon when a page shortcut is created.

Test case:
1. Visit a site with a favicon in a private tab.
   (Thanks to a previous patch, the favicon is only stored in memory).
2. Use Triple dots menu > Page > Add Page Shortcut.
3. Confirm that the created shortcut has the same icon as the page.
   (Note: the icon will also be saved to the disk cache.
    I did not change this behavior since the shortcut is already
    stored on the disk anyway).

	Jan Henning [:JanH]
	Comment 25 • 6 years ago

(In reply to Rob Wu [:robwu] from comment #19) 
> I have opened bug 1470174 for this (and not fixing it now because the code
> is being refactored by bug 1466130).

Could you just CC me there as well?

	Rob Wu [:robwu] Assignee
	Comment 26 • 6 years ago

@Jan
Done. Could you review my patches? Since you've looked into it before, reviewing should be easier compared to selecting another random reviewer (if not, please recommend another fitting reviewer).

I have manually verified that Firefox behaves as expected.
Locally, I even added logging to DiskProcessor.java and IconTask.java to confirm that the flags are propagated as expected.

	Jan Henning [:JanH]
	Comment 27 • 6 years ago

Thanks. I'm happy to look at your patches, but I probably won't be able to look at them properly until I return back home the weekend after this one. If you don't want to wait, either ask Sebastian (who originally wrote the icon code), or if he doesn't have time, either, maybe jchen?

	Rob Wu [:robwu] Assignee
	Comment 28 • 6 years ago

Could you review my patches now?

(I don't know how to add r? without triggering an e-mail notification for each, so I'm using ni? instead)

	Jan Henning [:JanH]
	Comment 29 • 6 years ago

Yes, I'll look at them this weekend.

	Jan Henning [:JanH]
	Comment 30 • 6 years ago

Comment on attachment 8986814 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of private tabs

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

Two small suggestions for the wording of the Javadoc, but otherwise fine. Thanks for taking this bug.

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
@@ +98,5 @@
>          return privileged;
>      }
>  
> +    /**
> +     * Is this request initiated from a tab in private browsing mode?

"Has this request been initiated..." sounds a little more natural to me.

::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
@@ +61,5 @@
>          return this;
>      }
>  
> +    /**
> +     * Set the private mode to avoid saving the result to the disk.

Maybe spell it out and say "Set the private mode to true to avoid..."?

	Jan Henning [:JanH]
	Comment 31 • 6 years ago

Comment on attachment 8986819 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of private autocompletion results

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

Unfortunately this doesn't cover "Recently closed" tabs, where TwoLinePageRow isn't used with the private mode theming and therefore mUrl.isPrivateMode() returns false.
This means that with
1. Open > 1 private mode tab.
2. Close some of them again, but leave at least one private mode tab open.
3. Open the Awesomescreen and look at History > Recently closed
the icons of the tabs appearing under "Recently closed" would still end up in the disk cache, wouldn't they?

	Rob Wu [:robwu] Assignee
	Comment 32 • 6 years ago

Test case in comment 31,
followed by checking that the closed tab's icon is not in cache/icons/.

	Rob Wu [:robwu] Assignee
	Comment 33 • 6 years ago

Comment on attachment 8991874 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of recently closed private tabs

This fixes the issue from comment 31.

	Jan Henning [:JanH]
	Comment 34 • 6 years ago

Comment on attachment 8991874 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of recently closed private tabs

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

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryItem.java
@@ +88,5 @@
>  
>          public void bind(ClosedTab closedTab) {
>              final TwoLinePageRow childPageRow = (TwoLinePageRow) this.itemView;
>              childPageRow.setShowIcons(false);
> +            childPageRow.setPrivateMode(closedTab.isPrivate);

Unfortunately this also means that you're enabling the private mode theming, which
a) poses the question whether we actually want to do that for this case, and
b) doesn't really work, as the background isn't controlled by the TwoLinePageRow, so you end up with white-on-white text for the title.

I think you're better off just introducing a dedicated setting for controlling the icon-loading behaviour of a TwoLinePageRow.

	Rob Wu [:robwu] Assignee
	Comment 36 • 6 years ago

Comment on attachment 8992214 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of TwoLinePageRow in private tabs

I haven't introduced a new setting; I just checked whether the selected tab is private.

Tested using the steps from comment 23 and comment 31.

	Jan Henning [:JanH]
	Comment 37 • 6 years ago

Comment on attachment 8992214 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of TwoLinePageRow in private tabs

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

Other things are relying on the currently selected tab as well to make that distinction, so yes, doing it that way should be fine here, too.

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java
@@ +308,5 @@
> +    /**
> +     * @return true if this view is shown inside a private tab, independent of whether
> +     * a private mode theme is applied via <code>setPrivateMode(true)</code>.
> +     */
> +    private boolean isTabInPrivateMode() {

The Javadoc is appreciated, but maybe name the function itself something like isCurrentTabInPrivateMode() to remove some ambiguity?

	Rob Wu [:robwu] Assignee
	Comment 38 • 6 years ago

Comment on attachment 8992214 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of TwoLinePageRow in private tabs

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

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java
@@ +308,5 @@
> +    /**
> +     * @return true if this view is shown inside a private tab, independent of whether
> +     * a private mode theme is applied via <code>setPrivateMode(true)</code>.
> +     */
> +    private boolean isTabInPrivateMode() {

It did initially not occur to me that `setPrivateMode` was only for theming, and that it could have been different from the actual PBM state. I tried to capture this in a comment for future readers.
A different method name would indeed say what the method does, but not why, so I intend to keep the comment and name as is.

	Rob Wu [:robwu] Assignee
	Comment 39 • 6 years ago

(In reply to Jan Henning [:JanH] from comment #37)
> The Javadoc is appreciated, but maybe name the function itself something
> like isCurrentTabInPrivateMode() to remove some ambiguity?

I misread your comment; you were only asking to make the name more verbose, without necessarily removing the comment.
I'll make that change and then try to figure out how to land these patches.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 40 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/43a2010067721dc68eef1e71a7d1d6fd9fafcc45
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9034458471c8478c767d7a27ee3cc3b4f76cebe
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b6236aaef8c848ce9e2d3f98ea7a22c1a1c26e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee63d789769e77377e6246817f900727b23f1289
https://hg.mozilla.org/integration/mozilla-inbound/rev/37bd009f11f4e05b02a6219a01616331a315efd0

https://hg.mozilla.org/mozilla-central/rev/43a201006772
https://hg.mozilla.org/mozilla-central/rev/c9034458471c
https://hg.mozilla.org/mozilla-central/rev/c5b6236aaef8
https://hg.mozilla.org/mozilla-central/rev/ee63d789769e
https://hg.mozilla.org/mozilla-central/rev/37bd009f11f4

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 6 years ago

I'm assuming this fix can just ride the trains, but feel free to nominate for Beta approval if you feel strongly otherwise.

	Sorina Florean [:sflorean]
	Comment 42 • 6 years ago

Tested with Nexus 5(Android 6.0.1) and OnePlus 5T (Android 8.1.0) following the steps provided and we couldn't reproduce the issue on the latest Nightly build (63.0a1 - 21/08).