1721904 - Page thumbnails from private mode tabs are kept in the cache after a crash

Status: RESOLVED FIXED

(Fenix :: General, defect, P2)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

We received a bug report from someone who mentioned that they mostly work in private mode, but are seeing the storage used by Firefox grow.

I think this needs a bigger investigation, for which I filed bug 1721905

In my own limited exploration I have found one case where sensitive cached data from private mode is left behind when the application is killed.

Steps to reproduce:

1. Open some tabs in private mode
2. Kill Firefox (I used kill -9 from a root shell - swiping the app away may not work)
3. Start Firefox again

Expected:

No private tabs are open and cache directories are empty.

Actual:

No private tabs are open, but I did see the following data on disk:

taimen:/data/data/org.mozilla.firefox/cache # find .
.
./gecko_temp
./mozac_browser_thumbnails
./mozac_browser_thumbnails/thumbnails
./mozac_browser_thumbnails/thumbnails/789b065e-fb9b-4373-b47a-ee39dc95b12c.0
./mozac_browser_thumbnails/thumbnails/e6a057a2-b438-4a08-a7b8-5cfa501b5e08.0
./mozac_browser_thumbnails/thumbnails/5171ea7f-9278-42c4-8eae-15073f66ca08.0
./jna-3506402
./kfz44b8d.default
./kfz44b8d.default/cache2
./kfz44b8d.default/cache2/doomed
./kfz44b8d.default/cache2/entries
./kfz44b8d.default/cache2/entries/342AC7BEC15653F647CF0E081A0209627846056F

The cache2/entries is a Nimbus JSON file. I assume that is fine. But the three files in mozac_browser_thumbnails are full screen screenshots of the tabs I had open in private mode.

Restarting Firefox does not make these go away. They stick around.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Christian mentioned that Jon and he worked on addressing this. Jon do you recall if we landed a fix here?

Comment 2

[Jonathan Almeida [:jonalmeida]]

We had some ideas on it, but we didn't explicitly land any patches for this, to the best of my knowledge.

Comment 3

[Chris Peterson [:cpeterson]]

sec-moderate -> P2

Comment 4

[Petru-Mugurel Lingurar [:petru]]

@Jon Curious about the ideas mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1721904#c2.
If we'd try to specifically tackle the mozac_browser_thumbnails/thumbnails issue I think we should differentiate between thumbnails and such for private vs normal browsing.
My idea for this is that we could mark each file with it's browsing mode indicator or just when private but using separate directories seems cleaner and would support other related functionalities also.
Then we could have a foreground service that can be started:

• when we detect a crash
• when when the task is removed

to cleanup the private browsing mode directory.

Comment 5

[Jonathan Almeida [:jonalmeida]]

The ideas were similar. Either pre/post-fix the file names with the browsing mode and remove those or use a separate folder. I'm also inclined to using a separate folder.

For the clean-up strategy, during initialization of the ThumbnailsStorage, we could do a full directory delete or file name search before we start persisting new files. That avoids the problem of the app force quitting and leaving thumbnails around.

The files can also be created as a TempFile and then removed with deleteOnExit. We could use this solution with either direction, filename fixtures or separate folder.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Investigated the possible approaches and while using deleteOnExit seemed better to usd we'd had to do deep modifications in how we currently cache the thumbnails - through the help of the DiskLruCache external library.

So I went into separating the storages for thumbnails from normal tabs vs private tabs but I want to confirm the general direction before finishing the patches so I'll add them here.
Note that I focused on ensuring all operations from ThumbnailsMiddleware work as before with as little changes as possible.
In my tests with using adb shell am force-stop org.mozilla.fenix.debug I see that these patches do help fix the issue by ensuring no leftover files for after the app is reopened.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_1_-_Separate_cache_dirs.patch

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_2_-_Ensure_removing_all_tabs_will_clear_thumbnails.patch

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_3_-_Ensure_removing_individual_tabs_will_clear_thumbnails.patch

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_4_-_Ensure_removing_part_of_opened_tabs_will_clear_thumbnails.patch

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_5_-_Ensure_updating_a_thumbnail_will_differentiate_between_normal_and_private_ones.patch

Comment 12

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_6_-_Ensure_removing_a_single_thumbnail_differentiates_between_normal_and_private_one.patch

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_7_-_Ensure_thumbnails_can_be_loaded_into_Views.patch

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_3_-_Ensure_removing_individual_tabs_will_clear_thumbnails.patch

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Attachment: Patch_4_-_Ensure_removing_part_of_opened_tabs_will_clear_thumbnails.patch

Comment 16

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Separate_cache_directories_for_thumbnails.patch

While individual patches may allow to more easily follow the changes needed for each thumbnails scenario this one patch for all changes will help in easily applying the changes.

