As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 709262 - Disable disk cache for users who have "Clear private data at shutdown"
: Disable disk cache for users who have "Clear private data at shutdown"
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Josh Matthews [:jdm]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 709964
  Show dependency treegraph
 
Reported: 2011-12-09 13:03 PST by Jason Duell [:jduell] (needinfo me)
Modified: 2014-06-27 14:42 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Disable disk cache if the user chooses to sanitize private data on shutdown. (5.69 KB, patch)
2011-12-09 16:11 PST, Josh Matthews [:jdm]
jduell.mcbugs: review-
Details | Diff | Splinter Review
Disable disk cache if the user chooses to sanitize private data on shutdown. (10.29 KB, patch)
2011-12-18 10:47 PST, Josh Matthews [:jdm]
michal.novotny: review-
Details | Diff | Splinter Review
Disable disk cache if the user chooses to sanitize private data on shutdown. (9.98 KB, patch)
2011-12-18 19:23 PST, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Disable disk cache if the user chooses to sanitize private data on shutdown. (9.90 KB, patch)
2011-12-18 19:26 PST, Josh Matthews [:jdm]
michal.novotny: review+
Details | Diff | Splinter Review

Description User image Jason Duell [:jduell] (needinfo me) 2011-12-09 13:03:58 PST
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 User image Josh Matthews [:jdm] 2011-12-09 15:45:19 PST
I'm testing a patch to do this.
Comment 2 User image Josh Matthews [:jdm] 2011-12-09 16:11:56 PST
Created attachment 580580 [details] [diff] [review]
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 User image Jason Duell [:jduell] (needinfo me) 2011-12-12 14:25:31 PST
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 User image Jason Duell [:jduell] (needinfo me) 2011-12-12 14:29:10 PST
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 User image Jason Duell [:jduell] (needinfo me) 2011-12-12 14:32:28 PST
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 User image Michal Novotny (:michal) 2011-12-12 16:57:40 PST
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 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-12 17:06:24 PST
(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 User image Jason Duell [:jduell] (needinfo me) 2011-12-15 14:52:41 PST
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 User image Josh Matthews [:jdm] 2011-12-18 10:47:53 PST
Created attachment 582685 [details] [diff] [review]
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 User image Josh Matthews [:jdm] 2011-12-18 10:57:13 PST
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 User image Mozilla RelEng Bot 2011-12-18 13:50:33 PST
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/josh@joshmatthews.net-b8c0aaf1de71
Comment 12 User image Michal Novotny (:michal) 2011-12-18 15:28:05 PST
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 User image Jason Duell [:jduell] (needinfo me) 2011-12-18 16:27:13 PST
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 User image Josh Matthews [:jdm] 2011-12-18 19:23:35 PST
Created attachment 582734 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.

You're right, this is a much better fix.
Comment 15 User image Josh Matthews [:jdm] 2011-12-18 19:26:50 PST
Created attachment 582735 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.
Comment 17 User image Jason Duell [:jduell] (needinfo me) 2011-12-19 10:31:51 PST
Thanks for finding the time to work on this, jdm.  Always nice to see you in necko-land. Come visit more often! :)
Comment 18 User image Ed Morley [:emorley] 2011-12-20 06:09:19 PST
https://hg.mozilla.org/mozilla-central/rev/b121a045b451

Note You need to log in before you can comment on or make changes to this bug.