[code caching] Fix issues with move semantics in GeneratedCodeCache

- Divides EnqueueAsPendingOperation into two methods, with a new
  TryBeginOperation method to check for any pending operations. This
  allows us to defer creating the PendingOperation until we know we
  need it. Otherwise, move semantics could potentially cause loss of
  data when we discard the PendingOperation. This worked before
  because the callbacks are copyable, and IOBuffer is ref-counted.

- Adds some std::moves for callbacks, and some trivial renaming for
  consistency.

- This is preparation for using mojo_base::BigBuffer for writing too.

Bug: chromium:992991
Change-Id: Ia8b16ba9403da2b95c2c9a57da93a9112fd62b17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1801757Reviewed-by: Mythri Alle <mythria@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696977}

content/browser/code_cache/generated_code_cache.cc

@@ -147,7 +147,9 @@ class GeneratedCodeCache::PendingOperation {
   const std::string& key() const { return key_; }
   const scoped_refptr<net::IOBufferWithSize> data() const { return data_; }
   ReadDataCallback ReleaseReadCallback() { return std::move(read_callback_); }
-  GetBackendCallback ReleaseCallback() { return std::move(callback_); }
+  GetBackendCallback ReleaseBackendCallback() {
+    return std::move(backend_callback_);
+  }
 
  private:
   PendingOperation(Operation op,
@@ -160,7 +162,7 @@ class GeneratedCodeCache::PendingOperation {
   const std::string key_;
   const scoped_refptr<net::IOBufferWithSize> data_;
   ReadDataCallback read_callback_;
-  GetBackendCallback callback_;
+  GetBackendCallback backend_callback_;
 };
 
 std::unique_ptr<GeneratedCodeCache::PendingOperation>
@@ -178,7 +180,7 @@ GeneratedCodeCache::PendingOperation::CreateFetchPendingOp(
     const ReadDataCallback& read_callback) {
   return base::WrapUnique(new PendingOperation(
       Operation::kFetch, std::move(key), scoped_refptr<net::IOBufferWithSize>(),
-      read_callback, GetBackendCallback()));
+      std::move(read_callback), GetBackendCallback()));
 }
 
 std::unique_ptr<GeneratedCodeCache::PendingOperation>
@@ -191,11 +193,11 @@ GeneratedCodeCache::PendingOperation::CreateDeletePendingOp(std::string key) {
 
 std::unique_ptr<GeneratedCodeCache::PendingOperation>
 GeneratedCodeCache::PendingOperation::CreateGetBackendPendingOp(
-    GetBackendCallback callback) {
+    GetBackendCallback backend_callback) {
   return base::WrapUnique(
       new PendingOperation(Operation::kGetBackend, std::string(),
                            scoped_refptr<net::IOBufferWithSize>(),
-                           ReadDataCallback(), std::move(callback)));
+                           ReadDataCallback(), std::move(backend_callback)));
 }
 
 GeneratedCodeCache::PendingOperation::PendingOperation(
@@ -203,12 +205,12 @@ GeneratedCodeCache::PendingOperation::PendingOperation(
     std::string key,
     scoped_refptr<net::IOBufferWithSize> buffer,
     const ReadDataCallback& read_callback,
-    GetBackendCallback callback)
+    GetBackendCallback backend_callback)
     : op_(op),
       key_(std::move(key)),
       data_(buffer),
-      read_callback_(read_callback),
-      callback_(std::move(callback)) {}
+      read_callback_(std::move(read_callback)),
+      backend_callback_(std::move(backend_callback)) {}
 
 GeneratedCodeCache::PendingOperation::~PendingOperation() = default;
 
@@ -264,9 +266,10 @@ void GeneratedCodeCache::WriteData(const GURL& url,
   std::string key = GetCacheKey(url, origin_lock);
   // If there is an in progress operation corresponding to this key. Enqueue it
   // so we can issue once the in-progress operation finishes.
-  if (EnqueueAsPendingOperation(
-          key, GeneratedCodeCache::PendingOperation::CreateWritePendingOp(
-                   key, buffer))) {
+  if (!TryBeginOperation(key)) {
+    EnqueueAsPendingOperation(
+        key, GeneratedCodeCache::PendingOperation::CreateWritePendingOp(
+                 key, buffer));
     return;
   }
 
@@ -295,9 +298,10 @@ void GeneratedCodeCache::FetchEntry(const GURL& url,
   std::string key = GetCacheKey(url, origin_lock);
   // If there is an in progress operation corresponding to this key. Enqueue it
   // so we can issue once the in-progress operation finishes.
-  if (EnqueueAsPendingOperation(
-          key, GeneratedCodeCache::PendingOperation::CreateFetchPendingOp(
-                   key, read_data_callback))) {
+  if (!TryBeginOperation(key)) {
+    EnqueueAsPendingOperation(
+        key, GeneratedCodeCache::PendingOperation::CreateFetchPendingOp(
+                 key, read_data_callback));
     return;
   }
 
@@ -392,7 +396,7 @@ void GeneratedCodeCache::IssueOperation(PendingOperation* op) {
       DeleteEntryImpl(op->key());
       break;
     case kGetBackend:
-      DoPendingGetBackend(op->ReleaseCallback());
+      DoPendingGetBackend(op->ReleaseBackendCallback());
       break;
   }
 }
@@ -593,18 +597,22 @@ void GeneratedCodeCache::IssueQueuedOperationForEntry(const std::string& key) {
   IssueOperation(op.get());
 }
 
-bool GeneratedCodeCache::EnqueueAsPendingOperation(
-    const std::string& key,
-    std::unique_ptr<PendingOperation> op) {
+bool GeneratedCodeCache::TryBeginOperation(const std::string& key) {
   auto it = active_entries_map_.find(key);
-  if (it != active_entries_map_.end()) {
-    it->second.emplace(std::move(op));
-    return true;
-  }
+  if (it != active_entries_map_.end())
+    return false;
 
-  // Create a entry to indicate there is a in-progress operation for this key.
+  // Create an entry to indicate there is a in-progress operation for this key.
   active_entries_map_[key] = base::queue<std::unique_ptr<PendingOperation>>();
-  return false;
+  return true;
+}
+
+void GeneratedCodeCache::EnqueueAsPendingOperation(
+    const std::string& key,
+    std::unique_ptr<PendingOperation> op) {
+  auto it = active_entries_map_.find(key);
+  DCHECK(it != active_entries_map_.end());
+  it->second.emplace(std::move(op));
 }
 
 void GeneratedCodeCache::DoPendingGetBackend(GetBackendCallback user_callback) {

content/browser/code_cache/generated_code_cache.h

@@ -164,9 +164,12 @@ class CONTENT_EXPORT GeneratedCodeCache {
 
   // Issues the queued operation at the front of the queue for the given |key|.
   void IssueQueuedOperationForEntry(const std::string& key);
-  // Enqueues into the list if there is an in-progress operation. Otherwise
-  // creates an entry to indicate there is an active operation.
-  bool EnqueueAsPendingOperation(const std::string& key,
+  // Checks for in-progress operations. If there are none, marks the key as
+  // in-progress and returns true. Otherwise returns false.
+  bool TryBeginOperation(const std::string& key);
+  // Enqueues the operation as pending for the key. This should only be called
+  // if TryBeginOperation returned false.
+  void EnqueueAsPendingOperation(const std::string& key,
                                  std::unique_ptr<PendingOperation> op);
   void IssueOperation(PendingOperation* op);