Get main frame and subframe AppCache loads to work.

content/browser/appcache/appcache_url_loader_job.cc

 // Copyright (c) 2017 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/appcache/appcache_url_loader_job.h"
-#include "content/browser/appcache/appcache_entry.h"
+#include "content/browser/appcache/appcache_histograms.h"
+#include "content/browser/url_loader_factory_getter.h"
+#include "content/common/net_adapters.h"
+#include "content/public/common/resource_type.h"
 
 namespace content {
 
 AppCacheURLLoaderJob::~AppCacheURLLoaderJob() {}
 
 void AppCacheURLLoaderJob::Kill() {}
 
 bool AppCacheURLLoaderJob::IsStarted() const {
-  return false;
-}
-
-bool AppCacheURLLoaderJob::IsWaiting() const {
-  return false;
-}
-
-bool AppCacheURLLoaderJob::IsDeliveringAppCacheResponse() const {
-  return false;
-}
-
-bool AppCacheURLLoaderJob::IsDeliveringNetworkResponse() const {
-  return false;
-}
-
-bool AppCacheURLLoaderJob::IsDeliveringErrorResponse() const {
-  return false;
-}
-
-bool AppCacheURLLoaderJob::IsCacheEntryNotFound() const {
-  return false;
+  return delivery_type_ != AWAITING_DELIVERY_ORDERS;
 }
 
 void AppCacheURLLoaderJob::DeliverAppCachedResponse(const GURL& manifest_url,
                                                     int64_t cache_id,
                                                     const AppCacheEntry& entry,
-                                                    bool is_fallback) {}
-
-void AppCacheURLLoaderJob::DeliverNetworkResponse() {}
-
-void AppCacheURLLoaderJob::DeliverErrorResponse() {}
+                                                    bool is_fallback) {
+  delivery_type_ = APPCACHED_DELIVERY;
+
+  AppCacheHistograms::AddAppCacheJobStartDelaySample(base::TimeTicks::Now() -
+                                                     start_time_tick_);
+
+  manifest_url_ = manifest_url;
+  cache_id_ = cache_id;
+  entry_ = entry;
+  is_fallback_ = is_fallback;
+
+  storage_->LoadResponseInfo(manifest_url_, entry_.response_id(), this);

[michaeln, 2017/06/09 02:06:18]

Unfortunately, the rawptr is a problem

We're going to have to implement the AppCacheServiceImpl::Observer interface,
for OnServiceReinitialized, or access it thru a WeakPtr.

Also we have to call storage->CancelDelegateCallbacks(this) if we're destructed
prior to that method completing.

[ananta, 2017/06/09 04:48:19]

I added a call to CancelDelegateCallbacks in the dtor. The URLRequestJob also
has the same problem  from what I can see. If it is ok we can revisit this in a
later patchset. Added a TODO here.

[michaeln, 2017/06/09 19:59:53]

The other class is actually safe but its not at all obvious why. The 'host'
ensures the storage object does not get deleted and the 'job' is guaranteed to
be killed prior to the host being deleted. Very indirect, but there.

see host::OnServiceReinitialized(), hander::OnDestructionImminent(host), and
job::Kill().

+}
+
+void AppCacheURLLoaderJob::DeliverNetworkResponse() {
+  delivery_type_ = NETWORK_DELIVERY;
+
+  AppCacheHistograms::AddNetworkJobStartDelaySample(base::TimeTicks::Now() -
+                                                    start_time_tick_);
+
+  // In network service land, if we are processing a navigation request, we
+  // need to inform the loader callback that we are not going to handle this
+  // request. The loader callback is valid only for navigation requests.
+  if (!loader_callback_.is_null()) {
+    std::move(loader_callback_).Run(StartLoaderCallback());
+    return;
+  }
+
+  url_loader_factory_getter_->GetNetworkFactory()->get()->CreateLoaderAndStart(

[michaeln, 2017/06/09 01:48:29]

We don't yet know if there will be a chain of handlers for this use case, it
could be composed similary with a LoaderCallback arrangement, i'd suggest
waiting for a later CL to add subresource loading support and eliminating all of
the tentative branching logic. If possible, it'd be nice if the same arrangement
with this class calling the LoaderCallback(nullptr) could work, fewer code
paths. 

[ananta, 2017/06/09 04:48:19]

Removed

+      mojo::MakeRequest(&url_loader_network_), routing_id_, request_id_,
+      mojom::kURLLoadOptionSendSSLInfo, request_, std::move(client_info_));
+}
+
+void AppCacheURLLoaderJob::DeliverErrorResponse() {
+  delivery_type_ = ERROR_DELIVERY;
+
+  // TODO(ananta)
+  // Fill up the correct error code here and add support in NotifyError() to
+  // generate a proper error response.
+  NotifyCompleted(net::ERR_UNEXPECTED);
+
+  AppCacheHistograms::AddErrorJobStartDelaySample(base::TimeTicks::Now() -
+                                                  start_time_tick_);
+}
 
 const GURL& AppCacheURLLoaderJob::GetURL() const {
-  return url_;
-}
-
-AppCacheURLLoaderJob::AppCacheURLLoaderJob() {}
+  return request_.url;
+}
+
+AppCacheURLLoaderJob* AppCacheURLLoaderJob::AsURLLoaderJob() {
+  return this;
+}
+
+void AppCacheURLLoaderJob::FollowRedirect() {
+  if (url_loader_network_)
+    url_loader_network_->FollowRedirect();
+}
+
+void AppCacheURLLoaderJob::SetPriority(net::RequestPriority priority,
+                                       int32_t intra_priority_value) {
+  NOTREACHED() << "We don't support SetPriority()";
+}
+
+void AppCacheURLLoaderJob::Start(int routing_id,
+                                 int request_id,
+                                 mojom::URLLoaderRequest request,
+                                 mojom::URLLoaderClientPtr client) {
+  DCHECK(!binding_.is_bound());
+  binding_.Bind(std::move(request));
+
+  binding_.set_connection_error_handler(base::Bind(
+      &AppCacheURLLoaderJob::OnConnectionError, StaticAsWeakPtr(this)));
+
+  routing_id_ = routing_id;
+  request_id_ = request_id;
+
+  client_info_ = std::move(client);
+
+  // Send the cached AppCacheResponse if any.
+  if (info_.get())
+    SendResponseInfo();
+}
+
+void AppCacheURLLoaderJob::SetURLLoaderFactoryGetter(
+    URLLoaderFactoryGetter* url_loader_factory_getter) {
+  url_loader_factory_getter_ = url_loader_factory_getter;
+}
+
+AppCacheURLLoaderJob::AppCacheURLLoaderJob(const ResourceRequest& request,
+                                           AppCacheStorage* storage)
+    : request_(request),
+      storage_(storage),
+      start_time_tick_(base::TimeTicks::Now()),
+      cache_id_(kAppCacheNoCacheId),
+      is_fallback_(false),
+      buffer_(nullptr),
+      binding_(this),
+      routing_id_(-1),
+      request_id_(-1),
+      writable_handle_watcher_(FROM_HERE,
+                               mojo::SimpleWatcher::ArmingPolicy::MANUAL) {}
+
+void AppCacheURLLoaderJob::OnResponseInfoLoaded(
+    AppCacheResponseInfo* response_info,
+    int64_t response_id) {
+  DCHECK(IsDeliveringAppCacheResponse());
+
+  if (response_info) {
+    info_ = response_info;
+    reader_.reset(
+        storage_->CreateResponseReader(manifest_url_, entry_.response_id()));
+
+    if (!loader_callback_.is_null()) {

[michaeln, 2017/06/09 01:48:30]

in our current use case, navigation, this is never expected to be null

[ananta, 2017/06/09 04:48:19]

Replaced with a DCHECK

+      std::move(loader_callback_)
+          .Run(base::Bind(&AppCacheURLLoaderJob::Start, StaticAsWeakPtr(this),
+                          0, 0));
+    }
+
+    // TODO(ananta)
+    // Handle range requests.
+
+    response_body_stream_ = std::move(data_pipe_.producer_handle);
+
+    // Wait for the data pipe to be ready to accept data.
+    writable_handle_watcher_.Watch(
+        response_body_stream_.get(), MOJO_HANDLE_SIGNAL_WRITABLE,
+        base::Bind(&AppCacheURLLoaderJob::OnResponseBodyStreamReady,

[michaeln, 2017/06/09 01:48:29]

is it possible/expected for OnResponseBodyStreamReady to be called prior to
return from the Watch() method, or shortly after return, if the pipe is in a
writable state at the time Watch() is called?

[ananta, 2017/06/09 04:48:19]

It is on the same thread?. Unless I am missing something here.

+                   StaticAsWeakPtr(this)));
+
+    if (client_info_)
+      SendResponseInfo();
+
+    ReadMore();
+  } else {
+    // Error case here. We fallback to the network.
+    // TODO(ananta)
+    // Should we be invoking the loader callback here to give control to the
+    // next handler in the chain?.

[michaeln, 2017/06/09 01:48:29]

The TODO comment seems stale or misplaced, for navigations the answer is yes,
and this is a question for the DeliverNetworkResponse() method.

[ananta, 2017/06/09 04:48:19]

Thanks. Removed the comment.

+    DeliverNetworkResponse();
+    AppCacheHistograms::CountResponseRetrieval(
+        false, IsResourceTypeFrame(request_.resource_type),
+        manifest_url_.GetOrigin());
+
+    cache_entry_not_found_ = true;
+  }
+}
+
+void AppCacheURLLoaderJob::OnCacheLoaded(AppCache* cache, int64_t cache_id) {
+  NOTREACHED() << "Unhandled at the moment.";
+}
+
+void AppCacheURLLoaderJob::OnReadComplete(int result) {
+  DLOG(WARNING) << "AppCache read completed with result: " << result;
+
+  bool is_main_resource = IsResourceTypeFrame(request_.resource_type);
+
+  if (result == 0) {
+    NotifyCompleted(result);
+    AppCacheHistograms::CountResponseRetrieval(true, is_main_resource,
+                                               manifest_url_.GetOrigin());

[michaeln, 2017/06/09 01:48:30]

you could early return here and avoid the if (result > 0) down below

[ananta, 2017/06/09 04:48:19]

Done.

+  } else if (result < 0) {
+    // TODO(ananta)
+    // Populate the relevant fields of the ResourceRequestCompletionStatus
+    // structure.
+    NotifyCompleted(result);
+    AppCacheHistograms::CountResponseRetrieval(false, is_main_resource,
+                                               manifest_url_.GetOrigin());
+    return;
+  }
+
+  if (result > 0) {
+    uint32_t bytes_written = static_cast<uint32_t>(result);
+    response_body_stream_ = pending_write_->Complete(bytes_written);
+    pending_write_ = nullptr;
+    ReadMore();
+  }
+}
+
+void AppCacheURLLoaderJob::OnConnectionError() {
+  base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
+}
+
+void AppCacheURLLoaderJob::SendResponseInfo() {
+  DCHECK(client_info_);
+
+  // If this is null it means the response information was sent to the client.
+  if (!data_pipe_.consumer_handle.is_valid())
+    return;
+
+  const net::HttpResponseInfo* http_info = info_->http_response_info();
+
+  ResourceResponseHead response_head;
+  response_head.headers = http_info->headers;
+
+  // TODO(ananta)
+  // Copy more fields.
+  http_info->headers->GetMimeType(&response_head.mime_type);
+  http_info->headers->GetCharset(&response_head.charset);
+
+  response_head.request_time = http_info->request_time;
+  response_head.response_time = http_info->response_time;
+  response_head.content_length = info_->response_data_size();
+
+  client_info_->OnReceiveResponse(response_head, http_info->ssl_info,
+                                  mojom::DownloadedTempFilePtr());
+
+  client_info_->OnStartLoadingResponseBody(
+      std::move(data_pipe_.consumer_handle));
+}
+
+void AppCacheURLLoaderJob::ReadMore() {
+  DCHECK(!pending_write_.get());
+
+  uint32_t num_bytes;
+  // TODO: we should use the abstractions in MojoAsyncResourceHandler.
+  MojoResult result = NetToMojoPendingBuffer::BeginWrite(
+      &response_body_stream_, &pending_write_, &num_bytes);
+  if (result == MOJO_RESULT_SHOULD_WAIT) {
+    // The pipe is full. We need to wait for it to have more space.
+    writable_handle_watcher_.ArmOrNotify();
+    return;
+  } else if (result != MOJO_RESULT_OK) {
+    // The response body stream is in a bad state. Bail.
+    // TODO: How should this be communicated to our client?

[michaeln, 2017/06/09 01:48:29]

Probably NotifyComplete(ERR)? As coded will progresss stop and the request will
hang?

[ananta, 2017/06/09 04:48:19]

Added a call to NotifyCompleted here.

+    writable_handle_watcher_.Cancel();
+    response_body_stream_.reset();
+    return;
+  }
+
+  CHECK_GT(static_cast<uint32_t>(std::numeric_limits<int>::max()), num_bytes);
+  buffer_ = nullptr;
+  buffer_ = new NetToMojoIOBuffer(pending_write_.get());

[michaeln, 2017/06/09 01:48:29]

Is the buffer_ data member is needed? It's only accessed in this method and the
reader retains its own scoped_ref<>. So its not clear to me it is needed?

I don't know much about mojo datapipe usage, so I hope jam takes a look at this
part. There's a lot of overlap with BlobURLLoader.

[ananta, 2017/06/09 04:48:19]

The buffer_ member should not be needed I think. Removed it. About the overlap,
I plan to add a separate patchset to move the common code out into a helper
class.

+
+  reader_->ReadData(
+      buffer_.get(), info_->response_data_size(),
+      base::Bind(&AppCacheURLLoaderJob::OnReadComplete, StaticAsWeakPtr(this)));
+}
+
+void AppCacheURLLoaderJob::OnResponseBodyStreamReady(MojoResult result) {
+  // TODO: Handle a bad |result| value.

[michaeln, 2017/06/09 01:48:30]

ditto probably NotifyComplete(ERR)

[ananta, 2017/06/09 04:48:19]

Done.

+  DCHECK_EQ(result, MOJO_RESULT_OK);
+  ReadMore();
+}
+
+void AppCacheURLLoaderJob::NotifyCompleted(int error_code) {
+  // TODO(ananta)
+  // Fill other details in the ResourceRequestCompletionStatus structure.
+  // In case of an error we may need to call OnReceiveResponse on the client.
+  // That requires HTTP header generation etc.

[michaeln, 2017/06/09 01:48:29]

I think we only need http header generation and a call to OnReceiveResponse when
we're called thru the DeleiverErrorResponse() callsite, the other callsites
happen after headers have already been sent.

[ananta, 2017/06/09 04:48:19]

Thanks. moved this comment to the DeliverErrorResponse function.

+  ResourceRequestCompletionStatus request_complete_data;
+  request_complete_data.error_code = error_code;
+  client_info_->OnComplete(request_complete_data);
+}
 
 }  // namespace content