Stop using AppCacheResponseWriter from ServiceWorkerResourceWriter

This CL removes AppCacheResponseReader dependency from
ServiceWorkerResourceWriter by re-implementing resource writing
logic. See [1] for the reasoning of the rewrite.

[1] https://docs.google.com/document/d/1UhdZnaeEP8GywZR0Gik6wF6jKmNY3je6aDhPfJ05YhU/edit?usp=sharing

This CL should not have behavior changes, and existing tests like
ServiceWorkerResourceStorageTest cover this change.

Bug: 1117369
Change-Id: I2fe4075ddff1c185768e0dd245e49f1ac5c26962
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2430687
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814137}

content/browser/service_worker/service_worker_disk_cache.cc

@@ -14,11 +14,6 @@ namespace content {
 ServiceWorkerDiskCache::ServiceWorkerDiskCache()
     : AppCacheDiskCache(/*use_simple_cache=*/true) {}
 
-ServiceWorkerResponseWriter::ServiceWorkerResponseWriter(
-    int64_t resource_id,
-    base::WeakPtr<AppCacheDiskCache> disk_cache)
-    : AppCacheResponseWriter(resource_id, std::move(disk_cache)) {}
-
 ServiceWorkerResponseMetadataWriter::ServiceWorkerResponseMetadataWriter(
     int64_t resource_id,
     base::WeakPtr<AppCacheDiskCache> disk_cache)

content/browser/service_worker/service_worker_disk_cache.h

@@ -26,18 +26,8 @@ class CONTENT_EXPORT ServiceWorkerDiskCache : public AppCacheDiskCache {
   ServiceWorkerDiskCache();
 };
 
-// TODO(crbug.com/1060076): Migrate to
-// storage::mojom::ServiceWorkerResourceWriter.
-class CONTENT_EXPORT ServiceWorkerResponseWriter
-    : public AppCacheResponseWriter {
- protected:
-  // Should only be constructed by the storage class.
-  friend class ServiceWorkerStorage;
-
-  ServiceWorkerResponseWriter(int64_t resource_id,
-                              base::WeakPtr<AppCacheDiskCache> disk_cache);
-};
-
+// TODO(crbug.com/1117369): Migrate to
+// storage::mojom::ServiceWorkerResourceMetadataWriter.
 class CONTENT_EXPORT ServiceWorkerResponseMetadataWriter
     : public AppCacheResponseMetadataWriter {
  protected:

content/browser/service_worker/service_worker_resource_ops.h

@@ -13,6 +13,76 @@ namespace content {
 
 class BigIOBuffer;
 
+// Manages a service worker disk cache entry. Creates and owns an entry.
+// TODO(bashi): Used by resource writers. Use in readers as well.
+class DiskEntryManager {
+ public:
+  DiskEntryManager(int64_t resource_id,
+                   base::WeakPtr<AppCacheDiskCache> disk_cache);
+  ~DiskEntryManager();
+
+  DiskEntryManager(const DiskEntryManager&) = delete;
+  DiskEntryManager& operator=(const DiskEntryManager&) = delete;
+
+  // Can be nullptr when a disk cache error occurs.
+  AppCacheDiskCacheEntry* entry() {
+    DCHECK_EQ(creation_phase_, CreationPhase::kDone);
+    return entry_;
+  }
+
+  // Calls the callback when entry() is created and can be used.
+  //
+  // Overlapping calls are not allowed. Specifically, once the method is called,
+  // it must not be called again until it calls the callback.
+  //
+  // If necessary, kicks off the creation of a disk cache entry for the
+  // `resource_id` passed to the constructor. After the callback is called,
+  // `entry()` can be safely called to obtain the created entry.
+  //
+  // Has a retry mechanism. If the first attempt fails, dooms the existing
+  // entry, then tries to create an entry again.
+  void EnsureEntryIsCreated(base::OnceClosure callback);
+
+ private:
+  // State of creating a disk_cache entry.
+  enum class CreationPhase {
+    kNoAttempt,
+    kInitialAttempt,
+    kDoomExisting,
+    kSecondAttempt,
+    kDone,
+  };
+
+  // Callbacks of EnsureEntryIsCreated(). These are static to manage the
+  // ownership of AppCacheDiskCacheEntry correctly.
+  // TODO(crbug.com/586174): Refactor service worker's disk cache to use
+  // disk_cache::EntryResult to make these callbacks non-static.
+  static void DidCreateEntryForFirstAttempt(
+      base::WeakPtr<DiskEntryManager> entry_manager,
+      AppCacheDiskCacheEntry** entry,
+      int rv);
+  static void DidDoomExistingEntry(
+      base::WeakPtr<DiskEntryManager> entry_manager,
+      int rv);
+  static void DidCreateEntryForSecondAttempt(
+      base::WeakPtr<DiskEntryManager> entry_manager,
+      AppCacheDiskCacheEntry** entry,
+      int rv);
+
+  void RunEnsureEntryIsCreatedCallback();
+
+  const int64_t resource_id_;
+  base::WeakPtr<AppCacheDiskCache> disk_cache_;
+  AppCacheDiskCacheEntry* entry_ = nullptr;
+
+  CreationPhase creation_phase_ = CreationPhase::kNoAttempt;
+
+  // Stored as a data member to handle //net-style maybe-async methods.
+  base::OnceClosure ensure_entry_is_created_callback_;
+
+  base::WeakPtrFactory<DiskEntryManager> weak_factory_{this};
+};
+
 // The implementation of storage::mojom::ServiceWorkerResourceReader.
 class ServiceWorkerResourceReaderImpl
     : public storage::mojom::ServiceWorkerResourceReader {
@@ -66,7 +136,7 @@ class ServiceWorkerResourceReaderImpl
   // Used to read metadata from disk cache.
   scoped_refptr<BigIOBuffer> metadata_buffer_;
   // Holds the return value of ReadResponseHead(). Stored as a member field
-  // to handle net style maybe-async methods.
+  // to handle //net-style maybe-async methods.
   network::mojom::URLResponseHeadPtr response_head_;
   // Holds the callback of ReadResponseHead(). Stored as a member field to
   // handle //net-style maybe-async methods.
@@ -76,7 +146,7 @@ class ServiceWorkerResourceReaderImpl
   std::unique_ptr<DataReader> data_reader_;
 
   // Holds the callback of EnsureEntryIsOpen(). Stored as a data member to
-  // handle net style maybe-async methods.
+  // handle //net-style maybe-async methods.
   base::OnceClosure open_entry_callback_;
 
 #if DCHECK_IS_ON()
@@ -95,15 +165,11 @@ class ServiceWorkerResourceReaderImpl
 };
 
 // The implementation of storage::mojom::ServiceWorkerResourceWriter.
-// Currently this class is an adaptor that uses ServiceWorkerResponseWriter
-// internally.
-// TODO(crbug.com/1055677): Fork the implementation of
-// ServiceWorkerResponseWriter and stop using it.
 class ServiceWorkerResourceWriterImpl
     : public storage::mojom::ServiceWorkerResourceWriter {
  public:
-  explicit ServiceWorkerResourceWriterImpl(
-      std::unique_ptr<ServiceWorkerResponseWriter> writer);
+  ServiceWorkerResourceWriterImpl(int64_t resource_id,
+                                  base::WeakPtr<AppCacheDiskCache> disk_cache);
 
   ServiceWorkerResourceWriterImpl(const ServiceWorkerResourceWriterImpl&) =
       delete;
@@ -112,14 +178,48 @@ class ServiceWorkerResourceWriterImpl
 
   ~ServiceWorkerResourceWriterImpl() override;
 
- private:
   // storage::mojom::ServiceWorkerResourceWriter implementations:
   void WriteResponseHead(network::mojom::URLResponseHeadPtr response_head,
                          WriteResponseHeadCallback callback) override;
   void WriteData(mojo_base::BigBuffer data,
                  WriteDataCallback callback) override;
 
-  const std::unique_ptr<ServiceWorkerResponseWriter> writer_;
+ private:
+  // Called while executing WriteResponseHead().
+  void WriteResponseHeadToEntry(
+      network::mojom::URLResponseHeadPtr response_head,
+      WriteResponseHeadCallback callback);
+  void DidWriteResponseHead(scoped_refptr<net::IOBuffer> buffer,
+                            size_t write_amount,
+                            int rv);
+
+  // Called while executing WriteData().
+  void WriteDataToEntry(mojo_base::BigBuffer data, WriteDataCallback callback);
+  void DidWriteData(scoped_refptr<net::IOBuffer> buffer,
+                    size_t write_amount,
+                    int rv);
+
+  DiskEntryManager entry_manager_;
+
+  // Points the current write position of WriteData().
+  size_t write_position_ = 0;
+
+  // Holds the callback of WriteResponseHead() or WriteData(). Stored as a data
+  // member to handle //net-style maybe-async methods.
+  net::CompletionOnceCallback write_callback_;
+
+#if DCHECK_IS_ON()
+  enum class State {
+    kIdle,
+    kWriteResponseHeadStarted,
+    kWriteResponseHeadHasEntry,
+    kWriteDataStarted,
+    kWriteDataHasEntry,
+  };
+  State state_ = State::kIdle;
+#endif  // DCHECK_IS_ON()
+
+  base::WeakPtrFactory<ServiceWorkerResourceWriterImpl> weak_factory_{this};
 };
 
 // The implementation of storage::mojom::ServiceWorkerResourceMetadataWriter.

content/browser/service_worker/service_worker_storage.cc

@@ -592,10 +592,10 @@ ServiceWorkerStorage::CreateResourceReader(int64_t resource_id) {
       resource_id, disk_cache()->GetWeakPtr());
 }
 
-std::unique_ptr<ServiceWorkerResponseWriter>
-ServiceWorkerStorage::CreateResponseWriter(int64_t resource_id) {
-  return base::WrapUnique(
-      new ServiceWorkerResponseWriter(resource_id, disk_cache()->GetWeakPtr()));
+std::unique_ptr<ServiceWorkerResourceWriterImpl>
+ServiceWorkerStorage::CreateResourceWriter(int64_t resource_id) {
+  return std::make_unique<ServiceWorkerResourceWriterImpl>(
+      resource_id, disk_cache()->GetWeakPtr());
 }
 
 std::unique_ptr<ServiceWorkerResponseMetadataWriter>

content/browser/service_worker/service_worker_storage.h

@@ -40,7 +40,6 @@ namespace content {
 
 class ServiceWorkerDiskCache;
 class ServiceWorkerResponseMetadataWriter;
-class ServiceWorkerResponseWriter;
 class ServiceWorkerStorageControlImplTest;
 
 namespace service_worker_storage_unittest {
@@ -95,10 +94,6 @@ class CONTENT_EXPORT ServiceWorkerStorage {
       int64_t deleted_version_id,
       const std::vector<int64_t>& newly_purgeable_resources)>;
 
-  using ResponseWriterCreationCallback = base::OnceCallback<void(
-      int64_t resource_id,
-      std::unique_ptr<ServiceWorkerResponseWriter> response_writer)>;
-
   using DatabaseStatusCallback =
       base::OnceCallback<void(ServiceWorkerDatabase::Status status)>;
   using GetUserDataInDBCallback =
@@ -191,7 +186,7 @@ class CONTENT_EXPORT ServiceWorkerStorage {
   // associated with the disabled disk cache if the storage is disabled.
   std::unique_ptr<ServiceWorkerResourceReaderImpl> CreateResourceReader(
       int64_t resource_id);
-  std::unique_ptr<ServiceWorkerResponseWriter> CreateResponseWriter(
+  std::unique_ptr<ServiceWorkerResourceWriterImpl> CreateResourceWriter(
       int64_t resource_id);
   std::unique_ptr<ServiceWorkerResponseMetadataWriter>
   CreateResponseMetadataWriter(int64_t resource_id);

content/browser/service_worker/service_worker_storage_control_impl.cc

@@ -262,8 +262,7 @@ void ServiceWorkerStorageControlImpl::CreateResourceWriter(
     int64_t resource_id,
     mojo::PendingReceiver<storage::mojom::ServiceWorkerResourceWriter> writer) {
   DCHECK_NE(resource_id, blink::mojom::kInvalidServiceWorkerResourceId);
-  mojo::MakeSelfOwnedReceiver(std::make_unique<ServiceWorkerResourceWriterImpl>(
-                                  storage_->CreateResponseWriter(resource_id)),
+  mojo::MakeSelfOwnedReceiver(storage_->CreateResourceWriter(resource_id),
                               std::move(writer));
 }