[Cronet] Purge storage directory if version does not match

components/cronet/android/cronet_url_request_context_adapter.cc

Index: components/cronet/android/cronet_url_request_context_adapter.cc
diff --git a/components/cronet/android/cronet_url_request_context_adapter.cc b/components/cronet/android/cronet_url_request_context_adapter.cc
index fcc1ea7bf6b39c9185944a9e8e242a8b90724378..bdf0db5e2ef9e02cb4637fb916e5ae5d45642c34 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -55,6 +55,13 @@
 namespace {
 
 const char kHttpServerProperties[] = "net.http_server_properties";
+// Current version of disk cache.
+const int32_t kDiskCacheVersion = 1;
+
+struct VersionHeader {
+  VersionHeader() { std::memset(this, 0, sizeof(*this)); };

[mmenke, 2016/03/31 15:39:07]

Think we should just just direct initialize version.  I'd add a
kDiskCacheVersionUnknown = 0 or something, and use that, just to be paranoid.

[xunjieli, 2016/03/31 18:45:58]

Done.

+  uint32_t version;

[mmenke, 2016/03/31 15:39:07]

include <stdint.h>

[xunjieli, 2016/03/31 18:45:58]

Done.

+};
 
 // Connects the HttpServerPropertiesManager's storage to the prefs.
 class PrefServiceAdapter
@@ -263,6 +270,58 @@ class BasicNetworkDelegate : public net::NetworkDelegateImpl {
   DISALLOW_COPY_AND_ASSIGN(BasicNetworkDelegate);
 };
 
+bool IsCurrentVersion(const base::FilePath& version_filepath) {
+  if (!base::PathExists(version_filepath))
+    return false;
+  base::File version_file(version_filepath,
+                          base::File::FLAG_OPEN | base::File::FLAG_READ);
+  VersionHeader version;
+  int bytes_read =
+      version_file.Read(0, reinterpret_cast<char*>(&version), sizeof(version));
+  if (bytes_read != sizeof(version)) {
+    DLOG(WARNING) << "Cannot read from version file.";
+    return false;
+  }
+  return version.version == kDiskCacheVersion;
+}
+
+// TODO(xunjieli): Handle failures.
+void InitializeCacheDiskCacheDirectory(const base::FilePath& dir) {
+  // Checks version file and remove old versions.
+  base::FilePath version_filepath = dir.Append("version");
+  if (IsCurrentVersion(version_filepath)) {
+    // The version is up-to-date, so there is nothing to do.
+    return;
+  }
+  if (!(base::DeleteFile(dir, true) && base::CreateDirectory(dir))) {

[mmenke, 2016/03/31 15:39:07]

Think it's worth a comment that DeleteFile returns true if the directory does
not exist, since we're strongly depending on that unexpected behavior here.

[xunjieli, 2016/03/31 18:45:58]

Done.

+    DLOG(WARNING) << "Cannot purge directory.";
+    return;
+  }
+  base::FilePath new_version = dir.Append("version");

[mmenke, 2016/03/31 15:39:07]

This is the same as version_filepath.  Just using that seems less regression
prone.

[xunjieli, 2016/03/31 18:45:58]

Done.

+  base::File new_version_file(
+      new_version, base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+
+  if (!new_version_file.IsValid()) {
+    DLOG(WARNING) << "Cannot create a version file.";
+    return;
+  }
+
+  DCHECK(new_version_file.created());
+  VersionHeader version;
+  version.version = kDiskCacheVersion;
+  int bytes_written = new_version_file.Write(
+      0, reinterpret_cast<char*>(&version), sizeof(version));
+  if (bytes_written != sizeof(version)) {
+    DLOG(WARNING) << "Cannot write to version file.";
+    return;
+  }
+  base::FilePath prefs_dir = dir.Append(FILE_PATH_LITERAL("prefs"));
+  if (!base::CreateDirectory(prefs_dir)) {
+    DLOG(WARNING) << "Cannot create prefs directory";
+    return;
+  }
+}
+
 }  // namespace
 
 namespace cronet {
@@ -434,13 +493,18 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
   context_builder.set_proxy_service(
       net::ProxyService::CreateWithoutProxyResolver(
           std::move(proxy_config_service_), net_log_.get()));
+
   config->ConfigureURLRequestContextBuilder(&context_builder, net_log_.get(),
                                             GetFileThread()->task_runner());
 
   // Set up pref file if storage path is specified.
   if (!config->storage_path.empty()) {
-    base::FilePath filepath(config->storage_path);
-    filepath = filepath.Append(FILE_PATH_LITERAL("local_prefs.json"));
+    base::FilePath storage_path(config->storage_path);
+    // Make sure disk cache directory has correct version.
+    InitializeCacheDiskCacheDirectory(storage_path);

[mmenke, 2016/03/31 15:39:07]

We're doing file IO on the network thread.  This doesn't trigger any DCHECKs? 
We don't actually get anything at all from not doing on the network thread,
since we're blocked on waiting for this to complete anyways, just wondering
about the checks.

[xunjieli, 2016/03/31 18:45:58]

No checks are triggered when I run the tests locally with a debug build. I
looked through the code. It seems that File I/O is permitted on thread started
with base::MessageLoop::TYPE_IO option. The network thread has this option.

in base/files/file_posix.h:
ThreadRestrictions::AssertIOAllowed
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_po...

+    base::FilePath filepath =
+        storage_path.Append(FILE_PATH_LITERAL("prefs"))
+            .Append(FILE_PATH_LITERAL("local_prefs.json"));
     json_pref_store_ = new JsonPrefStore(
         filepath, GetFileThread()->task_runner(), scoped_ptr<PrefFilter>());
     context_builder.SetFileTaskRunner(GetFileThread()->task_runner());