709262 - Disable disk cache for users who have "Clear private data at shutdown"

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jason Duell]

see conversation at 

  http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/1fd478855b2cf34f/e645dd54985d042b#

We could either limit the user's disk cache to a small, presumably-quick-to-delete size, or simply disable disk cache for users of this option (per bsmedberg's comment).  I think disabling the disk cache is an easier fix, and guarantees a fast shutdown, so that's my choice of solution.

Michal, can you take this?  It should be simple, and hopefully we can land for FF 11.

Comment 1

[Josh Matthews [:jdm]]

I'm testing a patch to do this.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Disable disk cache if the user chooses to sanitize private data on shutdown.

Local testing worked perfectly, based on the output of about:cache.

Comment 3

[Jason Duell]

Comment on attachment 580580 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.

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

Josh: thanks the for the patch!

So this disables the disk catch if sanitizing, which is great.  But now we've got a bunch of goop for 'finishDeleting' at shutdown that we no longer need (we added it in bug 695003).  I've filed bug 709964 for that.

Comment 4

[Jason Duell]

Comment on attachment 580580 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.

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

Oh, wait--dang!

This patch still disables the disk cache if "privacy.sanitize.sanitizeOnShutdown" set but the user *hasn't* selected the cache as one of things to clear.  In that case we should keep the disk cache on.  So you need to also check for "privacy.clearOnShutdown.cache"  (see the logic in nsCacheService::Shutdown)

Comment 5

[Jason Duell]

And I'd appreciate it if you could double-check that those two prefs work together exactly as I seem to think they do--I basically trusted that michal got it right when I +r'd 695003.

Comment 6

[Michal Novotny [:michal]]

Comment on attachment 580580 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.

> +        } else if (!strcmp(SANITIZE_ON_SHUTDOWN_PREF, data.get())) {
> +            rv = branch->GetBoolPref(SANITIZE_ON_SHUTDOWN_PREF,
> +                                    &mWillSanitizeOnShutdown);
> +            if (NS_FAILED(rv))
> +                return rv;
> +            nsCacheService::SetDiskCacheEnabled(DiskCacheEnabled());

As Jason said, you need to observe "privacy.clearOnShutdown.cache" as well. Also you need to read the pref values in nsCacheProfilePrefObserver::ReadPrefs().

Anyway, IMO this patch doesn't work as expected. Disabling the disk cache doesn't remove the cache from the disk, it just won't cache any other item from that moment on. But the description in pref dialog says "When I quit Firefox, it should automatically clear all: ...".

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Michal Novotny (:michal) from comment #6)
> Anyway, IMO this patch doesn't work as expected. Disabling the disk cache
> doesn't remove the cache from the disk, it just won't cache any other item
> from that moment on. But the description in pref dialog says "When I quit
> Firefox, it should automatically clear all: ...".

I agree. It seems like we still need the deletion logic to handle the session where the user first set the option, and/or to recover from a previous faliure to completely delete the disk cache. :( So, WONTFIX bug 709964?

Comment 8

[Jason Duell]

OK, good catch--we'll keep the nsDeleteDir finishDelete logic in place.  But it will actually delete stuff only once (the first time the browser is closed after the user has set clear private data).

As far as this patch goes, it looks like we just need to add the additional check for privacy.clearOnShutdown.cache and we're ready to land, right?

Comment 9

[Josh Matthews [:jdm]]

Attachment: Disable disk cache if the user chooses to sanitize private data on shutdown.

This patch required a bit more effort than originally believed, since it turned out that disabling the disk cache also disabled its sanitization. I've checked every use of mEnableDiskCacheDevice in nsCacheService, and I'm reasonably confident that I have changed the only uses that require knowledge of whether the disk cache is _really truly_ disabled or not (ie. sanitization). Unfortunately, since the browser-implemented sanitization just uses the public evictEntries API, we have to keep track of whether we're in the shutdown state and whether we're expecting sanitization to occur :(

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 582685 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.

I'll throw Michal on this review as well, since he probably knows this code better.

Comment 11

[Mozilla RelEng Bot]

Try run for b8c0aaf1de71 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b8c0aaf1de71
Results (out of 55 total builds):
    success: 47
    warnings: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b8c0aaf1de71

Comment 12

[Michal Novotny [:michal]]

Comment on attachment 582685 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.

I personally don't like the changes in nsCacheService::EvictEntriesForClient() and nsCacheService::OnProfileShutdown(). What about to simply delete a cache directory during shutdown if it exists?

In nsCacheService::Shutdown(), get the cache parent directory from mObserver before we release it. Then change the finishDeleting block to something like this:

if (finishDeleting) {
  rv = parentDir->AppendNative(NS_LITERAL_CSTRING("Cache"));
  if (NS_SUCCEEDED(rv)) {
    bool exists;
    if (NS_SUCCEEDED(parentDir->Exists(&exists)) && exists)
      nsDeleteDir::DeleteDir(parentDir, false);
  }
  Telemetry::AutoTimer<Telemetry::NETWORK_DISK_CACHE_SHUTDOWN_CLEAR_PRIVATE> timer;
  nsDeleteDir::Shutdown(finishDeleting);
} else {


>      bool finishDeleting = false;
>      nsresult rv;
>      nsCOMPtr<nsIPrefBranch2> branch = do_GetService(NS_PREFSERVICE_CONTRACTID);
>      if (!branch) {
>          NS_WARNING("Failed to get pref service!");
>      } else {
>          bool isSet;
> -        rv = branch->GetBoolPref("privacy.sanitize.sanitizeOnShutdown", &isSet);
> +        rv = branch->GetBoolPref(SANITIZE_ON_SHUTDOWN_PREF, &isSet);
>          if (NS_SUCCEEDED(rv) && isSet) {
> -            rv = branch->GetBoolPref("privacy.clearOnShutdown.cache", &isSet);
> +            rv = branch->GetBoolPref(CLEAR_ON_SHUTDOWN_PREF, &isSet);
>              if (NS_SUCCEEDED(rv) && isSet) {
>                  finishDeleting = true;
>              }
>          }
>      }

Instead of finishDeleting you can now use (mWillSanitizeOnShutdown && mClearCacheOnShutdown). And please rename mWillSanitizeOnShutdown to mSanitizeOnShutdown.

Comment 13

[Jason Duell]

Comment on attachment 582685 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.

Michal does in fact know this code better than I, so I'll defer to him.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Disable disk cache if the user chooses to sanitize private data on shutdown.

You're right, this is a much better fix.

Comment 15

[Josh Matthews [:jdm]]

Attachment: Disable disk cache if the user chooses to sanitize private data on shutdown.

Comment 16

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b121a045b451

Comment 17

[Jason Duell]

Thanks for finding the time to work on this, jdm.  Always nice to see you in necko-land. Come visit more often! :)

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b121a045b451