[CacheStorage] Doom entries before closing if they're mid-write

CacheStorageCache dooms entries before closing them if their write operations
fail. This prevents them from being used in the future. In the event that
a ScopedEntry is held by a callback and the callback never gets to run, but
does get deleted, a partially written entry would be closed and will be
reusable.

This is a hypothetical problem. We're not actually sure this can happen since
the CacheStorageCache is kept alive by its manager during async operations. But
for safety, I've made a new smart pointer for disk_cache::Entry*'s that will
by default doom an entry before closing it unless WritingCompleted() has been
called on its deleter. This new smart pointer is used anywhere that Entry*
writing occurs in CacheStorage.

Bug: 617683
Change-Id: Ia5b83bd87b4997fea08cba013fc4701441db6f42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1600267
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659210}

content/browser/BUILD.gn

@@ -609,6 +609,7 @@ jumbo_source_set("browser") {
     "cache_storage/legacy/legacy_cache_storage.h",
     "cache_storage/legacy/legacy_cache_storage_cache.cc",
     "cache_storage/legacy/legacy_cache_storage_cache.h",
+    "cache_storage/scoped_writable_entry.h",
     "child_process_launcher.cc",
     "child_process_launcher.h",
     "child_process_launcher_helper.cc",

content/browser/cache_storage/cache_storage_blob_to_disk_cache.cc

@@ -30,7 +30,7 @@ CacheStorageBlobToDiskCache::CacheStorageBlobToDiskCache()
 CacheStorageBlobToDiskCache::~CacheStorageBlobToDiskCache() = default;
 
 void CacheStorageBlobToDiskCache::StreamBlobToCache(
-    disk_cache::ScopedEntryPtr entry,
+    ScopedWritableEntry entry,
     int disk_cache_body_index,
     blink::mojom::BlobPtr blob,
     uint64_t blob_size,

content/browser/cache_storage/cache_storage_blob_to_disk_cache.h

@@ -11,9 +11,9 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "content/browser/cache_storage/scoped_writable_entry.h"
 #include "content/common/content_export.h"
 #include "mojo/public/cpp/bindings/binding.h"
-#include "net/disk_cache/disk_cache.h"
 #include "services/network/public/cpp/net_adapters.h"
 #include "third_party/blink/public/mojom/blob/blob.mojom.h"
 
@@ -24,7 +24,7 @@ class CONTENT_EXPORT CacheStorageBlobToDiskCache
     : public blink::mojom::BlobReaderClient {
  public:
   using EntryAndBoolCallback =
-      base::OnceCallback<void(disk_cache::ScopedEntryPtr, bool)>;
+      base::OnceCallback<void(ScopedWritableEntry, bool)>;
 
   // The buffer size used for reading from blobs and writing to disk cache.
   static const int kBufferSize;
@@ -35,7 +35,7 @@ class CONTENT_EXPORT CacheStorageBlobToDiskCache
   // Writes the body of |blob| to |entry| with index
   // |disk_cache_body_index|. |entry| is passed to the callback once complete.
   // Only call this once per instantiation of CacheStorageBlobToDiskCache.
-  void StreamBlobToCache(disk_cache::ScopedEntryPtr entry,
+  void StreamBlobToCache(ScopedWritableEntry entry,
                          int disk_cache_body_index,
                          blink::mojom::BlobPtr blob,
                          uint64_t blob_size,
@@ -57,7 +57,7 @@ class CONTENT_EXPORT CacheStorageBlobToDiskCache
   void OnDataPipeReadable(MojoResult result);
 
   int cache_entry_offset_ = 0;
-  disk_cache::ScopedEntryPtr entry_;
+  ScopedWritableEntry entry_;
 
   int disk_cache_body_index_ = 0;
   EntryAndBoolCallback callback_;

content/browser/cache_storage/cache_storage_blob_to_disk_cache_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/test/null_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
+#include "content/browser/cache_storage/scoped_writable_entry.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
@@ -181,7 +182,7 @@ class CacheStorageBlobToDiskCacheTest : public testing::Test {
     return callback_called_ && callback_success_;
   }
 
-  void StreamCallback(disk_cache::ScopedEntryPtr entry_ptr, bool success) {
+  void StreamCallback(ScopedWritableEntry entry_ptr, bool success) {
     disk_cache_entry_ = std::move(entry_ptr);
     callback_success_ = success;
     callback_called_ = true;
@@ -194,7 +195,7 @@ class CacheStorageBlobToDiskCacheTest : public testing::Test {
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
   std::unique_ptr<storage::BlobDataHandle> blob_handle_;
   std::unique_ptr<disk_cache::Backend> cache_backend_;
-  disk_cache::ScopedEntryPtr disk_cache_entry_;
+  ScopedWritableEntry disk_cache_entry_;
   std::unique_ptr<TestCacheStorageBlobToDiskCache>
       cache_storage_blob_to_disk_cache_;
   std::string data_;

content/browser/cache_storage/cache_storage_cache_entry_handler.h

@@ -12,6 +12,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "content/browser/cache_storage/cache_storage_cache_handle.h"
+#include "content/browser/cache_storage/scoped_writable_entry.h"
 #include "content/common/content_export.h"
 #include "net/disk_cache/disk_cache.h"
 #include "storage/browser/blob/blob_data_builder.h"
@@ -51,7 +52,7 @@ struct PutContext {
 
   // Provided while writing to the cache.
   ErrorCallback callback;
-  disk_cache::ScopedEntryPtr cache_entry;
+  ScopedWritableEntry cache_entry;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PutContext);

content/browser/cache_storage/cache_storage_cache_unittest.cc

@@ -40,6 +40,8 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "crypto/symmetric_key.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "mojo/public/cpp/system/data_pipe.h"
 #include "mojo/public/cpp/system/data_pipe_drainer.h"
 #include "net/base/test_completion_callback.h"
 #include "net/disk_cache/disk_cache.h"
@@ -52,6 +54,7 @@
 #include "storage/browser/blob/blob_storage_context.h"
 #include "storage/browser/blob/blob_url_request_job_factory.h"
 #include "storage/browser/quota/quota_manager_proxy.h"
+#include "storage/browser/test/fake_blob.h"
 #include "storage/browser/test/mock_quota_manager_proxy.h"
 #include "storage/browser/test/mock_special_storage_policy.h"
 #include "storage/common/blob_storage/blob_handle.h"
@@ -94,6 +97,22 @@ void SizeCallback(base::RunLoop* run_loop,
     run_loop->Quit();
 }
 
+// A blob that never finishes writing to its pipe.
+class SlowBlob : public storage::FakeBlob {
+ public:
+  explicit SlowBlob(base::OnceClosure quit_closure)
+      : FakeBlob("foo"), quit_closure_(std::move(quit_closure)) {}
+
+  void ReadAll(mojo::ScopedDataPipeProducerHandle producer_handle,
+               blink::mojom::BlobReaderClientPtr client) override {
+    // Don't respond, forcing the consumer to wait forever.
+    std::move(quit_closure_).Run();
+  }
+
+ private:
+  base::OnceClosure quit_closure_;
+};
+
 // A disk_cache::Backend wrapper that can delay operations.
 class DelayableBackend : public disk_cache::Backend {
  public:
@@ -416,15 +435,14 @@ class CacheStorageCacheTest : public testing::Test {
     blob_storage_context_ = blob_storage_context->context();
 
     const bool is_incognito = MemoryOnly();
-    base::FilePath temp_dir_path;
     if (!is_incognito) {
       ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-      temp_dir_path = temp_dir_.GetPath();
+      temp_dir_path_ = temp_dir_.GetPath();
     }
 
     quota_policy_ = new MockSpecialStoragePolicy;
     mock_quota_manager_ = new MockQuotaManager(
-        is_incognito, temp_dir_path, base::ThreadTaskRunnerHandle::Get().get(),
+        is_incognito, temp_dir_path_, base::ThreadTaskRunnerHandle::Get().get(),
         quota_policy_.get());
     mock_quota_manager_->SetQuota(
         kOrigin, blink::mojom::StorageType::kTemporary, 1024 * 1024 * 100);
@@ -456,13 +474,13 @@ class CacheStorageCacheTest : public testing::Test {
     // CacheStorageCacheHandle reference counting.  A LegacyCacheStorage
     // must be present to be notified when a cache becomes unreferenced.
     mock_cache_storage_ = std::make_unique<MockLegacyCacheStorage>(
-        temp_dir_path, MemoryOnly(), base::ThreadTaskRunnerHandle::Get().get(),
+        temp_dir_path_, MemoryOnly(), base::ThreadTaskRunnerHandle::Get().get(),
         quota_manager_proxy_, blob_storage_context_->AsWeakPtr(),
         /* cache_storage_manager = */ nullptr, kOrigin,
         CacheStorageOwner::kCacheAPI);
 
     cache_ = std::make_unique<TestCacheStorageCache>(
-        kOrigin, kCacheName, temp_dir_path, mock_cache_storage_.get(),
+        kOrigin, kCacheName, temp_dir_path_, mock_cache_storage_.get(),
         quota_manager_proxy_, blob_storage_context->context()->AsWeakPtr());
     cache_->Init();
   }
@@ -842,6 +860,7 @@ class CacheStorageCacheTest : public testing::Test {
   storage::BlobStorageContext* blob_storage_context_;
   std::unique_ptr<MockLegacyCacheStorage> mock_cache_storage_;
 
+  base::FilePath temp_dir_path_;
   std::unique_ptr<TestCacheStorageCache> cache_;
 
   blink::mojom::FetchAPIRequestPtr body_request_;
@@ -2211,6 +2230,53 @@ TEST_P(CacheStorageCacheTestP, OpsFailOnClosedBackend) {
   VerifyAllOpsFail();
 }
 
+// Shutdown the cache in the middle of its writing the response body. Upon
+// restarting, that response shouldn't be available. See crbug.com/617683.
+TEST_P(CacheStorageCacheTestP, UnfinishedPutsShouldNotBeReusable) {
+  // Create a response with a blob that takes forever to write its bytes to the
+  // mojo pipe. Guaranteeing that the response isn't finished writing by the
+  // time we close the backend.
+  base::RunLoop run_loop;
+  auto blob = blink::mojom::SerializedBlob::New();
+  blob->uuid = "mock blob";
+  blob->size = 100;
+  mojo::MakeStrongBinding(std::make_unique<SlowBlob>(run_loop.QuitClosure()),
+                          MakeRequest(&blob->blob));
+  blink::mojom::FetchAPIResponsePtr response = CreateNoBodyResponse();
+  response->url_list = {kBodyUrl};
+  response->blob = std::move(blob);
+
+  blink::mojom::BatchOperationPtr operation =
+      blink::mojom::BatchOperation::New();
+  operation->operation_type = blink::mojom::OperationType::kPut;
+  operation->request = BackgroundFetchSettledFetch::CloneRequest(body_request_);
+  operation->response = std::move(response);
+  std::vector<blink::mojom::BatchOperationPtr> operations;
+  operations.emplace_back(std::move(operation));
+
+  // Start the put operation and let it run until the blob is supposed to write
+  // to its pipe.
+  cache_->BatchOperation(std::move(operations), /* trace_id = */ 0,
+                         base::DoNothing(), base::DoNothing());
+  run_loop.Run();
+
+  // Shut down the cache. Doing so causes the write to cease, and the entry
+  // should be erased.
+  cache_ = nullptr;
+  base::RunLoop().RunUntilIdle();
+
+  // Create a new Cache in the same space.
+  ChromeBlobStorageContext* blob_storage_context =
+      ChromeBlobStorageContext::GetFor(&browser_context_);
+  cache_ = std::make_unique<TestCacheStorageCache>(
+      kOrigin, kCacheName, temp_dir_path_, nullptr /* CacheStorage */,
+      quota_manager_proxy_, blob_storage_context->context()->AsWeakPtr());
+  cache_->Init();
+
+  // Now attempt to read the same response from the cache. It should fail.
+  EXPECT_FALSE(Match(body_request_));
+}
+
 TEST_P(CacheStorageCacheTestP, BlobReferenceDelaysClose) {
   // Create the backend and put something in it.
   EXPECT_TRUE(Put(body_request_, CreateBlobBodyResponse()));

content/browser/cache_storage/legacy/legacy_cache_storage_cache.h

@@ -19,6 +19,7 @@
 #include "base/optional.h"
 #include "content/browser/cache_storage/cache_storage_cache.h"
 #include "content/browser/cache_storage/cache_storage_handle.h"
+#include "content/browser/cache_storage/scoped_writable_entry.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "net/base/completion_once_callback.h"
 #include "net/base/io_buffer.h"
@@ -333,16 +334,19 @@ class CONTENT_EXPORT LegacyCacheStorageCache : public CacheStorageCache {
       int64_t trace_id,
       scoped_refptr<net::IOBuffer> buffer,
       int buf_len,
-      disk_cache::ScopedEntryPtr entry,
+      ScopedWritableEntry entry,
       std::unique_ptr<proto::CacheMetadata> headers);
   void WriteSideDataDidWrite(
       ErrorCallback callback,
-      disk_cache::ScopedEntryPtr entry,
+      ScopedWritableEntry entry,
       int expected_bytes,
       std::unique_ptr<content::proto::CacheResponse> response,
       int side_data_size_before_write,
       int64_t trace_id,
       int rv);
+  void WriteSideDataComplete(ErrorCallback callback,
+                             ScopedWritableEntry entry,
+                             blink::mojom::CacheStorageError error);
 
   // Puts the request and response object in the cache. The response body (if
   // present) is stored in the cache, but not the request body. Returns OK on
@@ -367,8 +371,10 @@ class CONTENT_EXPORT LegacyCacheStorageCache : public CacheStorageCache {
                            int disk_cache_body_index);
   void PutDidWriteBlobToCache(std::unique_ptr<PutContext> put_context,
                               BlobToDiskCacheIDMap::KeyType blob_to_cache_key,
-                              disk_cache::ScopedEntryPtr entry,
+                              ScopedWritableEntry entry,
                               bool success);
+  void PutComplete(std::unique_ptr<PutContext> put_context,
+                   blink::mojom::CacheStorageError error);
 
   // Asynchronously calculates the current cache size, notifies the quota
   // manager of any change from the last report, and sets cache_size_ to the new

content/browser/cache_storage/scoped_writable_entry.h

+// Copyright 2019 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.
+
+#ifndef CONTENT_BROWSER_CACHE_STORAGE_SCOPED_WRITABLE_ENTRY_H_
+#define CONTENT_BROWSER_CACHE_STORAGE_SCOPED_WRITABLE_ENTRY_H_
+
+#include <memory>
+
+#include "net/disk_cache/disk_cache.h"
+
+namespace content {
+
+// A custom deleter that closes the entry. But if WritingCompleted() hasn't been
+// called, it will doom the entry before closing it.
+class ScopedWritableDeleter {
+ public:
+  ScopedWritableDeleter() = default;
+  ScopedWritableDeleter(ScopedWritableDeleter&& other) = default;
+  ScopedWritableDeleter& operator=(ScopedWritableDeleter&& other) = default;
+
+  void operator()(disk_cache::Entry* entry) {
+    if (!completed_)
+      entry->Doom();
+
+    // |entry| is owned by the backend, we just need to close it as it's
+    // ref-counted.
+    entry->Close();
+  }
+
+  void WritingCompleted() { completed_ = true; }
+
+ private:
+  bool completed_ = false;
+};
+
+// Use this to manage disk_cache::Entry*'s that should be doomed before closing
+// unless told otherwise (via calling WritingCompleted on the deleter).
+//
+// Example:
+// ScopedWritableEntry entry(my_entry);
+// .. write some stuff ..
+// entry.get_deleter().WritingCompleted();
+typedef std::unique_ptr<disk_cache::Entry, ScopedWritableDeleter>
+    ScopedWritableEntry;
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_CACHE_STORAGE_SCOPED_WRITABLE_ENTRY_H_

net/disk_cache/disk_cache.h

@@ -480,6 +480,9 @@ struct EntryDeleter {
 };
 
 // Automatically closes an entry when it goes out of scope.
+// Warning: Be careful. Automatically closing may not be the desired behavior
+// when writing to an entry. You may wish to doom first (e.g., in case writing
+// hasn't yet completed but the browser is shutting down).
 typedef std::unique_ptr<Entry, EntryDeleter> ScopedEntryPtr;
 
 }  // namespace disk_cache