Clear up session only storage on localstorage shutdown

content/browser/dom_storage/local_storage_context_mojo.cc

Index: content/browser/dom_storage/local_storage_context_mojo.cc
diff --git a/content/browser/dom_storage/local_storage_context_mojo.cc b/content/browser/dom_storage/local_storage_context_mojo.cc
index ddc0ef8323938f5d4c310ebce7e034d3eee53070..6a9f0df8c3643ae9c51c5ef62ad5feddcdd8ab4b 100644
--- a/content/browser/dom_storage/local_storage_context_mojo.cc
+++ b/content/browser/dom_storage/local_storage_context_mojo.cc
@@ -19,6 +19,7 @@
 #include "services/file/public/interfaces/constants.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "sql/connection.h"
+#include "storage/browser/quota/special_storage_policy.h"
 #include "third_party/leveldatabase/env_chromium.h"
 
 namespace content {
@@ -219,15 +220,15 @@ LocalStorageContextMojo::LocalStorageContextMojo(
     service_manager::Connector* connector,
     scoped_refptr<DOMStorageTaskRunner> task_runner,
     const base::FilePath& old_localstorage_path,
-    const base::FilePath& subdirectory)
-    : connector_(connector),
+    const base::FilePath& subdirectory,
+    storage::SpecialStoragePolicy* special_storage_policy)
+    : connector_(connector ? connector->Clone() : nullptr),
       subdirectory_(subdirectory),
+      special_storage_policy_(special_storage_policy),
       task_runner_(std::move(task_runner)),
       old_localstorage_path_(old_localstorage_path),
       weak_ptr_factory_(this) {}
 
