1045886 - Remove Cache directory from Android profiles

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Richard Newman [:rnewman]]

Apparently we're using cache2 now. That uses the Android cache directory for storage.

And yet we have hundreds of megabytes hanging around in Cache folders inside profiles, which won't be cleaned up, and are apparently not used.

We should blow these away.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

*cough*
https://bugzilla.mozilla.org/show_bug.cgi?id=961024#c9

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Actually that was about having 2 different cache2 directories.  This one is a bit more surprising, as there is supposed to be code to clean up the cache that is not in use: CacheObserver::SchduleAutoDelete + CacheStorageService::CleaupCacheDirectories, but that seems to have gone now? 
http://mxr.mozilla.org/mozilla-central/ident?i=kAutoDeleteCacheVersion

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Honza - Can you take a look at this?

Comment 5

[Jason Duell]

Michal, I assume you can take this?  We've got less than 10 days to fix but I'm guessing it shouldn't be hard.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Remove Cache directory from Android profiles

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8470480 [details] [diff] [review]
Remove Cache directory from Android profiles

I think setting this pref should be enough.
I pushed to try to get a build that I can try out on my phone.

Comment 8

[Richard Newman [:rnewman]]

Try might not be super helpful here -- it'll have new testing profiles, not real phone profiles with the old cache.

Things to double-check and think about:

* That sessionstore-windows-restored always fires, even when Fennec isn't set to restore windows from last time. It's not documented.

* But: doing even the initialization work during sessionstore-windows-restored might be too early. I'd be inclined to use the delayed start handler (Bug 964510) -- listen for browser-delayed-startup-finished instead of sessionstore-windows-restored. That way it's less likely to impact the user experience.

* Conversely, doing the actual work after a 30-second delay might be too long for the "click a link in Twitter" crowd; there's no guarantee that the browser will still be running 30 seconds later. Any clues as to why that number was picked?

A quick skim of CacheStorageService doesn't fill me with confidence that the profileless cache in the trash will get cleaned up if Gecko terminates between the move and the actual deletion. We need to check this.

* It looks like nsDeleteDir tends to move files to the Android cache directory, queuing the deletion on a new thread. I believe the rationale for that is to do the (cheap) move early, and the expensive removal later on another thread.

It's entirely possible that the Android cache directory for the app is on a separate filesystem, so moving 300MB is a Terrible Idea®. If the C++ equivalent of getCacheDir returns something like /data/data/org.mozilla.fennec/cache, then we're probably fine; if it returns anything else, be wary.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

I tested the build from try, and it seems after flipping the pref to turn on cache2, and restarting the browser, the old cache folder was removed. It was located at /data/data/org.mozilla.fennec/files/mozilla/siefekzn.default/Cache

Richard's concerns seem valid though, so I'll try to see if I can address them. Michal might know more about why that number was picked as a delay.

Comment 10

[Michal Novotny [:michal]]

Comment on attachment 8470480 [details] [diff] [review]
Remove Cache directory from Android profiles

Clearing review request, since as per our discussion on IRC, new patch is needed.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Remove Cache directory from Android profiles

Comment 12

[Michal Novotny [:michal]]

Comment on attachment 8470867 [details] [diff] [review]
Remove Cache directory from Android profiles

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