Comment 17

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 9322083 [details] [diff] [review]
Bug_1721904_-_Separate_cache_directories_for_thumbnails.patch

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

Thanks for the patch! Overall this looks good and the right direction.

::: android-components/components/browser/thumbnails/src/main/java/mozilla/components/browser/thumbnails/ThumbnailsMiddleware.kt
@@ +62,5 @@
>          }
>          next(action)
>      }
> +
> +    private fun deleteTabThumbnail(tabId: String, isPrivate: Boolean) {

Cosmetic: Not necessary, but since `saveThumbnails` is a single API with a private boolean flag, you could consider lifting this change there too for symmetry.

::: android-components/components/browser/thumbnails/src/main/java/mozilla/components/browser/thumbnails/utils/PrivateThumbnailsDiskCache.kt
@@ +8,5 @@
> +import java.io.File
> +
> +class PrivateThumbnailsDiskCache(private val appContext: Context): ThumbnailDiskCache() {
> +    init {
> +        clear()

Nice.

Comment 18

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Separate_cache_dirs_all_patches_in_one.patch

All proposed patches squashed in just one that should help in easily applying the changes.

Comment 19

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Part1_Separate_cache_dirs.patch

Comment 20

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Part2_Ensure_removing_all_tabs_will_clear_thumbnails.patch

Comment 21

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Part3_Ensure_removing_individual_tabs_will_clear_thumbnails.patch

Comment 22

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Part4_Ensure_removing_part_of_opened_tabs_will_clear_thumbnails.patch

Comment 23

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Part5_Ensure_updating_a_thumbnail_differentiates_between_normal_and_private_thumbn.patch

Comment 24

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Part6_Ensure_removing_a_single_thumbnail_differentiates_between_normal_and_private.patch

Comment 25

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug_1721904_-_Part7_Ensure_cached_thumbnails_can_be_loaded_into_Views.patch

Comment 26

[Petru-Mugurel Lingurar [:petru]]

Updated the proposed patches with documentation and tests.
Ran detekt, ktlint and all unit tests in A-C and Fenix successfully.
In manual testing all seemed okay - same previous behavior but if using adb shell am force-stop org.mozilla.fenix.debug the private thumbnails are leaked until the next time the app starts.

Tried using deleteOnExit also with

override fun getCacheDirectory(): File {
   return File(getCacheParentDirectory(appContext), PRIVATE_CACHE_DIRECTORY_NAME).also {
       it.deleteOnExit()
   }

in the new PrivateThumbnailsDiskCache but that didn't seem to work when killing the app with adb shell am force-stop org.mozilla.fenix.debug.

Seems like we have a similar issue with browser icons from private tabs which are not automatically deleted when those tabs are closed.
Seeing that the thumbnails were automatically deleted when the private tabs are closed - so we already have a different behavior in between icons and thumbnails I am not sure if we should look into have similar patches for icons also.

Comment 27

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 9325757 [details] [diff] [review]
Bug_1721904_-_Separate_cache_dirs_all_patches_in_one.patch

Adding myself for f? so this bug doesn't get forgotten.

Comment 28

[Matt Tighe [:matt-tighe]]

Attachment: Bug_1721904_introduce_private_thumbnail_cache.patch

Comment 29

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 9344462 [details] [diff] [review]
Bug_1721904_introduce_private_thumbnail_cache.patch

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

LGTM, sorry for the long wait.

::: fenix/app/src/main/java/org/mozilla/fenix/perf/StrictModeManager.kt
@@ +54,4 @@
>      @VisibleForTesting(otherwise = PRIVATE)
>      val suppressionCount = AtomicLong(0)
>  
> +    private val isEnabledByBuildConfig = config.channel.isDebug

Assuming this change was added just during testing and will not be committed.

Comment 30

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 9325757 [details] [diff] [review]
Bug_1721904_-_Separate_cache_dirs_all_patches_in_one.patch

Removing r? from obsolete patch.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 9344462 [details] [diff] [review]
Bug_1721904_introduce_private_thumbnail_cache.patch

You don't need to ask for sec-approval for a sec-moderate or sec-low bug; see https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html (linked from the yellow-orange security bug banner above)

If there's a part of the patch that worries you about being revealing you could ask out of band about it

Comment 32

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1721904 - update thumbnail caching on app open (#3518)

Comment 33

[BMO Github Automation]

Authored by https://github.com/MatthewTighe
https://github.com/mozilla-mobile/firefox-android/commit/dee12005df0740b9260cc2c6150b5e20ff1c2f3d
[main] Bug 1721904 - update thumbnail caching on app open

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:matt-tighe, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Comment 35

[Giorgio Maone [:ma1]]

Putting Dan Ballard in CC because we couldn't backport this right now to our 115esr-based Tor Browser for Android but we want to do it in 13.0.2 (late November).

Can we please keep this confidential until then? Thank you!

Comment 36

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter