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

RESOLVED FIXED in mozilla11

Status

()

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

People

(Reporter: jduell, Assigned: jdm)

Tracking

unspecified
mozilla11
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 1

6 years ago
I'm testing a patch to do this.
Assignee: nobody → josh
(Assignee)

Comment 2

6 years ago
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.
Attachment #580580 - Flags: review?(jduell.mcbugs)
(Reporter)

Updated

6 years ago
Blocks: 709964
(Reporter)

Comment 3

6 years ago
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+
(Reporter)

Comment 4

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

Updated

6 years ago
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"
(Reporter)

Comment 5

6 years ago
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?
(Reporter)

Comment 8

6 years ago
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?
(Assignee)

Comment 9

6 years ago
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 :(
Attachment #582685 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

6 years ago
Attachment #580580 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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)

Comment 11

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

Comment 13

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

Comment 14

6 years ago
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.
Attachment #582734 - Flags: review?(michal.novotny)
(Assignee)

Updated

6 years ago
Attachment #582685 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
Created attachment 582735 [details] [diff] [review]
Disable disk cache if the user chooses to sanitize private data on shutdown.
Attachment #582735 - Flags: review?(michal.novotny)
(Assignee)

Updated

6 years ago
Attachment #582734 - Attachment is obsolete: true
Attachment #582734 - Flags: review?(michal.novotny)
Attachment #582735 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b121a045b451
(Reporter)

Comment 17

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

6 years ago
Blocks: 713172
(Assignee)

Updated

6 years ago
No longer blocks: 713172
You need to log in before you can comment on or make changes to this bug.