(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #0)
> Apparently we're using cache2 now. That uses the Android cache directory for
> storage.
> 
> And yet we have hundreds of megabytes hanging around in Cache folders inside
> profiles, which won't be cleaned up, and are apparently not used.

We don't delete the old cache directory on desktop either. As far as I remember, Honza wanted to delete the old cache once we switch to the new cache permanently. Right now, the new backend is enabled via temp preference. I'm not against deleting the old cache now, but we need to be consistent with the desktop. So I think we should remove auto_delete_cache_version preference from browser/app/profile/firefox.js and put a common preference to modules/libpref/init/all.js.


(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #8)
> * That sessionstore-windows-restored always fires, even when Fennec isn't
> set to restore windows from last time. It's not documented.
> 
> * But: doing even the initialization work during
> sessionstore-windows-restored might be too early. I'd be inclined to use the
> delayed start handler (Bug 964510) -- listen for
> browser-delayed-startup-finished instead of sessionstore-windows-restored.
> That way it's less likely to impact the user experience.

I wouldn't be worried about user impact since we postpone as much of the actual work as we can using timer. But if it not guaranteed that sessionstore-windows-restored is always fired then we should probably listen to other notification.


> * Conversely, doing the actual work after a 30-second delay might be too
> long for the "click a link in Twitter" crowd; there's no guarantee that the
> browser will still be running 30 seconds later. Any clues as to why that
> number was picked?

I don't see this to be a problem. Short session cannot generate too much new data, so the old cache is just removed to trash and it will be deleted later.


> * It looks like nsDeleteDir tends to move files to the Android cache
> directory, queuing the deletion on a new thread. I believe the rationale for
> that is to do the (cheap) move early, and the expensive removal later on
> another thread.
> 
> It's entirely possible that the Android cache directory for the app is on a
> separate filesystem, so moving 300MB is a Terrible Idea®. If the C++
> equivalent of getCacheDir returns something like
> /data/data/org.mozilla.fennec/cache, then we're probably fine; if it returns
> anything else, be wary.

This is definitely not a problem for the new cache since it is located at the same place as the trash. It could be a problem in case of removing the old cache, but can it really happen that CACHE_DIRECTORY is different from /data/data/org.mozilla.fennec/cache or that it is on a different partition than /data/data/org.mozilla.fennec/files?

::: mobile/android/app/mobile.js
@@ +56,5 @@
>  pref("browser.cache.disk.smart_size.enabled", true);
>  pref("browser.cache.disk.smart_size.first_run", true);
>  
> +// Bug 1045886: Remove cache directory from android profiles
> +pref("browser.cache.auto_delete_cache_version", 0);

See above, please make a common preference in modules/libpref/init/all.js

::: netwerk/cache2/CacheObserver.cpp
@@ +449,5 @@
>      CacheFileIOManager::OnProfile();
>      return NS_OK;
>    }
>  
> +#if defined(MOZ_WIDGET_ANDROID)

I don't see a reason why to listen to a different notification on Android and desktop.

Comment 13

[Richard Newman [:rnewman]]

(In reply to Michal Novotny (:michal) from comment #12)

> We don't delete the old cache directory on desktop either. As far as I
> remember, Honza wanted to delete the old cache once we switch to the new
> cache permanently.

If we need to be consistent, then yes, let's delete it right now on every platform. (Being consistent by keeping it on mobile is not an option.)

I don't view potentially starting over with an empty cache (if the user switches between cache versions) as worth handling. Am I missing some other reason to keep the old cache?


> I wouldn't be worried about user impact since we postpone as much of the
> actual work as we can using timer.

Don't we do the initial file move in the initial handler? I seem to remember reading that.

Regardless, on mobile we care about work down to the granularity of Cu.import and running any code at all, so the more we can avoid doing *any* work during startup, the better. That's why we added the delayed startup handler.


> I don't see this to be a problem. Short session cannot generate too much new
> data, so the old cache is just removed to trash and it will be deleted later.

That's my concern -- so long as it does get deleted later. This might be an edge case that desktop doesn't really hit.

 
> This is definitely not a problem for the new cache since it is located at
> the same place as the trash. It could be a problem in case of removing the
> old cache, but can it really happen that CACHE_DIRECTORY is different from
> /data/data/org.mozilla.fennec/cache or that it is on a different partition
> than /data/data/org.mozilla.fennec/files?

I've been around Android long enough that nothing would surprise me. It's technically possible for the cache dir to be something like /Caches/org.mozilla.fennec/. We also support having the app (but not data) on the SD card, so I have no idea what happens then.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: Remove Cache directory from Android profiles

Comment 15

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48e333722371

Comment 16

[Ed Morley [:emorley]]

Backed out for assertions during test_bug437844.xul:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=48e333722371&jobname=mochitest-other

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1ad12a3939

Comment 17

[Valentin Gosu [:valentin] (he/him)]

I have looked at the assertions
Assertion count 21 is greater than expected range 19-20 assertions.

Before the patch:
http://pastebin.mozilla.org/5945795
3 assertions in nsXULElement.cpp:2578 (###!!! ASSERTION: writing to the cache file, but the XUL cache is off?)
17 assertions in nsLineLayout.cpp:934 (###!!! ASSERTION: bad inline size: 'metrics.ISize(lineWM) >= 0')

After the patch:
http://pastebin.mozilla.org/5945800
3 assertions in nsXULElement.cpp:2578 (###!!! ASSERTION: writing to the cache file, but the XUL cache is off?)
18 assertions in nsLineLayout.cpp:934 (###!!! ASSERTION: bad inline size: 'metrics.ISize(lineWM) >= 0')

So it seems there's an extra assertion (caused by a reflow? from nsBlockFrame::ReflowInlineFrame).
Not sure how this patch is causing that extra assert.
Interestingly, the assertion count used to be 20-21 (which would put our patch in that range), but RyanVM changed it because of a perma-orange in changeset 185894:80788eb70e2d

He suggested changing it back. I just pushed it to try, and if everything is green, I'll push it to inbound tonight.

Comment 18

[Valentin Gosu [:valentin] (he/him)]

https://tbpl.mozilla.org/?tree=Try&rev=959e041e5444

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Attachment: Increase assertion count in test_bug437844.xul

Comment 20

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4269c52c4f62
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2268f67609

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4269c52c4f62
https://hg.mozilla.org/mozilla-central/rev/ab2268f67609

Comment 22

[Richard Newman [:rnewman]]

Valentin: please request uplift for this when ready. We will probably miss cutoff for beta, but if you're confident that this is safe, I'd argue for it. I don't want us losing users because of this for another six weeks.

AaronMT: could you make sure this gets verified ASAP so we can uplift?

Comment 23

[Aaron Train [:aaronmt]]

Sure. Here's what I double checked here from what I gather:

  31.0

    root@klte:/data/data/org.mozilla.firefox_beta/files/mozilla/fb72yswk.default # du -sk Cache                                                                   

    18196	Cache
    
    root@klte:/data/data/org.mozilla.firefox_beta/files/mozilla/fb72yswk.default # ls -l Cache                                                                    
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 0
    drwx------ u0_a199  u0_a199           2014-08-13 17:41 1
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 2
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 3
    drwx------ u0_a199  u0_a199           2014-08-13 17:42 4
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 5
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 6
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 7
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 8
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 9
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 A
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 B
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 C
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 D
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 E
    drwx------ u0_a199  u0_a199           2014-08-13 17:48 F
    -rw------- u0_a199  u0_a199   4194304 2014-08-13 17:50 _CACHE_001_
    -rw------- u0_a199  u0_a199   4194304 2014-08-13 17:50 _CACHE_002_
    -rw------- u0_a199  u0_a199   4194304 2014-08-13 17:50 _CACHE_003_
    -rw------- u0_a199  u0_a199     16660 2014-08-13 17:50 _CACHE_MAP_

  Upgraded to 32.0 Beta, browsed a bit, checked on Cache
    
    root@klte:/data/data/org.mozilla.firefox_beta/files/mozilla/fb72yswk.default # du -sH Cache

    36392	Cache

Which confirms that in 32 Beta, right now, currently Cache sticks around.

Took a Nightly 32, browsed a bit

  root@klte:/data/data/org.mozilla.fennec/files/mozilla/yfuk9wp6.default # du -sH Cache                                                   
 
    32152	Cache

Updated to latest mozilla-central (34) (TBPL)

  root@klte:/data/data/org.mozilla.fennec/files/mozilla/yfuk9wp6.default # du -sH Cache 

    Cache: No such file or directory

Let me know if there's anything else to check here, otherwise this looks fine to me.

Comment 24

[Valentin Gosu [:valentin] (he/him)]

Thanks Aaron.

The way the deletion works is it moves the Cache folder from files/mozilla/[profile] to cache/Cache.Trash and sets a timer (30 sec) to delete it.

A couple of other scenarios would be:
* Open Fennec, and close it immediately. Open it again and check that the Cache folder is gone (including the trash).
* Make Fennec be saved on the SD card, open it and check that it deletes the cache. (The cache folder path can be identified by looking at about:cache)

You can switch between the old and the new cache by flipping the browser.cache.use_new_backend_temp pref.

Comment 25

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8471276 [details] [diff] [review]
Remove Cache directory from Android profiles

Approval Request Comment
[Feature/regressing bug #]: Left over from the old cache
[User impact if declined]: Old cache folder will stay in place, possibly taking up much needed space (especially on Android).
[Describe test coverage new/current, TBPL]: Landed on m-c. QA testing completed. We lack automated testing made difficult by the 30 sec timer.
[Risks and why]: Low risk. The patch contains few changes, and has been reviewed by a Necko peer.
[String/UUID change made/needed]: None

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm willing to take this in beta8 as leaving the old cache in place doesn't seem like a good option (as noted in comment 13). I want to give this more time on Nightly before making the uplift decision. The next mobile beta is beta8, which goes to build on Mon, Aug 18. It is definitely late to take this change but I think we still have a reasonable amount of time to test and safely land.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb73a8ce27a2

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Aurora needed the assertion patch as well.
https://hg.mozilla.org/releases/mozilla-aurora/rev/bfdc3bc16b17

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8471276 [details] [diff] [review]
Remove Cache directory from Android profiles

I haven't seen any fallout from this on m-c or aurora. beta+

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8472328 [details] [diff] [review]
Increase assertion count in test_bug437844.xul

Do we need the assertion patch on beta as well?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/07eb5ce30325
https://hg.mozilla.org/releases/mozilla-beta/rev/c444cb84a78b

It's hard to predict if the assertion fix is going to be needed or not, but I'm going to guess that will given Aurora's need for it. I'll back it out if it turns tests orange.

Comment 32

[Valentin Gosu [:valentin] (he/him)]

Thanks Ryan!