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

RESOLVED FIXED in mozilla31

Status

()

Core
Networking: Cache
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mayhemer, Assigned: michal)

Tracking

unspecified
mozilla31
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cache2])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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).
(Reporter)

Comment 1

4 years ago
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.
Flags: needinfo?(doug.turner)
(Reporter)

Comment 2

4 years ago
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
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

4 years ago
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.
Flags: needinfo?(doug.turner)

Comment 5

4 years ago
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.
(Reporter)

Comment 6

4 years ago
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...
(Reporter)

Updated

4 years ago
Depends on: 913820
(Reporter)

Updated

4 years ago
No longer depends on: 913822
(Reporter)

Comment 7

4 years ago
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.
Flags: needinfo?(michal.novotny)
Summary: HTTP cache v2: Threat "Never remember history" setting → HTTP cache v2: Threat "Clear history when Firefox closes" + "Cache" checked setting
(Reporter)

Comment 8

4 years ago
Created attachment 8375707 [details] [diff] [review]
backup (pref read)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
(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.
Depends on: 968106
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 11

4 years ago
Created attachment 8400080 [details] [diff] [review]
patch v1

This patch ensures that:

- index is deleted during shutdown
- entries dir, doomed dir and all trash dirs are deleted
Assignee: nobody → michal.novotny
Attachment #8400080 - Flags: review?(honzab.moz)
(Reporter)

Comment 12

4 years ago
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() ?
Attachment #8400080 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 13

4 years ago
Created attachment 8403830 [details] [diff] [review]
patch v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/70f74c983cdf
Attachment #8375707 - Attachment is obsolete: true
Attachment #8400080 - Attachment is obsolete: true
Attachment #8403830 - Flags: review+
(Assignee)

Comment 14

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/70f74c983cdf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

Updated

4 years ago
Depends on: 1002329
Depends on: 1091330
You need to log in before you can comment on or make changes to this bug.