DOM Storage: Populate commit batch values lazily to avoid memory bloat

content/browser/dom_storage/dom_storage_area.cc

Index: content/browser/dom_storage/dom_storage_area.cc
diff --git a/content/browser/dom_storage/dom_storage_area.cc b/content/browser/dom_storage/dom_storage_area.cc
index f623fdcee0efcc1c91f0d6112b49a8b98babad79..426f756b672753e04b015c37b7c2e6f763c53003 100644
--- a/content/browser/dom_storage/dom_storage_area.cc
+++ b/content/browser/dom_storage/dom_storage_area.cc
@@ -195,6 +195,7 @@ bool DOMStorageArea::SetItem(const base::string16& key,
   if (success && backing_ &&
       (old_value->is_null() || old_value->string() != value)) {
     CommitBatch* commit_batch = CreateCommitBatchIfNeeded();
+    // Values are populated later to avoid holding duplicate memory.
     commit_batch->changed_values[key] = base::NullableString16(value, false);

[michaeln, 2016/09/02 22:39:57]

looks like you probably meant to put a non-null "" empty string value or a null
NullableString16() here

[jsbell, 2016/09/02 22:54:20]

Oh, duh. Lost that at some point. :P

Done.

   }
   return success;
@@ -327,9 +328,15 @@ void DOMStorageArea::PurgeMemory() {
 void DOMStorageArea::Shutdown() {
   DCHECK(!is_shutdown_);
   is_shutdown_ = true;
-  map_ = NULL;
-  if (!backing_)
+
+  if (!backing_) {
+    map_ = NULL;
     return;
+  }
+
+  if (commit_batch_)

[michaeln, 2016/09/02 22:39:57]

Juggling where to set map_ null is a little awkward. Not sure this suggestion is
any better but I think you could assert(backing_) if commit_batch_ is non null
and put the test up before the early return for no backing. Nothing can be
committed without a backing store.

if (commit_batch_) {
  DCHECK(backing_);
  PopulateCommitBatchValues();
}

map_ = NULL;
if (!backing_)
  return;

[jsbell, 2016/09/02 22:54:20]

Yeah, I went through several iterations here. I do slightly prefer yours, so
going with that.

+    PopulateCommitBatchValues();
+  map_ = NULL;
 
   bool success = task_runner_->PostShutdownBlockingTask(
       FROM_HERE,
@@ -427,6 +434,12 @@ DOMStorageArea::CommitBatch* DOMStorageArea::CreateCommitBatchIfNeeded() {
   return commit_batch_.get();
 }
 
+void DOMStorageArea::PopulateCommitBatchValues() {
+  DCHECK(task_runner_->IsRunningOnPrimarySequence());
+  for (auto& key_value : commit_batch_->changed_values)
+    key_value.second = map_->GetItem(key_value.first);
+}
+
 void DOMStorageArea::StartCommitTimer() {
   if (is_shutdown_ || !commit_batch_)
     return;
@@ -473,6 +486,7 @@ void DOMStorageArea::PostCommitTask() {
 
   DCHECK(backing_.get());
 
+  PopulateCommitBatchValues();
   commit_rate_limiter_.add_samples(1);
   data_rate_limiter_.add_samples(commit_batch_->GetDataSize());