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 127221)
+++ webkit/dom_storage/dom_storage_area.cc	(working copy)
@@ -16,6 +16,23 @@
 
 namespace dom_storage {
 
+struct DomStorageArea::CommitBatch {
+  bool clear_all_first;
+  ValuesMap changed_values;
+  CommitBatch() : clear_all_first(false) {}
+};
+
+// static
+const FilePath::CharType DomStorageArea::kDatabaseFileExtension[] =
+    FILE_PATH_LITERAL(".localstorage");
+
+// static
+FilePath DomStorageArea::DatabaseFileNameFromOrigin(const GURL& origin) {
+  std::string filename = fileapi::GetOriginIdentifierFromURL(origin);
+  return FilePath().Append(kDatabaseFileExtension).
+      InsertBeforeExtensionASCII(filename);
+}
+
 DomStorageArea::DomStorageArea(
     int64 namespace_id, const GURL& origin,
     const FilePath& directory, DomStorageTaskRunner* task_runner)
@@ -24,50 +41,35 @@
       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);
 }
@@ -75,31 +77,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 = GetCommitBatch();
+    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 = GetCommitBatch();
+    commit_batch->changed_values[key] = NullableString16(true);
   }
   return success;
 }
 
 bool DomStorageArea::Clear() {
+  if (is_shutdown_)
+    return false;
   InitialImportIfNeeded();
   if (map_->Length() == 0)
     return false;
@@ -107,15 +114,16 @@
   map_ = new DomStorageMap(kPerAreaQuota);
 
   if (backing_.get()) {
-    changed_values_.clear();
-    clear_all_next_commit_ = true;
-    ScheduleCommitChanges();
+    CommitBatch* commit_batch = GetCommitBatch();
+    commit_batch->clear_all_first = true;
+    commit_batch->changed_values.clear();
   }
 
   return true;
 }
 
 DomStorageArea* DomStorageArea::ShallowCopy(int64 destination_namespace_id) {
+  DCHECK(!is_shutdown_);
   DCHECK_NE(kLocalStorageNamespaceId, namespace_id_);
   DCHECK_NE(kLocalStorageNamespaceId, destination_namespace_id);
   // SessionNamespaces aren't backed by files on disk.
@@ -127,8 +135,21 @@
   return copy;
 }
 
+void DomStorageArea::Shutdown() {
+  DCHECK(!is_shutdown_);
+  is_shutdown_ = true;
+
+  // If there are changes for which a commit is not yet in flight,
+  // we post a task now to flush them out.
+  if (!commit_batch_.get())
+    return;
+  bool success = task_runner_->PostShutdownBlockingCommitTask(
+      FROM_HERE, base::Bind(&DomStorageArea::CommitShutdownChanges, this));
+  DCHECK(success);
+}
+
 void DomStorageArea::InitialImportIfNeeded() {
-  if (initial_import_done_)
+  if (is_initial_import_done_)
     return;
 
   DCHECK_EQ(kLocalStorageNamespaceId, namespace_id_);
@@ -137,38 +158,73 @@
   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::GetCommitBatch() {

[benm (inactive), 2012/03/19 14:22:53]

As this method is more than a getter (it may kick off the timer to commit
changes) I think the name should reflect that extra behavior. How about
CommitBatchIfNeeded or something like that?

[michaeln, 2012/03/19 16:26:52]

maybe StartCommitBatchIfNeeded()?

[benm (inactive), 2012/03/19 17:08:45]

sg, or maybe Schedule instead of Start, you decide :)

+  DCHECK(is_shutdown_);

[benm (inactive), 2012/03/19 14:22:53]

!is_shutdown?

[michaeln, 2012/03/19 16:04:42]

Done.

+  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(1));

[benm (inactive), 2012/03/19 14:22:53]

might be worth putting a constant in dom_storage_types for the time delta as
we're using it in a couple of places now.

[michaeln, 2012/03/19 16:04:42]

Done.

+    }
+  }
+  return commit_batch_.get();
+}
+
+void DomStorageArea::OnCommitTimer() {
   DCHECK_EQ(kLocalStorageNamespaceId, namespace_id_);
   DCHECK(backing_.get());
-  DCHECK(clear_all_next_commit_ || !changed_values_.empty());
-  DCHECK(task_runner_.get());
-
-  if (commit_in_flight_)
+  DCHECK(commit_batch_.get());
+  DCHECK(!in_flight_commit_batch_.get());
+  if (is_shutdown_)
     return;
 
-  commit_in_flight_ = task_runner_->PostDelayedTask(
-      FROM_HERE, base::Bind(&DomStorageArea::CommitChanges, this),
-      base::TimeDelta::FromSeconds(1));
-  DCHECK(commit_in_flight_);
+  // This method executes on the 'read' sequence, we schedule
+  // a task for immediate exeuction on the 'write' sequence.

[benm (inactive), 2012/03/19 14:22:53]

execution

[michaeln, 2012/03/19 16:04:42]

Done.

+  in_flight_commit_batch_ = commit_batch_.Pass();
+  bool success = task_runner_->PostShutdownBlockingCommitTask(
+      FROM_HERE, base::Bind(&DomStorageArea::CommitChanges, this));
+  DCHECK(success);
 }
 
 void DomStorageArea::CommitChanges() {
-  DCHECK(backing_.get());
-  if (backing_->CommitChanges(clear_all_next_commit_, changed_values_)) {
-    clear_all_next_commit_ = false;
-    changed_values_.clear();
+  // This method executes on the 'write' sequence.

[benm (inactive), 2012/03/19 14:22:53]

Can we DCHECK that this is the case?

[michaeln, 2012/03/19 16:04:42]

I wish we could, but right now there is no way to assert that since
SequencedWorkerPool doesn't support it yet.

+  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::OnCommitComplete() {
+  in_flight_commit_batch_.reset();
+  if (commit_batch_.get() && !is_shutdown_) {
+    // More changes have accrued, restart the timer.
+    task_runner_->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&DomStorageArea::OnCommitTimer, this),
+        base::TimeDelta::FromSeconds(1));
   }
-  commit_in_flight_ = false;
 }
 
-// static
-FilePath DomStorageArea::DatabaseFileNameFromOrigin(const GURL& origin) {
-  std::string filename = fileapi::GetOriginIdentifierFromURL(origin)
-      + ".localstorage";
-  return FilePath().AppendASCII(filename);
+void DomStorageArea::CommitShutdownChanges() {
+  // This method executes on the 'write' sequence.
+  DCHECK(commit_batch_.get());
+  bool success = backing_->CommitChanges(
+      commit_batch_->clear_all_first,
+      commit_batch_->changed_values);
+  DCHECK(success);
 }
 
 }  // namespace dom_storage