pasko@, this seems relevant to your on disk writes - i.e. writing stuff when we need to.

Copying from my response via email: We have base::android::ApplicationStatusListener and it is used in /net to flush the in-memory index of disk cache shortly after Chrome finds that APPLICATION_STATE_HAS_STOPPED_ACTIVITIES. We still have to provide fallbacks for the case when the process gets killed before the state is written to disk.

Ah, thanks! That looks fairly straight-forward to route up. Sounds like we want to do the same for the two SQLite stores.

Keep in mind that WebView doesn't get that signal, and is currently relying on periodic saving.

Adding torne@ to make sure we don't regress WebView in any way.

I mean, processes can also crash or devices lose power. And there are platforms other than Android. I don't think anyone is proposing to drop the periodic saving code or relying on this signal for anything.

I agree with davidben in #4 and #7. I plan to wire up base::android::ApplicationStatusListener to the SQLitePersistentCookieStore and SQLiteChannelIDStore. Their periodic (at least every 30s) saving will stay as a fallback. There will still be cases (crashes, power loss, etc.) where we might lose data, but the ApplicationStatusListener will improve the situation.

Sure; but I recall a change a long time ago that increased one of Chromium's periodic save timeouts and made WebView less reliable which had to be reverted (I don't remember exactly what it was, though). :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83325fee9ee045b846a4965e2773c3bf26bcdba0

commit 83325fee9ee045b846a4965e2773c3bf26bcdba0
Author: Nick Harper <nharper@chromium.org>
Date: Wed May 17 18:38:24 2017

Add android application status listener to //net SQLite stores

This listener listens for APPLICATION_STATE_HAS_STOPPED_ACTIVITIES and commits
any pending changes to the sqlite database.

Bug: 719726
Change-Id: Iddb25fc7c34b5c9e87395c92eecb23becae1c163
Reviewed-on: https://chromium-review.googlesource.com/503516
Reviewed-by: Matt Mueller <mattm@chromium.org>
Commit-Queue: Nick Harper <nharper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#472509}
[modify] https://crrev.com/83325fee9ee045b846a4965e2773c3bf26bcdba0/net/extras/sqlite/sqlite_channel_id_store.cc
[modify] https://crrev.com/83325fee9ee045b846a4965e2773c3bf26bcdba0/net/extras/sqlite/sqlite_persistent_cookie_store.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a1792097d320077386c51680171d0fd66108072

commit 6a1792097d320077386c51680171d0fd66108072
Author: Eric Stevenson <estevenson@chromium.org>
Date: Wed May 17 22:25:24 2017

Revert "Add android application status listener to //net SQLite stores"

This reverts commit 83325fee9ee045b846a4965e2773c3bf26bcdba0.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Add android application status listener to //net SQLite stores
> 
> This listener listens for APPLICATION_STATE_HAS_STOPPED_ACTIVITIES and commits
> any pending changes to the sqlite database.
> 
> Bug: 719726
> Change-Id: Iddb25fc7c34b5c9e87395c92eecb23becae1c163
> Reviewed-on: https://chromium-review.googlesource.com/503516
> Reviewed-by: Matt Mueller <mattm@chromium.org>
> Commit-Queue: Nick Harper <nharper@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#472509}

TBR=pasko@chromium.org,mattm@chromium.org,nharper@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Bug: 719726

Change-Id: I460bd2c2151d74469eb6a0da95520557fee39b79
Reviewed-on: https://chromium-review.googlesource.com/508190
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Commit-Queue: Eric Stevenson <estevenson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#472589}
[modify] https://crrev.com/6a1792097d320077386c51680171d0fd66108072/net/extras/sqlite/sqlite_channel_id_store.cc
[modify] https://crrev.com/6a1792097d320077386c51680171d0fd66108072/net/extras/sqlite/sqlite_persistent_cookie_store.cc

Oops, didn't mean to submit to CQ right away - see  issue 723836  for reason.

I don't think you should block this on issue 470582; sorry if I wasn't clearer in the other bug. There's no actual reason to wait until ApplicationStatus is moved out of base in order to implement this in a WebView-compatible way - just expose a "flush pending data to disk" hook to the content embedder and attach your logic in net to that, and then take your existing ApplicationStatus-based code and put that in src/chrome as the thing which calls that hook. That code is going to look basically the same whether it's implemented before or after I move ApplicationStatus, other than the import statements.

After issue 470582, I can use whatever you do with net/disk_cache/simple/simple_index.(cc|h) for these stores as well.

Drat. I was hoping you would do this first for your feature, and then I could ask you to change net/disk_cache/simple to also use it. :)

More seriously: I'm not an expert in the network stack, and it seems more sensible for someone familiar with the network code to refactor it to stop encoding ApplicationStatus-based policy, or at least explain to me clearly what it should do so that I can write the CL.

e.g. for the sqlite stores, I'd guess it makes sense to have an API called something like.. "flushPersistentStateToDisk" or similar, with a comment explaining that it's a good idea for the embedder to call this if it's transitioning to a state where the hosting process might soon be killed abruptly.

