1050578 - Move disk cache and prefs flushing into application-background listener

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

The disk cache and prefs are flushed right now when the app is backgrounded in AndroidGeckoEvent::APP_BACKGROUNDING:http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#332. That works for onPause, but if the app is killed any other way (QuitNow, guest mode, etc.), these won't be flushed. We should move that code into an application-background listener so it handles all scenarios.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Move disk cache and prefs flushing into application-background listener

Comment 2

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

How is using "application-background" any better? It only seems to be fired from AndroidGeckoEvent::APP_BACKGROUNDING, which is where the cache code is currently located:
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#327

Comment 3

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

I see that this code depends on bug 995138

Comment 4

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

Comment on attachment 8469692 [details] [diff] [review]
Move disk cache and prefs flushing into application-background listener

Brian and I talked about this approach. I don't like it because it abuses the "application-background" notification. If we want to decouple the cache shutdown from onPause, let's create a new "API" for it using two new notifications: "diskcache-shutdown" and "diskcache-startup" (assuming Gecko doesn't already have a message\event\notification API for it).

1. Add observers to nsAppShell for "diskcache-shutdown" and "diskcache-startup"
2. Move the code to do shutdown and startup into the handlers for the notifications in #1
3. Call NotifyObservers for "diskcache-shutdown" in AndroidGeckoEvent::APP_BACKGROUNDING, like we currently do for "application-background"
4. 3. Call NotifyObservers for "diskcache-startup" in AndroidGeckoEvent::APP_FOREGROUNDING, like we currently do for "application-foreground"

That should be all we need for this patch. Bug 995138 can add more notifyObservers for "diskcache-shutdown", as needed.

This way "application-background" stays true to it's nature and we create specific notifications to handle the diskcache.

Comment 5

[Brian Nicholson (:bnicholson)]

The other thing we discussed was the possibility of having multiple cache shutdown messages sent out of parity, which is theoretically possible since we'd have multiple dispatch points. For instance, a shutdown triggered while we're already in the background could be a problem.

One way we could fix this would be to have a very simple CacheManager in browser.js, and the different dispatch points would go through this by sending a "Cache:Shutdown". The CacheManager would internally store a boolean about whether a cache shutdown is already in effect. If so, it would do nothing; otherwise, it would send out the "real" cache shutdown events that the other observers would handle.

The alternative to this would be for every diskcache-shutdown listener to be responsible for checking whether the thing it's shutting down has been shut down already.

Comment 6

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.