538588 - Options -> Advanced -> Network -> Offline Storage confusing: Clear Now button only affects disk cache

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Nickolay_Ponomarev]

1. Open an "offline-enabled" web app, like http://demos.hacks.mozilla.org/openweb/todo/
2. Allow it to use the offline cache in the notification bar that appears (this one - https://developer.mozilla.org/en/Offline_resources_in_Firefox#Storage_location_and_clearing_the_offline_cache )
3. Go to Options/Preferences -> Advanced -> Network.
4. Click "Clear Now" button in the "Offline Storage" section.

Actual results:
Disk cache is cleared, offline cache is not (you can see this in about:cache, also in the UI the site is still in the "The following sites have stored data for offline use" list in the same section of the preferences!

There's no user-visible feedback at all, looks broken.

Expected results:
1) Clear Now button separated from the "Offline Storage" and clears only the disk cache
-or-
2) Clear Now button also clears the offline cache.

Comment 2

[juan becerra [:juanb]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2) Gecko/20100105 Firefox/3.6 (RC1)

While running https://litmus.mozilla.org/show_test.cgi?id=8844 I found that clear recent history, or clearing offline storage doesn't remove "offline cache device" data, on all platforms. I went to http://starkravingfinkle.org/projects/offline/todo.html and allowed the site to store data for offline use.

When I clear recent history I see that the disk cache device data is cleared, but not the offline cache device data as displayed in about:cache. If you list the cache entries, you can't really anything there, but it seems confusing.

This also happens in 3.5.7.

Comment 3

[Nickolay_Ponomarev]

"clear recent history" is a different bug - bug 538595.

What do you mean by "clearing offline storage"? There's no such thing in the UI or in the litmus testcase.

Comment 4

[juan becerra [:juanb]]

Prefereces/Advanced/Network - (Offline Storage) - Clear Now is what I was referring to as "clearing offline storage".

Tools / Clear Recent History / Expand Details / Cache (selected) is what I was referring to as "clear recent history" and which is, as you say, bug 538595.

I was expecting both of these to do the same thing, which it sort of does currently.

The litmus test case doesn't make it explicit. I was just testing around.

Comment 5

[Michal Novotny [:michal]]

Attachment: possible fix

This patch adds information about the size of the application cache and a button to clear it.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: suggestion 1

According the UI, I would suggest more dividing.  Offline cache is really something else then the normal cache, and since this is the Advanced tab, we can be more detailed and technical here, IMO.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 573352 [details] [diff] [review]
possible fix

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

f+/- (= 0).

::: browser/components/preferences/advanced.js
@@ +350,5 @@
> +      var host = list.firstChild.getAttribute("host");
> +
> +      // send out an offline-app-removed signal.  The nsDOMStorage
> +      // service will clear DOM storage for this host.
> +      obs.notifyObservers(null, "offline-app-removed", host);

Instead, you can use nsIDOMStorageManager.clearOfflineApps().

Better see:
http://hg.mozilla.org/mozilla-central/annotate/53f4c8abf558/browser/base/content/sanitize.js#l204

@@ +356,5 @@
> +      // remove the permission
> +      pm.remove(host, "offline-app",
> +                Components.interfaces.nsIPermissionManager.ALLOW_ACTION);
> +      pm.remove(host, "offline-app",
> +                Components.interfaces.nsIOfflineCacheUpdateService.ALLOW_NO_WARN);

The clear dialog doesn't do this...  Question is if to do it here...  However, there is no place to remove the permission anywhere.  Not sure whether the selective clear (check boxes in the list and pressing [Remove...]) does it or not.  Worth check it and stay in parity with it.  I personally am for adding this removal because it is missing and I was missing it few times my self.

Some code sharing with sanitize.js via some jsm would be great.

Comment 8

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 573352 [details] [diff] [review]
possible fix

This looks fine.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 574605 [details]
suggestion 1

Isn't this a bit better arranged?  Just adding a separator, nothing more, since offline cache is something quit different from normal http cache.

This is the Advanced tab, we can be quit technical here.

Comment 10

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 574605 [details]
suggestion 1

WFM.

Comment 11

[Michal Novotny [:michal]]

Attachment: patch v2

