Enable mojo localstorage to actually store on disk.

content/renderer/dom_storage/local_storage_cached_area.cc

Index: content/renderer/dom_storage/local_storage_cached_area.cc
diff --git a/content/renderer/dom_storage/local_storage_cached_area.cc b/content/renderer/dom_storage/local_storage_cached_area.cc
index a7ba25ed57f06cf00d6ece81ee364c866c84134d..7721c79ac6fcd196a0920f51f2ff21e9094adec7 100644
--- a/content/renderer/dom_storage/local_storage_cached_area.cc
+++ b/content/renderer/dom_storage/local_storage_cached_area.cc
@@ -150,7 +150,15 @@ void LocalStorageCachedArea::RemoveItem(const base::string16& key,
 
 void LocalStorageCachedArea::Clear(const GURL& page_url,
                                    const std::string& storage_area_id) {
-  // No need to prime the cache in this case.
+  // No need to prime the cache in this case. We still need to make sure the
+  // LevelDBWrapper is actually ready and connected to a database to ensure
+  // changes will get written to the database if the browser quits soon.
+  if (!map_) {
+    base::TimeTicks before = base::TimeTicks::Now();
+    leveldb_->Sync();

[michaeln, 2017/01/25 02:28:38]

that's unfortunate?

would an async call to storage_partition_service->ClearLocalStorage that does
not rely on the level_db_ instance being setup help avoid this Sync() call?

[Marijn Kruisselbrink, 2017/01/25 22:00:07]

Yes, it is. I don't really see a way around it though unfortunately. At least it
seems from my limited testing that committing changes still works fine even if
that has to happen during browser shutdown. But setting up the connection to
leveldb, verifying its schema version etc just doesn't, probably partially
because of the many thread hops involved.
The whole reason for the sync call is to make sure the level db instance is
setup. I would kind of expect most real life usage of localstorage to not start
out by clearing it all, so the performance impact of this should be fairly
minimal. I just want to avoid possibly losing all writes in a situation where a
page happens to start out by clearing storage. Specifically, without this sync
call here the browser test I've added doesn't pass because the browser shuts
down somewhere in the middle of still setting up the connection to the database.

Also not sure if it is guaranteed that committing things during shutdown works;
it just seems StoragePartitions are destroyed early enough in the shutdown
process for things to still work. But I don't quite understand what all the
threads are that are involved, and in what order, so it's very well possible
that there still are situations where changes that javascript thinks were
written end up getting lost anyway. More research is necessary, but at least
starting out from a situation where uncommitted changes can only exist if there
is a connection to the database makes it more plausible that this actually is a
solvable problem.

[michaeln, 2017/01/25 23:53:28]

I see, the problem is not the renderer-to-browser connection to the
leveldb_wrapper, but the browser processes internal connection from
mojocontext-to-LevelDBDatabase.

About the general problem of committed during shutdown, the way it works on CrOS
is problematic. There's a very limited about of time before the process is
killed and its completely racy whether localstorage data is flushed or not. A
long time ago I started looking at a change to BrowserProcessImpl::EndSession to
at least try to wait for localstorage to be flushed.

https://codereview.chromium.org/1140013003/diff/80001/chrome/browser/browser_...

+    base::TimeDelta time_to_prime = base::TimeTicks::Now() - before;
+    UMA_HISTOGRAM_TIMES("LocalStorage.MojoTimeToPrimeClear", time_to_prime);
+  }
 
   Reset();
   map_ = new DOMStorageMap(kPerStorageAreaQuota);