If the usage in simplecache is exactly analogous, and should be attached to the same exact function, then that's good because it's less code to write, but I don't actually know whether that's the case because I haven't read the code in any detail.

So if you want this to actually get done soon, you should at least take a look at the net code and tell me whether my guess above makes sense and seems like a good API, and whether the exact same thing would apply to simplecache or not, because that will increase the chance that I get around to doing it in the near future :)

For simplecache I think flushPersistentStateToDisk is a little too coarse. Currently simplecache manages the flush interval differently depending on whether it is likely to be killed or not. This logic is truly not ideal, and the intervals are not properly tuned, but I'd prefer to keep it for now and experiment with better options/params independently from the refactoring.

Consistency requirements for simplecache are currently softer than for the cookie storage (but we should probably discuss changing it for SW caches because we recently realized that those are not supposed to silently drop cache entries, even in rare cases).

I am not sure what consistency requirements are for channel id store.

So to reflect what we are currently doing, it would be something like having SetLikelyToBeKilled(bool) exposed from disk cache. See:

https://codesearch.chromium.org/chromium/src/net/disk_cache/simple/simple_index.cc?l=483

Why not just flush()? Well, we can spend an extended period of time in the state of 'likely to be killed' and the activity on the cache will be small in such states (JS timers somewhat throttled), but when it is actually killed, the synched state is not ideal (will make the backend slower to fully start next time). So one way to react on SetLikelyToBeKilled(true) is to switch to 'more expensive syncing mode' for every operation  (caveat: simplecache is not doing so now, only tweaks the flush interval).

I will look into how to plumb some sort of hook from the content embedder to the appropriate parts of net stack, to use for the cookie/channel id stores. I'm planning on doing some refactoring/cleanup work around cookie stores and channel ID stores (with the goal of consolidating them into a single store), so adding the android application status listener will probably come after that (so I only have to add it in one place instead of two).

SetLikelyToBeKilled(bool) would be a pretty difficult API for WebView to use in a realistic way, because that's the exact class of thing we can't tell reliably due to pretty much the same various reasons why we can't implement ApplicationStatus.

There are times when we can tell that we're probably not going to be killed, because we know we're attached to an activity that's running; but unlike Chrome it's not necessarily the case that we're likely to be killed when we're *not* attached to an activity that's running, because WebView is used in a lot of weird ways. There are also practical problems with actually tracking the activity lifecycle accurately even when we are associated with one.

So, if you provided an API like that, I'm not sure we'd actually bother to call it, because we might not think it's worth the trouble to approximate this state badly.

This is what I mean by not encoding policy: SetLikelyToBeKilled is effectively just "is in foreground" by another name, and doesn't actually expose anything meaningful about what the consequences of toggling it will actually be, so it's still encoding the policy into the net layer.

I'm not sure what a good answer is for this case, to be honest. What's the actual current behaviour in WebView? Presumably the state transitions never occur, but does that mean that simple cache is always flushing *more* often, or always flushing *less* often?

This bug is about the cookie store, not simple cache. If the state transitions never occur, Nick's CL would not make the cookie store flush any more or less than it did before. If WebView never calls this API, that's fine.

The goal in this bug is merely to not regress WebView. The cookie store already flushes periodically to be safe against crashes and to service non-Android platforms. That's not changing here and there is no proposal to change it. Even when the signal is available, we would still want to keep the periodic flush. This is just to do slightly better on Chrome for Android, where it is easy to do so.

It would be nice to make it work for WebView too, but it sounds like this is still an open question. Solving it for Chrome for Android is not going to make it harder to solve it for WebView later. Once the right trigger is figured out for WebView, that can be routed up too. Thus there is no reason for WebView to hold back this improvement in Chrome for Android.

I'm replying to pasko's suggestion in c#21 - my point is not that you should wait to be able to do the right thing for WebView before doing this at all, it's that you should make sure the thing you do for Chrome is something that WebView *could* conceivably make use of later, which is why I'm proposing an API that's very specific about what it actually does where the policy decision is deferred to higher layers.

I think that for the cookie store (and channel ID store), a simple flush() API would be reasonable. When the application state transitions to APPLICATION_STATE_HAS_STOPPED_ACTIVITIES, whatever glue at the content embedder that listens to that signal from the ApplicationStatusListener can call the flush() method on the cookie (or channel ID) store.

That approach would solve your concern around not having the net stack directly use the ApplicationStatusListener and instead have net's embedder use it. It sounds like this might not be the right approach for simple cache, but this bug isn't about changing simple cache's use of ApplicationStatusListener - it's just about adding a use of ApplicationStatusListener for the cookie store.

Yes. That's what I proposed as the solution back in the issue about this breaking WebView (https://bugs.chromium.org/p/chromium/issues/detail?id=723836#c13) and again in comment 20 here :)

This is no longer needed for Channel ID since that is being deprecated and removed (crbug.com/875053). However, this still seems like a reasonable thing to do for the cookie store.