ServiceWorkerWriteToCacheJob: refactor

content/browser/service_worker/service_worker_write_to_cache_job.cc

Index: content/browser/service_worker/service_worker_write_to_cache_job.cc
diff --git a/content/browser/service_worker/service_worker_write_to_cache_job.cc b/content/browser/service_worker/service_worker_write_to_cache_job.cc
index 781a65257b61f1371eb8eaa703cdae73565c3dd0..a5105f743bd6e35cf12d761462f044841476e0c5 100644
--- a/content/browser/service_worker/service_worker_write_to_cache_job.cc
+++ b/content/browser/service_worker/service_worker_write_to_cache_job.cc
@@ -6,8 +6,11 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
+#include "content/browser/service_worker/service_worker_cache_writer.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_disk_cache.h"
 #include "content/browser/service_worker/service_worker_metrics.h"
@@ -40,260 +43,8 @@ const char kRedirectError[] =
     "The script resource is behind a redirect, which is disallowed.";
 const char kServiceWorkerAllowed[] = "Service-Worker-Allowed";
 
-const int kBufferSize = 16 * 1024;
-
 }  // namespace
 
-// Reads an existing resource and copies it via
-// ServiceWorkerWriteToCacheJob::WriteData.
-class ServiceWorkerWriteToCacheJob::Copier : public base::RefCounted<Copier> {
- public:
-  Copier(base::WeakPtr<ServiceWorkerWriteToCacheJob> owner,
-         scoped_ptr<ServiceWorkerResponseReader> reader,
-         int bytes_to_copy,
-         const base::Callback<void(ServiceWorkerStatusCode)>& callback)
-      : owner_(owner),
-        reader_(reader.release()),
-        bytes_to_copy_(bytes_to_copy),
-        callback_(callback) {
-    DCHECK_LT(0, bytes_to_copy_);
-  }
-
-  void Start() {
-    io_buffer_ = new net::IOBuffer(kBufferSize);
-    ReadSomeData();
-  }
-
- private:
-  friend class base::RefCounted<Copier>;
-  ~Copier() {}
-
-  void ReadSomeData() {
-    reader_->ReadData(io_buffer_.get(), kBufferSize,
-                      base::Bind(&Copier::OnReadDataComplete, this));
-  }
-
-  void OnReadDataComplete(int result) {
-    if (!owner_)
-      return;
-    if (result <= 0) {
-      // We hit EOF or error but weren't done copying.
-      Complete(SERVICE_WORKER_ERROR_FAILED);
-      return;
-    }
-
-    int bytes_to_write = std::min(bytes_to_copy_, result);
-    owner_->WriteData(io_buffer_.get(), bytes_to_write,
-                      base::Bind(&Copier::OnWriteDataComplete, this));
-  }
-
-  void OnWriteDataComplete(int result) {
-    if (result < 0) {
-      Complete(SERVICE_WORKER_ERROR_FAILED);
-      return;
-    }
-
-    DCHECK_LE(result, bytes_to_copy_);
-    bytes_to_copy_ -= result;
-    if (bytes_to_copy_ == 0) {
-      Complete(SERVICE_WORKER_OK);
-      return;
-    }
-
-    ReadSomeData();
-  }
-
-  void Complete(ServiceWorkerStatusCode status) {
-    if (!owner_)
-      return;
-    callback_.Run(status);
-  }
-
-  base::WeakPtr<ServiceWorkerWriteToCacheJob> owner_;
-  scoped_ptr<ServiceWorkerResponseReader> reader_;
-  int bytes_to_copy_ = 0;
-  base::Callback<void(ServiceWorkerStatusCode)> callback_;
-  scoped_refptr<net::IOBuffer> io_buffer_;
-};
-
-// Abstract consumer for ServiceWorkerWriteToCacheJob that processes data from
-// the network.
-class ServiceWorkerWriteToCacheJob::NetDataConsumer {
- public:
-  virtual ~NetDataConsumer() {}
-
-  // Called by |owner_|'s OnResponseStarted.
-  virtual void OnResponseStarted() = 0;
-
-  // HandleData should call |owner_|->NotifyReadComplete() when done.
-  virtual void HandleData(net::IOBuffer* buf, int size) = 0;
-};
-
-// Dumb consumer that just writes everything it sees to disk.
-class ServiceWorkerWriteToCacheJob::PassThroughConsumer
-    : public ServiceWorkerWriteToCacheJob::NetDataConsumer {
- public:
-  explicit PassThroughConsumer(ServiceWorkerWriteToCacheJob* owner)
-      : owner_(owner), weak_factory_(this) {}
-  ~PassThroughConsumer() override {}
-
-  void OnResponseStarted() override {
-    owner_->WriteHeaders(
-        base::Bind(&PassThroughConsumer::OnWriteHeadersComplete,
-                   weak_factory_.GetWeakPtr()));
-    owner_->SetPendingIO();
-  }
-
-  void HandleData(net::IOBuffer* buf, int size) override {
-    if (size == 0) {
-      owner_->OnPassThroughComplete();
-      return;
-    }
-
-    owner_->WriteData(buf, size,
-                      base::Bind(&PassThroughConsumer::OnWriteDataComplete,
-                                 weak_factory_.GetWeakPtr()));
-    owner_->SetPendingIO();
-  }
-
- private:
-  void OnWriteHeadersComplete() {
-    owner_->ClearPendingIO();
-    owner_->CommitHeadersAndNotifyHeadersComplete();
-  }
-
-  void OnWriteDataComplete(int result) {
-    owner_->ClearPendingIO();
-    owner_->NotifyReadComplete(result);
-  }
-
-  ServiceWorkerWriteToCacheJob* owner_;
-  base::WeakPtrFactory<PassThroughConsumer> weak_factory_;
-};
-
-// Compares an existing resource with data progressively fed to it.
-// Calls back to |owner|->OnCompareComplete once done.
-class ServiceWorkerWriteToCacheJob::Comparer
-    : public ServiceWorkerWriteToCacheJob::NetDataConsumer {
- public:
-  Comparer(ServiceWorkerWriteToCacheJob* owner,
-           scoped_ptr<ServiceWorkerResponseReader> reader)
-      : owner_(owner), reader_(reader.release()), weak_factory_(this) {}
-  ~Comparer() override {}
-
-  void OnResponseStarted() override {
-    owner_->CommitHeadersAndNotifyHeadersComplete();
-  }
-
-  void HandleData(net::IOBuffer* buf, int size) override {
-    net_data_ = buf;
-    net_data_offset_ = 0;
-    net_data_size_ = size;
-
-    if (size == 0 && info_) {
-      Complete(bytes_matched_ == info_->response_data_size);
-      return;
-    }
-
-    if (!info_) {
-      read_buffer_ = new net::IOBuffer(kBufferSize);
-      info_ = new HttpResponseInfoIOBuffer;
-      reader_->ReadInfo(info_.get(), base::Bind(&Comparer::OnReadInfoComplete,
-                                                weak_factory_.GetWeakPtr()));
-      owner_->SetPendingIO();
-      return;
-    }
-
-    ReadSomeData();
-    owner_->SetPendingIO();
-  }
-
- private:
-  int bytes_remaining() {
-    DCHECK(net_data_);
-    DCHECK_LE(0, net_data_offset_);
-    DCHECK_LE(net_data_offset_, net_data_size_);
-    return net_data_size_ - net_data_offset_;
-  }
-
-  void OnReadInfoComplete(int result) {
-    if (result < 0) {
-      Complete(false);
-      return;
-    }
-
-    if (bytes_remaining() == 0) {
-      Complete(bytes_matched_ == info_->response_data_size);
-      return;
-    }
-
-    ReadSomeData();
-  }
-
-  void ReadSomeData() {
-    DCHECK_LT(0, bytes_remaining());
-    int bytes_to_read = std::min(bytes_remaining(), kBufferSize);
-    reader_->ReadData(
-        read_buffer_.get(), bytes_to_read,
-        base::Bind(&Comparer::OnReadDataComplete, weak_factory_.GetWeakPtr()));
-  }
-
-  void OnReadDataComplete(int result) {
-    if (result <= 0) {
-      // We hit error or EOF but had more to compare.
-      Complete(false);
-      return;
-    }
-
-    DCHECK_LE(result, bytes_remaining());
-    if (memcmp(net_data_->data() + net_data_offset_, read_buffer_->data(),
-               result) != 0) {
-      Complete(false);
-      return;
-    }
-
-    net_data_offset_ += result;
-    if (bytes_remaining() == 0) {
-      NotifyReadComplete();
-      return;
-    }
-
-    ReadSomeData();
-  }
-
-  // Completes one HandleData() call.
-  void NotifyReadComplete() {
-    int size = net_data_size_;
-    net_data_ = nullptr;
-    net_data_offset_ = 0;
-    net_data_size_ = 0;
-
-    bytes_matched_ += size;
-    owner_->ClearPendingIO();
-    owner_->NotifyReadComplete(size);
-  }
-
-  // Completes the entire Comparer.
-  void Complete(bool is_equal) {
-    owner_->OnCompareComplete(bytes_matched_, is_equal);
-  }
-
-  ServiceWorkerWriteToCacheJob* owner_;
-  scoped_ptr<ServiceWorkerResponseReader> reader_;
-  scoped_refptr<net::IOBuffer> read_buffer_;
-  scoped_refptr<HttpResponseInfoIOBuffer> info_;
-
-  // Cumulative number of bytes successfully compared.
-  int bytes_matched_ = 0;
-
-  // State used for one HandleData() call.
-  scoped_refptr<net::IOBuffer> net_data_;
-  int net_data_offset_ = 0;
-  int net_data_size_ = 0;
-
-  base::WeakPtrFactory<Comparer> weak_factory_;
-};
-
 ServiceWorkerWriteToCacheJob::ServiceWorkerWriteToCacheJob(
     net::URLRequest* request,
     net::NetworkDelegate* network_delegate,
@@ -331,14 +82,12 @@ void ServiceWorkerWriteToCacheJob::Start() {
         net::URLRequestStatus::FAILED, net::ERR_FAILED));
     return;
   }