(In reply to Honza Bambas (:mayhemer) from comment #7)
> @@ +356,5 @@
> > +      // remove the permission
> > +      pm.remove(host, "offline-app",
> > +                Components.interfaces.nsIPermissionManager.ALLOW_ACTION);
> > +      pm.remove(host, "offline-app",
> > +                Components.interfaces.nsIOfflineCacheUpdateService.ALLOW_NO_WARN);
> 
> The clear dialog doesn't do this...  Question is if to do it here... 
> However, there is no place to remove the permission anywhere.  Not sure
> whether the selective clear (check boxes in the list and pressing
> [Remove...]) does it or not.  Worth check it and stay in parity with it.  I
> personally am for adding this removal because it is missing and I was
> missing it few times my self.

The "remove" button does it. I.e. clicking "clear now" button is equivalent to removing all items via "remove" button.


> Some code sharing with sanitize.js via some jsm would be great.

Do you have some concrete functions in mind?

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 576529 [details] [diff] [review]
patch v2

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

Nice, thanks!

r=honzab with few nits.


(In reply to Michal Novotny (:michal) from comment #11)
> > Some code sharing with sanitize.js via some jsm would be great.
> 
> Do you have some concrete functions in mind?

This particular:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#204

But I don't know how to share code between modules inside the browser module, so let's not bother.

::: browser/components/preferences/advanced.js
@@ +338,5 @@
> +    } catch(ex) {}
> +    this.updateActualAppCacheSize();
> +
> +    var obs = Components.classes["@mozilla.org/observer-service;1"]
> +                        .getService(Components.interfaces.nsIObserverService);

This seems to be unused..

@@ +347,5 @@
> +    var list = document.getElementById("offlineAppsList");
> +
> +    var storageMgr = Components.classes["@mozilla.org/dom/storagemanager;1"].
> +                     getService(Components.interfaces.nsIDOMStorageManager);
> +    storageMgr.clearOfflineApps();

Be consistent with dots.  I personally prefer to have it at the start of a new line (as in all previous cases).

This storage manager snippet is inside the logical code block for removing permission for the host list.

Please, move the blocks logically together and maybe just simply comment them.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> But I don't know how to share code between modules inside the browser
> module, so let's not bother.

We have bug 397719 on file, this is something that would be good to do in general.

Refactoring code into a JS module for re-use is not difficult - we should do that here if applicable, rather than copying code.

(might go without saying, but this patch should also get review from a browser peer)

Comment 14

[Honza Bambas (:mayhemer)]

Bug 397719 is quit dead, and this bug is a Q4 goal, so I would rather do the sharing in a followup bug or directly in bug 397719 it self.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I don't really understand why this bug is anyone's Q4 goal, but regardless, we shouldn't fix it hastily just to meet some arbitrary deadline. Factoring the code properly shouldn't be a difficult task; I'm happy to help out. I'll look into this more tomorrow.

Comment 16

[Honza Bambas (:mayhemer)]

It's part of improvements for application cache usage.  If we can have a sharing code or guidelines how to do it and then get it quickly reviewed then let's do it.  Otherwise I would like to avoid blocking a simple patch on arbitrary architectural changes.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

I don't think that's a fair characterization - I'm not suggesting a rewrite or refactor of any existing code, I'm just asking you to avoid *adding* additional duplicated code. Adding a Sanitize.jsm that includes a method with only the clearOfflineApps()/evictEntries(Components.interfaces.nsICache.STORE_OFFLINE) code that's shared between this code and the offlineApps clear() function in sanitize.js is simple - ping me on IRC if you have any questions.

The updateActualAppCacheSize code also seems like it could also be factored out, since it's almost identical to the existing updateActualCacheSize.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 576529 [details] [diff] [review]
patch v2

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

I work on the code sharing after discussion with Gavin on IRC.  There will be a new jsm file in browser/base/content for offline cache+user data deletion.

::: browser/components/preferences/advanced.js
@@ +233,5 @@
> +    cacheService.visitEntries(visitor);
> +  },
> +
> +  // Retrieves the amount of space currently used by application cache
> +  updateActualAppCacheSize: function ()

Gavin also suggests to turn this to a parametrized method since updateActualCacheSize and updateActualAppCacheSize are mostly the same except the strings.

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: code sharing patch v1

So, this works for me and also work with patch for bug 538595.

Michal, feel free to integrate this patch into your new version.  This one applies over your v2 patch.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 578427 [details] [diff] [review]
code sharing patch v1

Looks good to me. When we fix bug bug 397719 we'll want to merge this module into that one.

Comment 21

[Michal Novotny [:michal]]

Attachment: updated patch

- merged with Honza's patch #578427
- removed unused code
- merged methods updateActualCacheSize and updateActualAppCacheSize

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 578443 [details] [diff] [review]
updated patch

>diff --git a/browser/base/content/offlineCache.jsm b/browser/base/content/offlineCache.jsm

