[BlobAsync] Moving async handling into BlobStorageContext & quota out.

content/browser/service_worker/service_worker_url_request_job.cc

 // Copyright 2014 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/service_worker/service_worker_url_request_job.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
-#include <limits>
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
+#include "base/files/file_util.h"
 #include "base/guid.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
+#include "base/task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "content/browser/resource_context_impl.h"
 #include "content/browser/service_worker/embedded_worker_instance.h"
 #include "content/browser/service_worker/service_worker_blob_reader.h"
 #include "content/browser/service_worker/service_worker_fetch_dispatcher.h"
 #include "content/browser/service_worker/service_worker_provider_host.h"
 #include "content/browser/service_worker/service_worker_response_info.h"
 #include "content/browser/service_worker/service_worker_stream_reader.h"
 #include "content/common/resource_request_body_impl.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "content/common/service_worker/service_worker_utils.h"
 #include "content/public/browser/blob_handle.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/service_worker_context.h"
 #include "content/public/common/referrer.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
 #include "net/http/http_util.h"
 #include "net/log/net_log.h"
 #include "net/log/net_log_event_type.h"
@@ skipping 53 matching lines @@
     case m::REQUEST_JOB_ERROR_DESTROYED_WITH_BLOB:
     case m::REQUEST_JOB_ERROR_DESTROYED_WITH_STREAM:
     // Invalid type.
     case m::NUM_REQUEST_JOB_RESULT_TYPES:
       NOTREACHED() << result;
   }
   NOTREACHED() << result;
   return n::FAILED;
 }
 
+std::vector<int64_t> GetFileSizesOnBlockingPool(
+    std::vector<base::FilePath> file_paths) {
+  std::vector<int64_t> sizes;
+  sizes.reserve(file_paths.size());
+  for (const base::FilePath& path : file_paths) {
+    base::File::Info file_info;
+    if (!base::GetFileInfo(path, &file_info))

[falken, 2016/11/09 02:06:19]

maybe paranoid but should we check file_info.is_directory here?  (Since |size|
is undefined in that case according to the documentation.)

[dmurph, 2016/11/09 19:13:27]

Good idea. Done.

+      return std::vector<int64_t>();
+    sizes.push_back(file_info.size);
+  }
+  return sizes;
+}
+
 }  // namespace
 
