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

content/browser/dom_storage/dom_storage_area.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/dom_storage/dom_storage_area.h"
 
 #include <inttypes.h>
 
 #include <algorithm>
 #include <cctype>  // for std::isalnum
@@ skipping 177 matching lines @@
                              base::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_ &&
       (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;
 }
 
 bool DOMStorageArea::RemoveItem(const base::string16& key,
                                 base::string16* old_value) {
   if (is_shutdown_)
     return false;
   InitialImportIfNeeded();
   if (!map_->HasOneRef())
@@ skipping 111 matching lines @@
                            kPerStorageAreaOverQuotaAllowance);
 
   // Recreate the database object, this frees up the open sqlite connection
   // and its page cache.
   backing_->Reset();
 }
 
 void DOMStorageArea::Shutdown() {
   DCHECK(!is_shutdown_);
   is_shutdown_ = true;
+
+  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;
-  if (!backing_)
-    return;
 
   bool success = task_runner_->PostShutdownBlockingTask(
       FROM_HERE,
       DOMStorageTaskRunner::COMMIT_SEQUENCE,
       base::Bind(&DOMStorageArea::ShutdownInCommitSequence, this));
   DCHECK(success);
 }
 
 void DOMStorageArea::OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd) {
   DCHECK(task_runner_->IsRunningOnPrimarySequence());
@@ skipping 77 matching lines @@
   DCHECK(!is_shutdown_);
   if (!commit_batch_) {
     commit_batch_.reset(new CommitBatch());
     BrowserThread::PostAfterStartupTask(
         FROM_HERE, task_runner_,
         base::Bind(&DOMStorageArea::StartCommitTimer, this));
   }
   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;
 
   // Start a timer to commit any changes that accrue in the batch, but only if
   // no commits are currently in flight. In that case the timer will be
   // started after the commits have happened.
   if (commit_batches_in_flight_)
     return;
 
@@ skipping 26 matching lines @@
 
   PostCommitTask();
 }
 
 void DOMStorageArea::PostCommitTask() {
   if (is_shutdown_ || !commit_batch_)
     return;
 
   DCHECK(backing_.get());
 
+  PopulateCommitBatchValues();
   commit_rate_limiter_.add_samples(1);
   data_rate_limiter_.add_samples(commit_batch_->GetDataSize());
 
   // This method executes on the primary sequence, we schedule
   // a task for immediate execution on the commit sequence.
   DCHECK(task_runner_->IsRunningOnPrimarySequence());
   bool success = task_runner_->PostShutdownBlockingTask(
       FROM_HERE,
       DOMStorageTaskRunner::COMMIT_SEQUENCE,
       base::Bind(&DOMStorageArea::CommitChanges, this,
@@ skipping 38 matching lines @@
         commit_batch_->clear_all_first,
         commit_batch_->changed_values);
     DCHECK(success);
   }
   commit_batch_.reset();
   backing_.reset();
   session_storage_backing_ = NULL;
 }
 
 }  // namespace content