>+let OfflineCacheHelper = {

>+    try {
>+      // Offline app data is "timeless", and doesn't respect
>+      // the setting of timespan, it always clears everything

This comment isn't particularly relevant in this context, just remove it?

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js

>+  updateActualCacheSize: function (device)

>     var visitor = {
>+      device : null,

>+    visitor.device = device;

This shouldn't be needed, AFAICT.

>+  /**
>+   * Clears the application cache.
>+   */
>+  clearAppCache: function ()

It's confusing to refer to both "application cache" and "offline cache" in the code (and even "offline-app" permissions). Can we consistently use offlineAppCache, maybe?

>+      // remove the permission

It's a little bit odd that clearing the offlineAppCache also clears the ability for sites to use it in the future. This stuff is generally handled separately, why are we including it here?

>diff --git a/browser/locales/en-US/chrome/browser/preferences/preferences.properties b/browser/locales/en-US/chrome/browser/preferences/preferences.properties

>+offlineAppClearPrompt=Are you sure you want to remove offline data of all websites?
>+offlineAppClearConfirm=Clear

This prompt doesn't really flow very well - "remove" vs. "clear" discrepancy should be fixed at the very least. Do we need to prompt here at all? If we do, we should probably do a better job of explaining the negative effects this action might have in the prompt. But I'd rather not prompt at all, particularly if we remove the removal of permissions.

>+actualDiskCacheSize=Your disk cache is currently using %1$S %2$S of disk space

>+actualAppCacheSize=Your application cache is currently using %1$S %2$S of disk space

"Disk cache" vs. "application cache" is also jargony and misleading (both of them are on disk!). These concepts are difficult to usefully expose (this is the "advanced" pane, after all!). Can we use "Web content cache" and "offline application cache" consistently? We can file a followup to expose all of this properly, but I don't want to make things worse in the interim.

Comment 23

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #22)
> >+      // remove the permission
> 
> It's a little bit odd that clearing the offlineAppCache also clears the
> ability for sites to use it in the future. This stuff is generally handled
> separately, why are we including it here?

One of the reasons people clear their caches is to eliminate evidence of what websites they've been to recently. So, I think it definitely makes sense to do this for "Clear Recent History" and other privacy-oriented actions. I don't really know well enough whether this should be considered a privacy-oriented action though.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to Brian Smith (:bsmith) from comment #23)
> One of the reasons people clear their caches is to eliminate evidence of
> what websites they've been to recently. So, I think it definitely makes
> sense to do this for "Clear Recent History" and other privacy-oriented
> actions.

"Clear Recent History" is covered in bug 538595, and as mentioned in that bug, that dialog already has an entry that clears these permissions ("Site Preferences").

Comment 25

[Michal Novotny [:michal]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #22)
> >+      // remove the permission
> 
> It's a little bit odd that clearing the offlineAppCache also clears the
> ability for sites to use it in the future. This stuff is generally handled
> separately, why are we including it here?

I don't have a strong opinion on this, but if we decide to not clear the permission then "Clear now" isn't equal to removing all websites via "remove" button. So should we change removeOfflineApp() to not delete permission too? Also we would need to change the list of sites so that the size of the permission isn't included in the usage and we would need to hide sites for which we have just stored permission but no cache entries, right?

Comment 26

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

(In reply to Michal Novotny (:michal) from comment #25)
> I don't have a strong opinion on this, but if we decide to not clear the
> permission then "Clear now" isn't equal to removing all websites via
> "remove" button.

I understand now. I think that it is OK that "Clear now" isn't the same as removing all the websites via the "Remove" button. "Clear now" is used to free up disk space. "Remove" is used to remove that site's ability to use offline storage. When the user pushes "Clear now" and sees that all the sites are listed in the "The following websites have stored data for offline use" dialog box, they will know that they should "Remove" them if they don't want them.

By the way, I think "The following websites have stored data for offline use" is the wrong label; it should be ""The following websites are allowed to store data for offline use".

> So should we change removeOfflineApp() to not delete
> permission too?

No. AFAICT, the primary purpose of the "Remove" button is to remove the permission; freeing up the disk space used by the removed site is just a nice side-effect.

> Also we would need to change the list of sites so that the
> size of the permission isn't included in the usage

Yes.

> and we would need to hide
> sites for which we have just stored permission but no cache entries, right?

Like I said above, AFAICT, this box should list every site that has the permission, regardless of whether or not it is actually storing anything offline.

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #26)
> (In reply to Michal Novotny (:michal) from comment #25)
> > I don't have a strong opinion on this, but if we decide to not clear the
> > permission then "Clear now" isn't equal to removing all websites via
> > "remove" button.
> 
> I understand now. I think that it is OK that "Clear now" isn't the same as
> removing all the websites via the "Remove" button. "Clear now" is used to
> free up disk space. "Remove" is used to remove that site's ability to use
> offline storage. When the user pushes "Clear now" and sees that all the
> sites are listed in the "The following websites have stored data for offline
> use" dialog box, they will know that they should "Remove" them if they don't
> want them.

I agree with these proposals.  It makes sense.  True is that now it is not very clear what the buttons do (for users) or should do (for us, developers).

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 578443 [details] [diff] [review]
updated patch

Brian's suggestions sound good - I'll wait for a new patch!

Comment 29

[Michal Novotny [:michal]]

Attachment: patch v5 - fixed according to reviewers' comments

(In reply to Brian Smith (:bsmith) from comment #26)
> > Also we would need to change the list of sites so that the
> > size of the permission isn't included in the usage
> 
> Yes.

This isn't IMO possible since nsIDOMStorageManager::getUsage() returns usage that includes size of the permissions. This means that after clearing the whole offline application cache the list shows domains with small sizes like 14 bytes etc.

Comment 30

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

(In reply to Michal Novotny (:michal) from comment #29)
> This isn't IMO possible since nsIDOMStorageManager::getUsage() returns usage
> that includes size of the permissions. This means that after clearing the
> whole offline application cache the list shows domains with small sizes like
> 14 bytes etc.

Michal and I discussed this on IRC. I think it is fine to keep including the size of the permission. It will be a little confusing that "Clear now" doesn't cause the number to go all the way to zero all the time, but it is somewhat accurate. If we want the number to always drop to zero, then let's do that in a (lower-priority) followup bug.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to Michal Novotny (:michal) from comment #29)
> This isn't IMO possible since nsIDOMStorageManager::getUsage() returns usage
> that includes size of the permissions.

This seems silly. Can't we just stop doing that? Though looking at the code, I don't see where it takes permissions into account - it seems to be straight SQL?

Comment 32

[Michal Novotny [:michal]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #31)
> This seems silly. Can't we just stop doing that? Though looking at the code,
> I don't see where it takes permissions into account - it seems to be
> straight SQL?

I was wrong, this isn't a size of the permission. If I clear the whole cache and restart firefox, then it shows 0 bytes. I'll try to find out what's left in the database until the shutdown...

Comment 33

[Michal Novotny [:michal]]

Attachment: patch v6 - fixed invalidation of mCachedUsage in nsDOMStoragePersistentDB

The problem is in caching of usage in nsDOMStoragePersistentDB. I'm not sure that I understand correctly all the code around excluding offline usage and that the fix is correct, but I'm sure that the current code is broken. E.g. in nsDOMStoragePersistentDB::RemoveOwner(), the domain is compared with mCachedOwner after appending an asterisk, but mCachedOwner can never contain it.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

I think it would be best to spin the dom/ changes into a separate followup bug, we don't need to fix these at the same time.

Comment 35

[Michal Novotny [:michal]]

Attachment: patch v5

(In reply to Gavin Sharp (use [e-mail] for email) from comment #34)
> I think it would be best to spin the dom/ changes into a separate followup
> bug, we don't need to fix these at the same time.

Reverting back to patch v5. The follow up bug is #707915.

Comment 36

[Honza Bambas (:mayhemer)]

Gavin, ping on review here.  This is a Q4 goal for the networking team and is quit a simple patch.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 579273 [details] [diff] [review]
patch v5

>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd

>+<!ENTITY offlineStorage2.label           "Offline Web Content and User Data">

>diff --git a/browser/locales/en-US/chrome/browser/preferences/preferences.properties b/browser/locales/en-US/chrome/browser/preferences/preferences.properties

>+actualDiskCacheSize2=Your web content cache is currently using %1$S %2$S of disk space

Why do these have a "2" suffix? They look like new strings, so that shouldn't be necessary.

Comment 38

[Michal Novotny [:michal]]

Attachment: patch v7

(In reply to Gavin Sharp (use [e-mail] for email) from comment #37)
> Comment on attachment 579273 [details] [diff] [review]
> patch v5
> 
> Why do these have a "2" suffix? They look like new strings, so that
> shouldn't be necessary.
> 
> >+actualDiskCacheSize2=Your web content cache is currently using %1$S %2$S of disk space

Fixed.


> >+<!ENTITY offlineStorage2.label           "Offline Web Content and User Data">

This one isn't new.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

You'll have to rebase this on top of bug 699575 - sorry about that! Should be trivial, just need to move the module to browser/modules.

Comment 40

[Michal Novotny [:michal]]

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

Comment 41

[Matt Brubeck (:mbrubeck)]

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

Comment 42

[Daniel Holbert [:dholbert]]

Adding dev-doc-needed, since https://developer.mozilla.org/en/Offline_resources_in_Firefox explicitly calls out this bug as being an existing issue.

(Once Firefox 11 is released, I assume we'll want to update that MDN article.)

Comment 43

[Eric Shepherd [:sheppy]]

Doc updated:

https://developer.mozilla.org/en/HTML/Using_the_application_cache

(the one mentioned in comment 42 has been moved to that location)