-class ServiceWorkerURLRequestJob::BlobConstructionWaiter {
+class ServiceWorkerURLRequestJob::FileSizeResolver {

[falken, 2016/11/09 02:06:19]

Can we add a lightweight comment about what this class does? (Probably
BlobConstructionWaiter should have had one too.)

Something like "Sets the size on each DataElement in the request body that is a
file, so that ServiceWorkerURLRequestJob::CreateRequestBodyBlob can compute the
size of the entire request body, since DataElement size is not automatically set
for files." (if that's accurate)?

[dmurph, 2016/11/09 19:13:27]

Done.

  public:
-  explicit BlobConstructionWaiter(ServiceWorkerURLRequestJob* owner)
+  explicit FileSizeResolver(ServiceWorkerURLRequestJob* owner)
       : owner_(owner), weak_factory_(this) {
-    TRACE_EVENT_ASYNC_BEGIN1("ServiceWorker", "BlobConstructionWaiter", this,
-                             "URL", owner_->request()->url().spec());
+    TRACE_EVENT_ASYNC_BEGIN1("ServiceWorker", "BlobFilesWaiter", this, "URL",

[Marijn Kruisselbrink, 2016/11/09 18:39:39]

Is it intentional that you renamed the class to FileSizeResolver, but the
tracing still uses BlobFilesWaiter?

[dmurph, 2016/11/09 19:13:27]

Done.

+                             owner_->request()->url().spec());
     owner_->request()->net_log().BeginEvent(
-        net::NetLogEventType::SERVICE_WORKER_WAITING_FOR_REQUEST_BODY_BLOB);
+        net::NetLogEventType::SERVICE_WORKER_WAITING_FOR_REQUEST_BODY_FILES);
   }
 
-  ~BlobConstructionWaiter() {
+  ~FileSizeResolver() {
     owner_->request()->net_log().EndEvent(
-        net::NetLogEventType::SERVICE_WORKER_WAITING_FOR_REQUEST_BODY_BLOB,
+        net::NetLogEventType::SERVICE_WORKER_WAITING_FOR_REQUEST_BODY_FILES,
         net::NetLog::BoolCallback("success", phase_ == Phase::SUCCESS));
-    TRACE_EVENT_ASYNC_END1("ServiceWorker", "BlobConstructionWaiter", this,
-                           "Success", phase_ == Phase::SUCCESS);
+    TRACE_EVENT_ASYNC_END1("ServiceWorker", "BlobFilesWaiter", this, "Success",
+                           phase_ == Phase::SUCCESS);
   }
 
-  void RunOnComplete(const base::Callback<void(bool)>& callback) {
+  void RunOnComplete(base::TaskRunner* file_runner,

[falken, 2016/11/09 02:06:19]

This almost looks like the callback is run on the file_runner when complete.
Maybe the function name is just better as "Run" or "Resolve"?

[dmurph, 2016/11/09 19:13:26]

Done.

+                     const base::Callback<void(bool)>& callback) {
     DCHECK_EQ(static_cast<int>(Phase::INITIAL), static_cast<int>(phase_));
+    DCHECK(file_elements_.empty());
     phase_ = Phase::WAITING;
-    num_pending_request_body_blobs_ = 0;
+    body_ = owner_->body_;
     callback_ = callback;
 
-    for (const ResourceRequestBodyImpl::Element& element :
-         *(owner_->body_->elements())) {
-      if (element.type() != ResourceRequestBodyImpl::Element::TYPE_BLOB)
-        continue;
-
-      std::unique_ptr<storage::BlobDataHandle> handle =
-          owner_->blob_storage_context_->GetBlobDataFromUUID(
-              element.blob_uuid());
-      if (handle->IsBroken()) {
-        Complete(false);
-        return;
-      }
-      if (handle->IsBeingBuilt()) {
-        ++num_pending_request_body_blobs_;
-        handle->RunOnConstructionComplete(
-            base::Bind(&BlobConstructionWaiter::OneRequestBodyBlobCompleted,
-                       weak_factory_.GetWeakPtr()));
+    std::vector<base::FilePath> file_paths;
+    for (ResourceRequestBodyImpl::Element& element :
+         *body_->elements_mutable()) {
+      if (element.type() == ResourceRequestBodyImpl::Element::TYPE_FILE &&
+          element.length() == ResourceRequestBodyImpl::Element::kUnknownSize) {
+        file_elements_.push_back(&element);
+        file_paths.push_back(element.path());
       }
     }
+    if (file_elements_.empty()) {
+      Complete(true);
+      return;
+    }
 
-    if (num_pending_request_body_blobs_ == 0)
-      Complete(true);
+    PostTaskAndReplyWithResult(
+        file_runner, FROM_HERE,
+        base::Bind(&GetFileSizesOnBlockingPool, base::Passed(&file_paths)),
+        base::Bind(
+            &ServiceWorkerURLRequestJob::FileSizeResolver::OnFileSizesResolved,
+            weak_factory_.GetWeakPtr()));
   }
 
  private:
   enum class Phase { INITIAL, WAITING, SUCCESS, FAIL };
 
-  void OneRequestBodyBlobCompleted(
-      bool success,
-      storage::IPCBlobCreationCancelCode cancel_code) {
-    DCHECK_GT(num_pending_request_body_blobs_, 0UL);
-
-    if (success)
-      --num_pending_request_body_blobs_;
-    else
-      num_pending_request_body_blobs_ = 0;
-
-    if (num_pending_request_body_blobs_ == 0)
-      Complete(success);
+  void OnFileSizesResolved(std::vector<int64_t> sizes) {
+    bool success = !sizes.empty();
+    if (success) {
+      DCHECK_EQ(sizes.size(), file_elements_.size());
+      size_t num_elements = file_elements_.size();
+      for (size_t i = 0; i < num_elements; i++) {
+        auto* element = file_elements_[i];

[falken, 2016/11/09 02:06:19]

nit: I prefer writing out ResourceRequestBodyImpl::Element* here, easier to see
what element is.

[dmurph, 2016/11/09 19:13:27]

Done.

+        element->SetToFilePathRange(element->path(), element->offset(),
+                                    static_cast<uint64_t>(sizes[i]),

[falken, 2016/11/09 02:06:19]

is it always ok to static_cast this? maybe paranoid but should we do
checked_cast?

[dmurph, 2016/11/09 19:13:26]

Done.

+                                    element->expected_modification_time());
+      }
+      file_elements_.clear();
+    }
+    Complete(success);
   }
 
   void Complete(bool success) {
     DCHECK_EQ(static_cast<int>(Phase::WAITING), static_cast<int>(phase_));
     phase_ = success ? Phase::SUCCESS : Phase::FAIL;
     // Destroys |this|.
     callback_.Run(success);

[Marijn Kruisselbrink, 2016/11/09 18:39:39]

nit: if the callback can/does destroy |this|, it presumably also destroys
|callback_| itself. And destroying a callback while it is still executing is not
guaranteed to be safe.
For example if the callback is set to base::Bind(&foo, "bla") where foo is
defined as
void foo(const std::string& bar, bool success) { file_size_resolver_.reset(); /*
use bar */}
the bar parameter will end up being a reference to a string that is stored in
the callback, so as soon  file_size_resolver_ is reset bar will end up being a
no longer valid reference (I hope that made sense).
So if the callback can end up destroying callback_ you really should make
move/copy the callback to a local variable before calling it, with something
like base::ResetAndReturn(&callback_).Run(success), or just manually making a
copy.

[dmurph, 2016/11/09 19:13:27]

Ah good catch. I'll make a copy.

[falken, 2016/11/10 01:25:04]

Good idea, thanks for pointing out this issue in the original code mek.

   }
 
   // Owns and must outlive |this|.
   ServiceWorkerURLRequestJob* owner_;
 
   scoped_refptr<ResourceRequestBodyImpl> body_;
+  std::vector<ResourceRequestBodyImpl::Element*> file_elements_;
   base::Callback<void(bool)> callback_;
-  size_t num_pending_request_body_blobs_ = 0;
   Phase phase_ = Phase::INITIAL;
-  base::WeakPtrFactory<BlobConstructionWaiter> weak_factory_;
+  base::WeakPtrFactory<FileSizeResolver> weak_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(BlobConstructionWaiter);
+  DISALLOW_COPY_AND_ASSIGN(FileSizeResolver);
 };
 
 bool ServiceWorkerURLRequestJob::Delegate::RequestStillValid(
     ServiceWorkerMetrics::URLRequestJobResult* result) {
   return true;
 }
 
 ServiceWorkerURLRequestJob::ServiceWorkerURLRequestJob(
     net::URLRequest* request,
     net::NetworkDelegate* network_delegate,
@@ skipping 25 matching lines @@
       frame_type_(frame_type),
       fall_back_required_(false),
       body_(body),
       fetch_type_(fetch_type),
       weak_factory_(this) {
   DCHECK(delegate_) << "ServiceWorkerURLRequestJob requires a delegate";
 }
 
 ServiceWorkerURLRequestJob::~ServiceWorkerURLRequestJob() {
   stream_reader_.reset();
-  blob_construction_waiter_.reset();
+  file_size_resolver_.reset();
 
   if (!ShouldRecordResult())
     return;
   ServiceWorkerMetrics::URLRequestJobResult result =
       ServiceWorkerMetrics::REQUEST_JOB_ERROR_KILLED;
   if (response_body_type_ == STREAM)
     result = ServiceWorkerMetrics::REQUEST_JOB_ERROR_KILLED_WITH_STREAM;
   else if (response_body_type_ == BLOB)
     result = ServiceWorkerMetrics::REQUEST_JOB_ERROR_KILLED_WITH_BLOB;
   RecordResult(result);
@@ skipping 156 matching lines @@
     case FALLBACK_TO_NETWORK:
       FinalizeFallbackToNetwork();
       return;
 
     case FALLBACK_TO_RENDERER:
       FinalizeFallbackToRenderer();
       return;
 
     case FORWARD_TO_SERVICE_WORKER:
       if (HasRequestBody()) {
-        DCHECK(!blob_construction_waiter_);
-        blob_construction_waiter_.reset(new BlobConstructionWaiter(this));
-        blob_construction_waiter_->RunOnComplete(
+        DCHECK(!file_size_resolver_);
+        file_size_resolver_.reset(new FileSizeResolver(this));
+        file_size_resolver_->RunOnComplete(
+            BrowserThread::GetBlockingPool(),
             base::Bind(&ServiceWorkerURLRequestJob::RequestBodyBlobsCompleted,
                        GetWeakPtr()));
         return;
       }
 
       RequestBodyBlobsCompleted(true);
       return;
   }
 
   NOTREACHED();
@@ skipping 37 matching lines @@
     request->referrer =
         Referrer(GURL(request_->referrer()), blink::WebReferrerPolicyDefault);
   }
   request->fetch_type = fetch_type_;
   return request;
 }
 
 void ServiceWorkerURLRequestJob::CreateRequestBodyBlob(std::string* blob_uuid,
                                                        uint64_t* blob_size) {
   DCHECK(HasRequestBody());
-  // To ensure the blobs stick around until the end of the reading.
-  std::vector<std::unique_ptr<storage::BlobDataHandle>> handles;
-  std::vector<std::unique_ptr<storage::BlobDataSnapshot>> snapshots;
-  // TODO(dmurph): Allow blobs to be added below, so that the context can
-  // efficiently re-use blob items for the new blob.
-  std::vector<const ResourceRequestBodyImpl::Element*> resolved_elements;
+  const std::string uuid(base::GenerateGUID());

[falken, 2016/11/09 02:06:19]

nit: we don't seem to use |uuid| later, and call blob_builder.uuid() instead.

[dmurph, 2016/11/09 19:13:27]

Done.

+  storage::BlobDataBuilder blob_builder(uuid);
   for (const ResourceRequestBodyImpl::Element& element : (*body_->elements())) {
-    if (element.type() != ResourceRequestBodyImpl::Element::TYPE_BLOB) {
-      resolved_elements.push_back(&element);
-      continue;
-    }
-    std::unique_ptr<storage::BlobDataHandle> handle =
-        blob_storage_context_->GetBlobDataFromUUID(element.blob_uuid());
-    std::unique_ptr<storage::BlobDataSnapshot> snapshot =
-        handle->CreateSnapshot();
-    if (snapshot->items().empty())
-      continue;
-    const auto& items = snapshot->items();
-    for (const auto& item : items) {
-      DCHECK_NE(storage::DataElement::TYPE_BLOB, item->type());
-      resolved_elements.push_back(item->data_element_ptr());
-    }
-    handles.push_back(std::move(handle));
-    snapshots.push_back(std::move(snapshot));
-  }
-
-  const std::string uuid(base::GenerateGUID());
-  uint64_t total_size = 0;
-
-  storage::BlobDataBuilder blob_builder(uuid);
-  for (size_t i = 0; i < resolved_elements.size(); ++i) {
-    const ResourceRequestBodyImpl::Element& element = *resolved_elements[i];
-    if (total_size != std::numeric_limits<uint64_t>::max() &&
-        element.length() != std::numeric_limits<uint64_t>::max())
-      total_size += element.length();
-    else
-      total_size = std::numeric_limits<uint64_t>::max();
-    switch (element.type()) {
-      case ResourceRequestBodyImpl::Element::TYPE_BYTES:
-        blob_builder.AppendData(element.bytes(), element.length());
-        break;
-      case ResourceRequestBodyImpl::Element::TYPE_FILE:
-        blob_builder.AppendFile(element.path(), element.offset(),
-                                element.length(),
-                                element.expected_modification_time());
-        break;
-      case ResourceRequestBodyImpl::Element::TYPE_BLOB:
-        // Blob elements should be resolved beforehand.
-        NOTREACHED();
-        break;
-      case ResourceRequestBodyImpl::Element::TYPE_FILE_FILESYSTEM:
-        blob_builder.AppendFileSystemFile(element.filesystem_url(),
-                                          element.offset(), element.length(),
-                                          element.expected_modification_time());
-        break;
-      default:
-        NOTIMPLEMENTED();
-    }

[falken, 2016/11/09 02:06:19]

So happy about this deletion!

[dmurph, 2016/11/09 19:13:26]

Eliminating this code was one of my goals for the new system, so I am also very
happy :D :D

Thanks for your help!

+    blob_builder.AppendIPCDataElement(element);
   }
 
   request_body_blob_data_handle_ =
       blob_storage_context_->AddFinishedBlob(&blob_builder);
-  *blob_uuid = uuid;
-  *blob_size = total_size;
+  *blob_uuid = blob_builder.uuid();
+  *blob_size = request_body_blob_data_handle_->size();
 }
 
 void ServiceWorkerURLRequestJob::DidPrepareFetchEvent(
     scoped_refptr<ServiceWorkerVersion> version) {
   worker_ready_time_ = base::TimeTicks::Now();
   load_timing_info_.send_start = worker_ready_time_;
 
   // Record the time taken for the browser to find and possibly start an active
   // worker to which to dispatch a FetchEvent for a main frame resource request.
   // For context, a FetchEvent can only be dispatched to an ACTIVATED worker
@@ skipping 298 matching lines @@
   return ServiceWorkerUtils::IsMainResourceType(resource_type_);
 }
 
 bool ServiceWorkerURLRequestJob::HasRequestBody() {
   // URLRequest::has_upload() must be checked since its upload data may have
   // been cleared while handling a redirect.
   return request_->has_upload() && body_.get() && blob_storage_context_;
 }
 
 void ServiceWorkerURLRequestJob::RequestBodyBlobsCompleted(bool success) {
-  blob_construction_waiter_.reset();
+  file_size_resolver_.reset();
   if (!success) {
     RecordResult(
         ServiceWorkerMetrics::REQUEST_JOB_ERROR_REQUEST_BODY_BLOB_FAILED);
     // TODO(falken): This and below should probably be NotifyStartError, not
     // DeliverErrorResponse. But changing it causes
     // ServiceWorkerURLRequestJobTest.DeletedProviderHostBeforeFetchEvent to
     // fail.
     DeliverErrorResponse();
     return;
   }
@@ skipping 18 matching lines @@
       base::Bind(&ServiceWorkerURLRequestJob::DidPrepareFetchEvent,
                  weak_factory_.GetWeakPtr(), active_worker),
       base::Bind(&ServiceWorkerURLRequestJob::DidDispatchFetchEvent,
                  weak_factory_.GetWeakPtr())));
   worker_start_time_ = base::TimeTicks::Now();
   fetch_dispatcher_->MaybeStartNavigationPreload(request());
   fetch_dispatcher_->Run();
 }
 
 }  // namespace content