-  if (incumbent_response_id_ != kInvalidServiceWorkerResourceId) {
-    scoped_ptr<ServiceWorkerResponseReader> incumbent_reader =
-        context_->storage()->CreateResponseReader(incumbent_response_id_);
-    consumer_.reset(new Comparer(this, incumbent_reader.Pass()));
-  } else {
-    consumer_.reset(new PassThroughConsumer(this));
-  }
 
+  cache_writer_.reset(new ServiceWorkerCacheWriter(
+      base::Bind(&ServiceWorkerWriteToCacheJob::CreateResponseReader,
+                 base::Unretained(this)),
+      base::Bind(&ServiceWorkerWriteToCacheJob::CreateResponseWriter,
+                 base::Unretained(this))));
   version_->script_cache_map()->NotifyStartedCaching(
       url_, response_id_);
   did_notify_started_ = true;
@@ -417,7 +166,7 @@ bool ServiceWorkerWriteToCacheJob::ReadRawData(net::IOBuffer* buf,
   }
 
   DCHECK_EQ(0, *bytes_read);
-  consumer_->HandleData(buf, 0);
+  NotifyFinishedCaching(net::URLRequestStatus(), std::string());
   if (did_notify_finished_)

[falken, 2015/08/28 05:45:24]

After calling NotifyFinishedCaching, this will always be true.

