Closed Bug 709262 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Networking: Cache, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jduell.mcbugs, Assigned: jdm)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
I'm testing a patch to do this.
Assignee: nobody → josh
Local testing worked perfectly, based on the output of about:cache.
Attachment #580580 - Flags: review?(jduell.mcbugs)
Blocks: 709964
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.
Attachment #580580 - Flags: review?(jduell.mcbugs) → review+
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)
Attachment #580580 - Flags: review+ → review-
Summary: Limit or disable disk cache for users who have "Clear private data at shutdown" → Disable disk cache for users who have "Clear private data at shutdown"
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 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: ...".
(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?
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?
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 :(
Attachment #582685 - Flags: review?(jduell.mcbugs)
Attachment #580580 - Attachment is obsolete: true
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.
Attachment #582685 - Flags: review?(michal.novotny)
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 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.
Attachment #582685 - Flags: review?(michal.novotny) → review-
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.
Attachment #582685 - Flags: review?(jduell.mcbugs)
You're right, this is a much better fix.
Attachment #582734 - Flags: review?(michal.novotny)
Attachment #582685 - Attachment is obsolete: true
Attachment #582734 - Attachment is obsolete: true
Attachment #582734 - Flags: review?(michal.novotny)
Attachment #582735 - Flags: review?(michal.novotny) → review+
Thanks for finding the time to work on this, jdm.  Always nice to see you in necko-land. Come visit more often! :)
https://hg.mozilla.org/mozilla-central/rev/b121a045b451
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Blocks: 713172
No longer blocks: 713172
You need to log in before you can comment on or make changes to this bug.