920573 - HTTP cache v2: Threat "Clear history when Firefox closes" + "Cache" checked setting

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Since no block files are used there can be a big number of files to delete during shutdown.

options:
- disable disk store, keep all just in memory (simple, safest, slows rich browsing experience but plays with the average browser session duration)
- evict more aggressively during the session so we have fewer files at the end (deps on 913808)
- optimize deletion of files at shutdown like delete using multiple threads, drop unwritten data quickly, use some direct OS apis (if avail) to delete the whole dir, etc.

Adding dep on 913822 as it is closely related.  We might need to abandon writes, but speedup close of all files to be able to delete them (on windows).

Comment 1

[Honza Bambas (:mayhemer)]

Doug, what is your opinion to keep (in cache v2) cached content only in memory when "Never remember history" is set?  At least as a quick solution, plan for using disk can be done in a followup bug.

Comment 2

[Honza Bambas (:mayhemer)]

For the record: we can rename (a.k.a. 'trash) the cache directory and spawn a background process (separate binary) that would delete the files.  To make it as fast as possible, we can use shell commands directly.  E.g. on windows use [1].  On linux use 'rm -rf' spawn.  This won't block Firefox shutdown and subsequent start.  We can also spawn the cache delete binary any time when we find any 'trashed' files.

This process may block computer shutdown.  On windows (since Vista ?) users are given a screen with a list of processes those block.  When we give our cache deleting process a good title, people may understand and wait or at least know the cache will not be deleted when they force shutdown.

[1] http://stackoverflow.com/questions/213392/what-is-the-win32-api-function-to-use-to-delete-a-folder

Comment 3

[Jason Duell]

I'd really like us to just remove the "forget all history" option.  It seems like a pre-Private Browsing dinosaur.  Who do we need to talk to to see if that's an option (we might need telemetry)?

IIRC the old cache is already using RAM only when this is set, right?  If so it's fine to do that in the new cache too.  

The subprocess idea is interesting and might be a good solution.

Comment 4

[Doug Turner (:dougt)]

regarding comment 1 - sounds like that is the right solution.  follow up for using the disk - but I am not sure if it a priority over things like racing file/network  io.

Jason - yeah, asked FF fe people for telementry and maybe ask the product owners.  No idea if it's used or not.

Comment 5

[Josh Matthews [:jdm]]

Anecdotally, we received surprisingly large numbers of bug reports and SUMO issues from people who use Never Remember History when we caused breakage while implementing per-window private browsing.

Comment 6

[Honza Bambas (:mayhemer)]

I know two people using it: me and one of my friends.  I have a separate profile I run with low privileges and use it to visit pages that could be potentially harmful.  My friend is just happier when his machine doesn't remember stuff by default - he is a type of guy using wipe utils to delete files.

So, from my point of view, this is used...

Comment 7

[Honza Bambas (:mayhemer)]

So this seems to be a little bit more complicated...

There are following 3 prefs:


"browser.privatebrowsing.autostart", means the browser is always in PB mode
  - true when Options/Privacy/History/Never remember history is set
  - true when Options/Privacy/History/Use custom settings is set AND Always start in PB mode is checked
  - otherwise is false (Always remember or Custom w/o PB mode always on)
  - when this one changes BROWSER MUST RESTART, which is a nice thing regarding this bug


"privacy.sanitize.sanitizeOnShutdown"
  - true when Options/Privacy/History/Use custom settings is set AND Clear history when Firefox closes is checked
  - false otherwise (when PB mode always on is set, this doesn't have any meaning, since PB mode doesn't store anything)


"privacy.clearOnShutdown.cache", so called 'micro privacy sanitization' pref
  - true (by default!) when Options/Privacy/History/Use custom settings is set AND Clear history when Firefox closes is checked AND Cache is checked in the dialog open with the [ Settings... ] button bellow.


The last two (mainly the privacy.sanitize.sanitizeOnShutdown) prefs may switch during browser run, so we should respect them while workaround to only store in memory is not applicable here.

Michal, can be the code in bug 968101 used to delete during shutdown?  Please add proper dependencies to this bug for what needs to be done to make this work and also make some estimation how complicated deleting during shutdown should be, so we can rather postpone this.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: backup (pref read)

Comment 9

[(no longer active)]

Note that I don't think private browsing should add any complications here.  The stuff viewed from a private window should never end up in the disk cache.

Comment 10

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Michal, can be the code in bug 968101 used to delete during shutdown? 
> Please add proper dependencies to this bug for what needs to be done to make
> this work and also make some estimation how complicated deleting during
> shutdown should be, so we can rather postpone this.

It should be quite simple, we just need to remove the whole cache and wait until the eviction finishes.

Comment 11

[Michal Novotny [:michal]]

Attachment: patch v1

This patch ensures that:

- index is deleted during shutdown
- entries dir, doomed dir and all trash dirs are deleted

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8400080 [details] [diff] [review]
patch v1

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

r=honzab, check the comments

locally tested on win.

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +3467,5 @@
> +
> +  if (!aDir) {
> +    file = aFile;
> +  } else {
> +    nsAutoCString dir(aDir);

nsDependentCString, declare before use or inline to the function call

@@ +3512,5 @@
> +  mFailedTrashDirs.Clear();
> +  mTrashDir = nullptr;
> +
> +  while (true) {
> +    rv = FindTrashDirToRemove();

comment: fills mTrashDir

@@ +3515,5 @@
> +  while (true) {
> +    rv = FindTrashDirToRemove();
> +    if (rv == NS_ERROR_NOT_AVAILABLE) {
> +      LOG(("CacheFileIOManager::SyncRemoveAllCacheFiles() - No trash directory "
> +             "found."));

ind

::: netwerk/cache2/CacheIndex.cpp
@@ +481,5 @@
>        MOZ_ASSERT(false, "Unexpected state!");
>    }
>  
> +  if (sanitize) {
> +    index->RemoveIndexFromDisk();

will this remove the index when it has not been initialized during the session?

::: netwerk/cache2/CacheObserver.cpp
@@ +179,5 @@
> +                                        "privacy.sanitize.sanitizeOnShutdown",
> +                                        kDefaultSanitizeOnShutdown);
> +  mozilla::Preferences::AddBoolVarCache(&sClearCacheOnShutdown,
> +                                        "privacy.clearOnShutdown.cache",
> +                                        kDefaultClearCacheOnShutdown);

keep the file indention/formating style please.

::: netwerk/cache2/CacheObserver.h
@@ +48,5 @@
>      { return sHalfLifeExperiment; }
> +  static bool const SanitizeOnShutdown()
> +    { return sSanitizeOnShutdown; }
> +  static bool const ClearCacheOnShutdown()
> +    { return sClearCacheOnShutdown; }

These two are always used together, maybe merge them to only ClearCacheOnShutdown() ?

Comment 13

[Michal Novotny [:michal]]

Attachment: patch v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/70f74c983cdf

Comment 14

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> ::: netwerk/cache2/CacheIndex.cpp
> @@ +481,5 @@
> >        MOZ_ASSERT(false, "Unexpected state!");
> >    }
> >  
> > +  if (sanitize) {
> > +    index->RemoveIndexFromDisk();
> 
> will this remove the index when it has not been initialized during the
> session?

We initialize the index always when we have the profile, so when the index is not initialized we don't have a profile, we don't know where the cache is and also we don't know the preferences, so we don't know about that we should delete the cache.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/70f74c983cdf