Get main frame and subframe AppCache loads to work.

content/browser/appcache/appcache_url_loader_job.cc

Index: content/browser/appcache/appcache_url_loader_job.cc
diff --git a/content/browser/appcache/appcache_url_loader_job.cc b/content/browser/appcache/appcache_url_loader_job.cc
index 09f15459e3bd1a54fd050e1c1669795c9991956b..52050044abc898190a69a74d7af941ce0c3a43a6 100644
--- a/content/browser/appcache/appcache_url_loader_job.cc
+++ b/content/browser/appcache/appcache_url_loader_job.cc
@@ -3,51 +3,264 @@
 // 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/common/net_adapters.h"
+#include "content/public/common/resource_type.h"
 
 namespace content {
 
-AppCacheURLLoaderJob::~AppCacheURLLoaderJob() {}
+AppCacheURLLoaderJob::~AppCacheURLLoaderJob() {
+  if (storage_)

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

this is where we're likely to crash in some cases, the weakptr would protect
against that

[ananta, 2017/06/09 20:39:33]

Added weakptr support to AppCacheStorage.

+    storage_->CancelDelegateCallbacks(this);
+}
 
 void AppCacheURLLoaderJob::Kill() {}
 
 bool AppCacheURLLoaderJob::IsStarted() const {
-  return false;
+  return delivery_type_ != AWAITING_DELIVERY_ORDERS;
 }
 
-bool AppCacheURLLoaderJob::IsWaiting() const {
-  return false;
+void AppCacheURLLoaderJob::DeliverAppCachedResponse(const GURL& manifest_url,
+                                                    int64_t cache_id,
+                                                    const AppCacheEntry& entry,
+                                                    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;
+
+  // TODO(ananta)
+  // Implement the AppCacheServiceImpl::Observer interface or add weak pointer
+  // support to it.
+  storage_->LoadResponseInfo(manifest_url_, entry_.response_id(), this);
 }
 
-bool AppCacheURLLoaderJob::IsDeliveringAppCacheResponse() const {
-  return false;
+void AppCacheURLLoaderJob::DeliverNetworkResponse() {
+  delivery_type_ = NETWORK_DELIVERY;
+
+  AppCacheHistograms::AddNetworkJobStartDelaySample(base::TimeTicks::Now() -
+                                                    start_time_tick_);
+
+  DCHECK(!loader_callback_.is_null());
+  // 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.
+  std::move(loader_callback_).Run(StartLoaderCallback());
 }
 
-bool AppCacheURLLoaderJob::IsDeliveringNetworkResponse() const {
-  return false;
+void AppCacheURLLoaderJob::DeliverErrorResponse() {
+  delivery_type_ = ERROR_DELIVERY;
+  storage_ = nullptr;

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

i'd suggest moving this into the NotifyCompleted helper and to be safe
conditionally CancelDelegateCallbacks in there if the ptr is non-null

[ananta, 2017/06/09 20:39:32]

Done. We don't need to set it to null anymore as we hold a weakptr.

+  // TODO(ananta)
+  // Fill up the correct error code here and add support in NotifyError() to
+  // generate a proper error response.
+  // We may need to call OnReceiveResponse on the client. That requires HTTP
+  // header generation etc.
+  NotifyCompleted(net::ERR_UNEXPECTED);
+  AppCacheHistograms::AddErrorJobStartDelaySample(base::TimeTicks::Now() -
+                                                  start_time_tick_);
 }
 
-bool AppCacheURLLoaderJob::IsDeliveringErrorResponse() const {
-  return false;
+const GURL& AppCacheURLLoaderJob::GetURL() const {
+  return request_.url;
 }
 
-bool AppCacheURLLoaderJob::IsCacheEntryNotFound() const {
-  return false;
+AppCacheURLLoaderJob* AppCacheURLLoaderJob::AsURLLoaderJob() {
+  return this;
 }
 
-void AppCacheURLLoaderJob::DeliverAppCachedResponse(const GURL& manifest_url,
-                                                    int64_t cache_id,
-                                                    const AppCacheEntry& entry,
-                                                    bool is_fallback) {}
+void AppCacheURLLoaderJob::FollowRedirect() {
+  DCHECK(false);
+}
 
-void AppCacheURLLoaderJob::DeliverNetworkResponse() {}
+void AppCacheURLLoaderJob::SetPriority(net::RequestPriority priority,
+                                       int32_t intra_priority_value) {
+  NOTREACHED() << "We don't support SetPriority()";
+}
 
-void AppCacheURLLoaderJob::DeliverErrorResponse() {}
+void AppCacheURLLoaderJob::Start(mojom::URLLoaderRequest request,
+                                 mojom::URLLoaderClientPtr client) {
+  DCHECK(!binding_.is_bound());
+  binding_.Bind(std::move(request));
 
-const GURL& AppCacheURLLoaderJob::GetURL() const {
-  return url_;
+  binding_.set_connection_error_handler(base::Bind(
+      &AppCacheURLLoaderJob::OnConnectionError, StaticAsWeakPtr(this)));
+
+  client_info_ = std::move(client);
+
+  // Send the cached AppCacheResponse if any.
+  if (info_.get())
+    SendResponseInfo();
+}
+
+AppCacheURLLoaderJob::AppCacheURLLoaderJob(const ResourceRequest& request,
+                                           AppCacheStorage* storage)
+    : request_(request),
+      storage_(storage),
+      start_time_tick_(base::TimeTicks::Now()),
+      cache_id_(kAppCacheNoCacheId),
+      is_fallback_(false),
+      binding_(this),
+      writable_handle_watcher_(FROM_HERE,
+                               mojo::SimpleWatcher::ArmingPolicy::MANUAL) {}
+
+void AppCacheURLLoaderJob::OnResponseInfoLoaded(
+    AppCacheResponseInfo* response_info,
+    int64_t response_id) {
+  DCHECK(IsDeliveringAppCacheResponse());
+

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

As coded, i'm the raw storage_ptr_ is real likely to cause crashes. I think a
weakptr is probably a good fit eventually, but for this CL, i have some
suggestions about how to handlle it in this class, first...

null the storageptr out before return from this method, we only need the storage
obj to load the info and to create the reader, it's not needed anymore beyond
this point. Also no need to CancelDelegateCallbacks(this) in here either, The
only responseInfo callback we give it has been consumed.

suggestion, you could load it into a local variable and null out the data member
at the top of the method, use the local at the ->CreateResponseReader callsite.

[ananta, 2017/06/09 20:39:32]

We have a weakptr now. Checked it for null at the top and returned with
notifycompleted call.

+  if (response_info) {
+    info_ = response_info;
+    reader_.reset(
+        storage_->CreateResponseReader(manifest_url_, entry_.response_id()));
+
+    DCHECK(!loader_callback_.is_null());
+    std::move(loader_callback_)
+        .Run(base::Bind(&AppCacheURLLoaderJob::Start, StaticAsWeakPtr(this)));
+
+    // TODO(ananta)
+    // Handle range requests.
+
+    response_body_stream_ = std::move(data_pipe_.producer_handle);
+
+    // TODO(ananta)
+    // Move the asynchronous reading and mojo pipe handling code to a helper
+    // class. That would also need a change to BlobURLLoader.
+
+    // 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,
+                   StaticAsWeakPtr(this)));
+
+    if (client_info_)
+      SendResponseInfo();
+
+    ReadMore();
+  } else {
+    // Error case here. We fallback to the network.
+    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());
+    return;
+  } 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;
+  }
+
+  uint32_t bytes_written = static_cast<uint32_t>(result);
+  response_body_stream_ = pending_write_->Complete(bytes_written);
+  pending_write_ = nullptr;
+  ReadMore();
+}
+
+void AppCacheURLLoaderJob::OnConnectionError() {

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

we definitely want to nullout the storage_ ptr and call CancelDelegateCallbacks
in here, we don't want to enter OnRepsonseInfoLoaded() past this point

[ananta, 2017/06/09 20:39:32]

Done.We don't need to set it to null anymore as we hold a weakptr.

+  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(ananta)
+    // Add proper error handling here.
+    NotifyCompleted(net::ERR_UNEXPECTED);
+    writable_handle_watcher_.Cancel();
+    response_body_stream_.reset();
+    return;
+  }
+
+  CHECK_GT(static_cast<uint32_t>(std::numeric_limits<int>::max()), num_bytes);
+  scoped_refptr<NetToMojoIOBuffer> buffer = new NetToMojoIOBuffer(
+      pending_write_.get());
+
+  reader_->ReadData(
+      buffer.get(), info_->response_data_size(),
+      base::Bind(&AppCacheURLLoaderJob::OnReadComplete, StaticAsWeakPtr(this)));
 }
 
-AppCacheURLLoaderJob::AppCacheURLLoaderJob() {}
+void AppCacheURLLoaderJob::OnResponseBodyStreamReady(MojoResult result) {
+  // TODO(ananta)
+  // Add proper error handling here.
+  if (result != MOJO_RESULT_OK) {
+    DCHECK(false);
+    NotifyCompleted(net::ERR_UNEXPECTED);
+  }
+  ReadMore();
+}
+
+void AppCacheURLLoaderJob::NotifyCompleted(int error_code) {
+  // TODO(ananta)
+  // Fill other details in the ResourceRequestCompletionStatus structure.

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

this method might be a good bottle neck to conditionally null out the storage_
ptr and call CancelDelegateCallbacks(this), we defenitely don't want to enter
OnResponseInfoLoaded past this point.

[ananta, 2017/06/09 20:39:32]

Done. We don't need to set it to null anymore as we hold a weakptr.

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