-LocalStorageContextMojo::~LocalStorageContextMojo() {}
-
 void LocalStorageContextMojo::OpenLocalStorage(
     const url::Origin& origin,
     mojom::LevelDBWrapperRequest request) {
@@ -245,8 +246,8 @@ void LocalStorageContextMojo::GetStorageUsage(
 
 void LocalStorageContextMojo::DeleteStorage(const url::Origin& origin) {
   if (connection_state_ != CONNECTION_FINISHED) {
-    RunWhenConnected(base::BindOnce(&LocalStorageContextMojo::DeleteStorage,
-                                    weak_ptr_factory_.GetWeakPtr(), origin));
+    RunWhenConnected(
+        base::BindOnce(&LocalStorageContextMojo::DeleteStorage, this, origin));
     return;
   }
 
@@ -260,8 +261,8 @@ void LocalStorageContextMojo::DeleteStorage(const url::Origin& origin) {
 void LocalStorageContextMojo::DeleteStorageForPhysicalOrigin(
     const url::Origin& origin) {
   GetStorageUsage(base::BindOnce(
-      &LocalStorageContextMojo::OnGotStorageUsageForDeletePhysicalOrigin,
-      weak_ptr_factory_.GetWeakPtr(), origin));
+      &LocalStorageContextMojo::OnGotStorageUsageForDeletePhysicalOrigin, this,
+      origin));
 }
 
 void LocalStorageContextMojo::Flush() {
@@ -274,6 +275,47 @@ void LocalStorageContextMojo::Flush() {
     it.second->level_db_wrapper()->ScheduleImmediateCommit();
 }
 
+void LocalStorageContextMojo::Shutdown() {
+  DCHECK(!is_shutdown_);
+  DCHECK_NE(connection_state_, CONNECTION_SHUTDOWN);
+
+  // Nothing to do if no connection to the database was ever made.
+  if (connection_state_ == NO_CONNECTION) {
+    connection_state_ = CONNECTION_SHUTDOWN;
+    is_shutdown_ = true;
+    return;
+  }
+
+  if (connection_state_ != CONNECTION_FINISHED) {

[michaeln, 2017/05/03 21:01:29]

Similar to the NO_CONNECTION case, maybe we don't need to go thru the shutdown
sequence here either? If we're shutting down prior to being connected, either
the connection was taking forever to happen or the browsing session is super
short, both are exceptional cases in one way or another. I'd vote to try to do
less here and slam the connection state SHUTDOWN.

 670   is_shutdown_ = true;
 671   file_system_.reset();
 672   directory_.reset();
 673   leveldb_service_.reset();
 674   level_db_wrappers_.clear();
 675   database_.reset();
 676   connector_.reset();

Would it work to just pull the plug like that?

[Marijn Kruisselbrink, 2017/05/04 23:03:46]

Yeah, agreed that it makes sense to just immediately self-destruct when no
connection has been made yet. Incidentally I think that might also fix some
other (unrelated) issues I was having in a different CL...

+    RunWhenConnected(base::BindOnce(&LocalStorageContextMojo::Shutdown, this));
+    return;
+  }
+
+  connection_state_ = CONNECTION_SHUTDOWN;
+
+  // Flush any uncommitted data.
+  for (const auto& it : level_db_wrappers_)
+    it.second->level_db_wrapper()->ScheduleImmediateCommit();
+
+  // Respect the content policy settings about what to
+  // keep and what to discard.
+  if (force_keep_session_state_) {
+    OnShutdownCommitComplete(leveldb::mojom::DatabaseError::OK);
+    return;  // Keep everything.
+  }
+
+  bool has_session_only_origins =
+      special_storage_policy_.get() &&
+      special_storage_policy_->HasSessionOnlyOrigins();
+
+  if (has_session_only_origins) {
+    RetrieveStorageUsage(base::BindOnce(
+        &LocalStorageContextMojo::OnGotStorageUsageForShutdown, this));
+  } else {
+    OnShutdownCommitComplete(leveldb::mojom::DatabaseError::OK);
+  }
+}
+
 void LocalStorageContextMojo::PurgeMemory() {
   for (const auto& it : level_db_wrappers_)
     it.second->level_db_wrapper()->PurgeMemory();
@@ -302,7 +344,14 @@ std::vector<uint8_t> LocalStorageContextMojo::MigrateString(
   return result;
 }
 
+LocalStorageContextMojo::~LocalStorageContextMojo() {
+  DCHECK(is_shutdown_);
+  DCHECK_EQ(connection_state_, CONNECTION_SHUTDOWN);
+}
+
 void LocalStorageContextMojo::RunWhenConnected(base::OnceClosure callback) {
+  DCHECK_NE(connection_state_, CONNECTION_SHUTDOWN);
+
   // If we don't have a filesystem_connection_, we'll need to establish one.
   if (connection_state_ == NO_CONNECTION) {
     connection_state_ = CONNECTION_IN_PROGRESS;
@@ -584,4 +633,47 @@ void LocalStorageContextMojo::OnGotStorageUsageForDeletePhysicalOrigin(
   DeleteStorage(origin);
 }
 
+void LocalStorageContextMojo::OnGotStorageUsageForShutdown(
+    std::vector<LocalStorageUsageInfo> usage) {
+  std::vector<leveldb::mojom::BatchedOperationPtr> operations;
+  for (const auto& info : usage) {
+    if (special_storage_policy_->IsStorageProtected(info.origin))
+      continue;
+    if (!special_storage_policy_->IsStorageSessionOnly(info.origin))
+      continue;
+
+    url::Origin origin(info.origin);

[michaeln, 2017/05/03 21:01:29]

i see you've copied the behavior for the sqlite impl, not for this cl, but i'm
not sure this properly accounts for suborigins?

[Marijn Kruisselbrink, 2017/05/04 23:03:46]

Not sure.. but I imagine properly accounting for suborigins should happen inside
SpecialStoragePolicy's IsStorageProtected and IsStorageSessionOnly methods,
after which this should automatically do the right thing.

+    leveldb::mojom::BatchedOperationPtr item =
+        leveldb::mojom::BatchedOperation::New();
+    item->type = leveldb::mojom::BatchOperationType::DELETE_PREFIXED_KEY;
+    item->key = leveldb::StdStringToUint8Vector(
+        kDataPrefix + origin.Serialize() + kOriginSeparator);
+    operations.push_back(std::move(item));
+
+    item = leveldb::mojom::BatchedOperation::New();
+    item->type = leveldb::mojom::BatchOperationType::DELETE_KEY;
+    item->key = CreateMetaDataKey(origin);
+    operations.push_back(std::move(item));
+  }
+
+  if (!operations.empty()) {
+    database_->Write(
+        std::move(operations),
+        base::Bind(&LocalStorageContextMojo::OnShutdownCommitComplete, this));
+  } else {
+    OnShutdownCommitComplete(leveldb::mojom::DatabaseError::OK);
+  }
+}
+
+void LocalStorageContextMojo::OnShutdownCommitComplete(
+    leveldb::mojom::DatabaseError error) {
+  is_shutdown_ = true;
+  file_system_.reset();
+  directory_.reset();
+  leveldb_service_.reset();
+  level_db_wrappers_.clear();
+  database_.reset();
+  connector_.reset();
+}
+
 }  // namespace content