DomStorage commit task sequencing.

webkit/dom_storage/dom_storage_area.cc

Index: webkit/dom_storage/dom_storage_area.cc
===================================================================
--- webkit/dom_storage/dom_storage_area.cc	(revision 127736)
+++ webkit/dom_storage/dom_storage_area.cc	(working copy)
@@ -16,6 +16,14 @@
 
 namespace dom_storage {
 
+static const int kCommitTimerSeconds = 1;
+
+DomStorageArea::CommitBatch::CommitBatch()
+  : clear_all_first(false) {
+}
+DomStorageArea::CommitBatch::~CommitBatch() {}
+
+
 // static
 const FilePath::CharType DomStorageArea::kDatabaseFileExtension[] =
     FILE_PATH_LITERAL(".localstorage");
@@ -37,51 +45,35 @@
       directory_(directory),
       task_runner_(task_runner),
       map_(new DomStorageMap(kPerAreaQuota)),
-      backing_(NULL),
-      initial_import_done_(false),
-      clear_all_next_commit_(false),
-      commit_in_flight_(false) {
-
+      is_initial_import_done_(true),
+      is_shutdown_(false) {
   if (namespace_id == kLocalStorageNamespaceId && !directory.empty()) {
     FilePath path = directory.Append(DatabaseFileNameFromOrigin(origin_));
     backing_.reset(new DomStorageDatabase(path));
-  } else {
-    // Not a local storage area or no directory specified for backing
-    // database, (i.e. it's an incognito profile).
-    initial_import_done_ = true;
+    is_initial_import_done_ = false;
   }
 }
 
 DomStorageArea::~DomStorageArea() {
-  if (clear_all_next_commit_ || !changed_values_.empty()) {
-    // Still some data left that was not committed to disk, try now.
-    // We do this regardless of whether we think a commit is in flight
-    // as there is no guarantee that that commit will actually get
-    // processed. For example the task_runner_'s message loop could
-    // unexpectedly quit before the delayed task is fired and leave the
-    // commit_in_flight_ flag set. But there's no way for us to determine
-    // that has happened so force a commit now.
-
-    CommitChanges();
-
-    // TODO(benm): It's possible that the commit failed, and in
-    // that case we're going to lose data. Integrate with UMA
-    // to gather stats about how often this actually happens,
-    // so that we can figure out a contingency plan.
-  }
 }
 
 unsigned DomStorageArea::Length() {
+  if (is_shutdown_)
+    return 0;
   InitialImportIfNeeded();
   return map_->Length();
 }
 
 NullableString16 DomStorageArea::Key(unsigned index) {
+  if (is_shutdown_)
+    return NullableString16(true);
   InitialImportIfNeeded();
   return map_->Key(index);
 }
 
 NullableString16 DomStorageArea::GetItem(const string16& key) {
+  if (is_shutdown_)
+    return NullableString16(true);
   InitialImportIfNeeded();
   return map_->GetItem(key);
 }
@@ -89,31 +81,36 @@
 bool DomStorageArea::SetItem(const string16& key,
                              const string16& value,
                              NullableString16* old_value) {
+  if (is_shutdown_)
+    return false;
   InitialImportIfNeeded();
-
   if (!map_->HasOneRef())
     map_ = map_->DeepCopy();
   bool success = map_->SetItem(key, value, old_value);
   if (success && backing_.get()) {
-    changed_values_[key] = NullableString16(value, false);
-    ScheduleCommitChanges();
+    CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
+    commit_batch->changed_values[key] = NullableString16(value, false);
   }
   return success;
 }
 
 bool DomStorageArea::RemoveItem(const string16& key, string16* old_value) {
+  if (is_shutdown_)
+    return false;
   InitialImportIfNeeded();
   if (!map_->HasOneRef())
     map_ = map_->DeepCopy();
   bool success = map_->RemoveItem(key, old_value);
   if (success && backing_.get()) {
-    changed_values_[key] = NullableString16(true);
-    ScheduleCommitChanges();
+    CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
+    commit_batch->changed_values[key] = NullableString16(true);
   }
   return success;
 }
 
 bool DomStorageArea::Clear() {
+  if (is_shutdown_)
+    return false;
   InitialImportIfNeeded();
   if (map_->Length() == 0)
     return false;
@@ -121,9 +118,9 @@
   map_ = new DomStorageMap(kPerAreaQuota);
 
   if (backing_.get()) {
-    changed_values_.clear();
-    clear_all_next_commit_ = true;
-    ScheduleCommitChanges();
+    CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
+    commit_batch->clear_all_first = true;
+    commit_batch->changed_values.clear();
   }
 
   return true;
@@ -132,17 +129,31 @@
 DomStorageArea* DomStorageArea::ShallowCopy(int64 destination_namespace_id) {
   DCHECK_NE(kLocalStorageNamespaceId, namespace_id_);
   DCHECK_NE(kLocalStorageNamespaceId, destination_namespace_id);
-  // SessionNamespaces aren't backed by files on disk.
-  DCHECK(!backing_.get());
+  DCHECK(!backing_.get());  // SessionNamespaces aren't stored on disk.
 
   DomStorageArea* copy = new DomStorageArea(destination_namespace_id, origin_,
                                             FilePath(), task_runner_);
   copy->map_ = map_;
+  copy->is_shutdown_ = is_shutdown_;
   return copy;
 }
 
+void DomStorageArea::Shutdown() {
+  DCHECK(!is_shutdown_);
+  is_shutdown_ = true;
+  map_ = NULL;
+  if (!backing_.get())
+    return;
+
+  bool success = task_runner_->PostShutdownBlockingTask(
+      FROM_HERE,
+      DomStorageTaskRunner::COMMIT_SEQUENCE,
+      base::Bind(&DomStorageArea::ShutdownInCommitSequence, this));
+  DCHECK(success);
+}
+
 void DomStorageArea::InitialImportIfNeeded() {
-  if (initial_import_done_)
+  if (is_initial_import_done_)
     return;
 
   DCHECK_EQ(kLocalStorageNamespaceId, namespace_id_);
@@ -151,31 +162,85 @@
   ValuesMap initial_values;
   backing_->ReadAllValues(&initial_values);
   map_->SwapValues(&initial_values);
-  initial_import_done_ = true;
+  is_initial_import_done_ = true;
 }
 
-void DomStorageArea::ScheduleCommitChanges() {
+DomStorageArea::CommitBatch* DomStorageArea::CreateCommitBatchIfNeeded() {
+  DCHECK(!is_shutdown_);
+  if (!commit_batch_.get()) {
+    commit_batch_.reset(new CommitBatch());
+
+    // Start a timer to commit any changes that accrue in the batch,
+    // but only if a commit is not currently in flight. In that case
+    // the timer will be started after the current commit has happened.
+    if (!in_flight_commit_batch_.get()) {
+      task_runner_->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&DomStorageArea::OnCommitTimer, this),
+          base::TimeDelta::FromSeconds(kCommitTimerSeconds));
+    }
+  }
+  return commit_batch_.get();
+}
+
+void DomStorageArea::OnCommitTimer() {
   DCHECK_EQ(kLocalStorageNamespaceId, namespace_id_);
+  if (is_shutdown_)
+    return;
+
   DCHECK(backing_.get());
-  DCHECK(clear_all_next_commit_ || !changed_values_.empty());
-  DCHECK(task_runner_.get());
+  DCHECK(commit_batch_.get());
+  DCHECK(!in_flight_commit_batch_.get());
 
-  if (commit_in_flight_)
-    return;
+  // This method executes on the primary sequence, we schedule
+  // a task for immediate execution on the commit sequence.
+  in_flight_commit_batch_ = commit_batch_.Pass();
+  bool success = task_runner_->PostShutdownBlockingTask(
+      FROM_HERE,
+      DomStorageTaskRunner::COMMIT_SEQUENCE,
+      base::Bind(&DomStorageArea::CommitChanges, this));
+  DCHECK(success);
+}
 
-  commit_in_flight_ = task_runner_->PostDelayedTask(
-      FROM_HERE, base::Bind(&DomStorageArea::CommitChanges, this),
-      base::TimeDelta::FromSeconds(1));
-  DCHECK(commit_in_flight_);
+void DomStorageArea::CommitChanges() {
+  // This method executes on the commit sequence.
+  DCHECK(in_flight_commit_batch_.get());
+  bool success = backing_->CommitChanges(
+      in_flight_commit_batch_->clear_all_first,
+      in_flight_commit_batch_->changed_values);
+  DCHECK(success);  // TODO(michaeln): what if it fails?
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DomStorageArea::OnCommitComplete, this));
 }
 
-void DomStorageArea::CommitChanges() {
+void DomStorageArea::OnCommitComplete() {
+  // We're back on the primary sequence in this method.
+  if (is_shutdown_)
+    return;
+  in_flight_commit_batch_.reset();
+  if (commit_batch_.get()) {
+    // More changes have accrued, restart the timer.
+    task_runner_->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&DomStorageArea::OnCommitTimer, this),
+        base::TimeDelta::FromSeconds(kCommitTimerSeconds));
+  }
+}
+
+void DomStorageArea::ShutdownInCommitSequence() {
+  // This method executes on the commit sequence.
   DCHECK(backing_.get());
-  if (backing_->CommitChanges(clear_all_next_commit_, changed_values_)) {
-    clear_all_next_commit_ = false;
-    changed_values_.clear();
+  if (commit_batch_.get()) {
+    // Commit any changes that accrued prior to the timer firing.
+    bool success = backing_->CommitChanges(
+        commit_batch_->clear_all_first,
+        commit_batch_->changed_values);
+    DCHECK(success);
   }
-  commit_in_flight_ = false;
+  commit_batch_.reset();
+  in_flight_commit_batch_.reset();
+  backing_.reset();
 }
 
 }  // namespace dom_storage