Is it correct to call NotifyFinishedCaching here? In the current code, the
consumer_ may be PassThroughConsumer or Comparer. If it's the Comparer, it might
need to start reading headers from disk asynchronously, in which case it sets
is_io_pending().

[Elly Fong-Jones, 2015/08/31 15:03:46]

You are correct, this code is very wrong. It should only call
NotifyFinishedCaching if !did_notify_finished_ and !is_io_pending(), since that
means a synchronous EOF (errors are handled by the earlier !is_success()
clause). I'll add a comment about this logic.

Also, this is missing an EOF HandleNetData() call.

     return GetStatus().is_success();
   if (GetStatus().is_io_pending())
@@ -482,83 +231,6 @@ net::URLRequestStatus ServiceWorkerWriteToCacheJob::ReadNetData(
   return net_request_->status();
 }
 
-void ServiceWorkerWriteToCacheJob::WriteHeaders(const base::Closure& callback) {
-  if (!context_) {
-    AsyncNotifyDoneHelper(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
-        kFetchScriptError);
-    return;
-  }
-  TRACE_EVENT_ASYNC_STEP_INTO0("ServiceWorker",
-                               "ServiceWorkerWriteToCacheJob::ExecutingJob",
-                               this, "WriteHeaders");
-  writer_ = context_->storage()->CreateResponseWriter(response_id_);
-  scoped_refptr<HttpResponseInfoIOBuffer> info_buffer =
-      new HttpResponseInfoIOBuffer(
-          new net::HttpResponseInfo(net_request_->response_info()));
-  writer_->WriteInfo(
-      info_buffer.get(),
-      base::Bind(&ServiceWorkerWriteToCacheJob::OnWriteHeadersComplete,
-                 weak_factory_.GetWeakPtr(), callback));
-}
-
-void ServiceWorkerWriteToCacheJob::OnWriteHeadersComplete(
-    const base::Closure& callback,
-    int result) {
-  if (result < 0) {
-    ServiceWorkerMetrics::CountWriteResponseResult(
-        ServiceWorkerMetrics::WRITE_HEADERS_ERROR);
-    AsyncNotifyDoneHelper(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, result),
-        kFetchScriptError);
-    return;
-  }
-  TRACE_EVENT_ASYNC_STEP_INTO0("ServiceWorker",
-                               "ServiceWorkerWriteToCacheJob::ExecutingJob",
-                               this, "WriteHeadersCompleted");
-  callback.Run();
-}
-
-void ServiceWorkerWriteToCacheJob::WriteData(
-    net::IOBuffer* buf,
-    int bytes_to_write,
-    const base::Callback<void(int result)>& callback) {
-  DCHECK_LT(0, bytes_to_write);
-  TRACE_EVENT_ASYNC_STEP_INTO1(
-      "ServiceWorker", "ServiceWorkerWriteToCacheJob::ExecutingJob", this,
-      "WriteData", "Amount to write", bytes_to_write);
-
-  writer_->WriteData(
-      buf, bytes_to_write,
-      base::Bind(&ServiceWorkerWriteToCacheJob::OnWriteDataComplete,
-                 weak_factory_.GetWeakPtr(), callback));
-}
-
-void ServiceWorkerWriteToCacheJob::OnWriteDataComplete(
-    const base::Callback<void(int result)>& callback,
-    int result) {
-  DCHECK_NE(0, result);
-  if (!context_) {
-    AsyncNotifyDoneHelper(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
-        kFetchScriptError);
-    return;
-  }
-  if (result < 0) {
-    ServiceWorkerMetrics::CountWriteResponseResult(
-        ServiceWorkerMetrics::WRITE_DATA_ERROR);
-    AsyncNotifyDoneHelper(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, result),
-        kFetchScriptError);
-    return;
-  }
-  ServiceWorkerMetrics::CountWriteResponseResult(
-      ServiceWorkerMetrics::WRITE_OK);
-  callback.Run(result);
-  TRACE_EVENT_ASYNC_END0("ServiceWorker",
-                         "ServiceWorkerWriteToCacheJob::ExecutingJob", this);
-}
-
 void ServiceWorkerWriteToCacheJob::OnReceivedRedirect(
     net::URLRequest* request,
     const net::RedirectInfo& redirect_info,
@@ -675,17 +347,39 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
   if (net_request_->response_info().network_accessed)
     version_->embedded_worker()->OnNetworkAccessedForScriptLoad();
 
-  consumer_->OnResponseStarted();
+  http_info_.reset(new net::HttpResponseInfo(net_request_->response_info()));
+  scoped_refptr<HttpResponseInfoIOBuffer> info_buffer =
+      new HttpResponseInfoIOBuffer(
+          new net::HttpResponseInfo(net_request_->response_info()));
+  net::Error error = cache_writer_->WriteHeaders(

[falken, 2015/08/28 05:45:24]

Does this actually write headers? The current code avoids writing to disk until
there's a change. Maybe the name can be tweaked if this doesn't write headers.

[Elly Fong-Jones, 2015/08/31 15:03:46]

I renamed these methods to MaybeWriteHeaders and MaybeWriteData.

+      info_buffer.get(),
+      base::Bind(&ServiceWorkerWriteToCacheJob::OnWriteHeadersComplete,
+                 weak_factory_.GetWeakPtr()));
+  SetStatus(net::URLRequestStatus::FromError(error));
+  if (error != net::ERR_IO_PENDING) {
+    NotifyHeadersComplete();
+  }
 }
 
-void ServiceWorkerWriteToCacheJob::CommitHeadersAndNotifyHeadersComplete() {
-  http_info_.reset(new net::HttpResponseInfo(net_request_->response_info()));
+void ServiceWorkerWriteToCacheJob::OnWriteHeadersComplete(net::Error error) {
+  SetStatus(net::URLRequestStatus::FromError(error));
   NotifyHeadersComplete();
 }
 
+void ServiceWorkerWriteToCacheJob::OnWriteDataComplete(net::Error error) {
+  SetStatus(net::URLRequestStatus::FromError(error));
+  NotifyReadComplete(error == net::OK ? io_buffer_bytes_ : error);
+}
+
 void ServiceWorkerWriteToCacheJob::HandleNetData(int bytes_read) {
   io_buffer_bytes_ = bytes_read;
-  consumer_->HandleData(io_buffer_.get(), bytes_read);
+  net::Error error = cache_writer_->WriteData(
+      io_buffer_.get(), bytes_read,
+      base::Bind(&ServiceWorkerWriteToCacheJob::OnWriteDataComplete,
+                 weak_factory_.GetWeakPtr()));
+  SetStatus(net::URLRequestStatus::FromError(error));
+  if (error != net::ERR_IO_PENDING)
+    NotifyReadComplete(error == net::OK ? bytes_read : error);
 }
 
 void ServiceWorkerWriteToCacheJob::OnReadCompleted(
@@ -727,80 +421,6 @@ bool ServiceWorkerWriteToCacheJob::CheckPathRestriction(
   return true;
 }
 
-void ServiceWorkerWriteToCacheJob::SetPendingIO() {
-  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
-}
-
-void ServiceWorkerWriteToCacheJob::ClearPendingIO() {
-  SetStatus(net::URLRequestStatus());
-}
-
-void ServiceWorkerWriteToCacheJob::OnPassThroughComplete() {
-  NotifyFinishedCaching(net::URLRequestStatus(), std::string());
-  if (GetStatus().is_io_pending()) {
-    ClearPendingIO();
-    NotifyReadComplete(0);
-  }
-}
-
-void ServiceWorkerWriteToCacheJob::OnCompareComplete(int bytes_matched,
-                                                     bool is_equal) {
-  if (is_equal) {
-    // This version is identical to the incumbent, so discard it and fail this
-    // job.
-    version_->SetStartWorkerStatusCode(SERVICE_WORKER_ERROR_EXISTS);
-    AsyncNotifyDoneHelper(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
-        kFetchScriptError);
-    return;
-  }
-
-  // We must switch to the pass through consumer. Write what is known
-  // (headers + bytes matched) to disk.
-  WriteHeaders(base::Bind(&ServiceWorkerWriteToCacheJob::CopyIncumbent,
-                          weak_factory_.GetWeakPtr(), bytes_matched));
-  SetPendingIO();
-}
-
-void ServiceWorkerWriteToCacheJob::CopyIncumbent(int bytes_to_copy) {
-  if (bytes_to_copy == 0) {
-    OnCopyComplete(SERVICE_WORKER_OK);
-    return;
-  }
-  scoped_ptr<ServiceWorkerResponseReader> incumbent_reader =
-      context_->storage()->CreateResponseReader(incumbent_response_id_);
-  scoped_refptr<Copier> copier = new Copier(
-      weak_factory_.GetWeakPtr(), incumbent_reader.Pass(), bytes_to_copy,
-      base::Bind(&ServiceWorkerWriteToCacheJob::OnCopyComplete,
-                 weak_factory_.GetWeakPtr()));
-  copier->Start();  // It deletes itself when done.
-  DCHECK(GetStatus().is_io_pending());
-}
-
-void ServiceWorkerWriteToCacheJob::OnCopyComplete(
-    ServiceWorkerStatusCode status) {
-  if (status != SERVICE_WORKER_OK) {
-    AsyncNotifyDoneHelper(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED),
-        kFetchScriptError);
-    return;
-  }
-
-  // Continue processing the net data that triggered the comparison fail and
-  // copy.
-  if (io_buffer_bytes_ > 0) {
-    consumer_.reset(new PassThroughConsumer(this));
-    consumer_->HandleData(io_buffer_.get(), io_buffer_bytes_);
-    return;
-  }
-
-  // The copy was triggered by EOF from the network, which
-  // means the job is now done.
-  NotifyFinishedCaching(net::URLRequestStatus(), std::string());
-  ClearPendingIO();
-  NotifyReadComplete(0);
-}
-
 void ServiceWorkerWriteToCacheJob::AsyncNotifyDoneHelper(
     const net::URLRequestStatus& status,
     const std::string& status_message) {
@@ -816,10 +436,24 @@ void ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(
   DCHECK(!did_notify_finished_);
   int size = -1;
   if (status.is_success())
-    size = writer_ ? writer_->amount_written() : 0;
+    size = cache_writer_->BytesWritten();
   version_->script_cache_map()->NotifyFinishedCaching(url_, size, status,
                                                       status_message);
   did_notify_finished_ = true;
 }
 
+scoped_ptr<ServiceWorkerResponseReader>
+ServiceWorkerWriteToCacheJob::CreateResponseReader() {
+  LOG(ERROR) << "Reader: " << incumbent_response_id_;
+  if (incumbent_response_id_ != kInvalidServiceWorkerResponseId)
+    return context_->storage()->CreateResponseReader(incumbent_response_id_);
+  return nullptr;
+}
+
+scoped_ptr<ServiceWorkerResponseWriter>
+ServiceWorkerWriteToCacheJob::CreateResponseWriter() {
+  LOG(ERROR) << "Writer: " << response_id_;
+  return context_->storage()->CreateResponseWriter(response_id_);
+}
+
 }  // namespace content