Add support for subresource request fallback in AppCache for the network service..

content/browser/appcache/appcache_request_handler.cc

 // Copyright (c) 2011 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_request_handler.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 112 matching lines @@
   // TODO(vabr) This is a temporary fix (see crbug/141114). We should get rid of
   // it once a more general solution to crbug/121325 is in place.
   if (!maybe_load_resource_executed_)
     return NULL;
   if (request_->GetURL().GetOrigin() == location.GetOrigin())
     return NULL;
 
   DCHECK(!job_.get());  // our jobs never generate redirects
 
   std::unique_ptr<AppCacheJob> job;
+  // In network service land, the job initiates a fallback request. We reuse
+  // the existing job to deliver the fallback response.

[michaeln, 2017/07/17 21:04:55]

I think we might be able to better encapsulate this difference about the
lifecycle of the job object. Could the logic about reusing the existing job or
not was inside the CreateJob() method, wdyt?

[ananta, 2017/07/18 00:19:03]

Did not want to touch the CreateJob() function. I added another method
MaybeCreateJobForFallback() which is called from here and from
MaybeLoadFallbackForResponse()

+  if (!base::FeatureList::IsEnabled(features::kNetworkService)) {
+    job = CreateJob(network_delegate);
+  } else {
+    job.reset(job_.get());

[michaeln, 2017/07/17 21:04:55]

does the dcheck on line 130 no longer hold true?

[ananta, 2017/07/18 00:19:03]

It does. I changed the DCHECK for the network service case along with a comment

+  }
+
   if (found_fallback_entry_.has_response_id()) {
     // 6.9.6, step 4: If this results in a redirect to another origin,
     // get the resource of the fallback entry.
-    job = CreateJob(network_delegate);
     DeliverAppCachedResponse(found_fallback_entry_, found_cache_id_,
                              found_manifest_url_, true,
                              found_namespace_entry_url_);
   } else if (!found_network_namespace_) {
     // 6.9.6, step 6: Fail the resource load.
-    job = CreateJob(network_delegate);
     DeliverErrorResponse();
   } else {
     // 6.9.6 step 3 and 5: Fetch the resource normally.

[michaeln, 2017/07/17 21:04:55]

In the net::urlrequest based impl, exsting code returns nullptr in this case,
but in the CL we're returning a non-null job ptr in that case. Is this change
intentional?

[ananta, 2017/07/18 00:19:03]

I restored the code back to how it was originally. We call the newly added
method MaybeCreateJobForFallback() in the if and else statements.

   }
 
   return job.release();
 }
 
 AppCacheJob* AppCacheRequestHandler::MaybeLoadFallbackForResponse(
     net::NetworkDelegate* network_delegate) {
   if (!host_ ||
       !AppCacheRequest::IsSchemeAndMethodSupportedForAppCache(request_.get()) ||
       cache_entry_not_found_)
     return NULL;
   if (!found_fallback_entry_.has_response_id())
     return NULL;
 
   if (request_->IsCancelled()) {
     // 6.9.6, step 4: But not if the user canceled the download.
     return NULL;
   }
 
   // We don't fallback for responses that we delivered.
-  if (job_.get()) {
+  if (job_.get() && !base::FeatureList::IsEnabled(features::kNetworkService)) {
     DCHECK(!job_->IsDeliveringNetworkResponse());
     return NULL;
   }
 
   if (request_->IsSuccess()) {
     int code_major = request_->GetResponseCode() / 100;
     if (code_major !=4 && code_major != 5)
       return NULL;
 
     // Servers can override the fallback behavior with a response header.
     const std::string kFallbackOverrideHeader(
         "x-chromium-appcache-fallback-override");
     const std::string kFallbackOverrideValue(
         "disallow-fallback");
     std::string header_value;
     header_value = request_->GetResponseHeaderByName(kFallbackOverrideHeader);
     if (header_value == kFallbackOverrideValue)
       return NULL;
   }
 
   // 6.9.6, step 4: If this results in a 4xx or 5xx status code
   // or there were network errors, get the resource of the fallback entry.
-  std::unique_ptr<AppCacheJob> job = CreateJob(network_delegate);
+
+  // In network service land, the job initiates a fallback request. We reuse
+  // the existing job to deliver the fallback response.
+  std::unique_ptr<AppCacheJob> job;
+  if (!base::FeatureList::IsEnabled(features::kNetworkService)) {

[michaeln, 2017/07/17 21:04:55]

Ditto question about encapsulating reuse vs recreate inside of CreateJob()?

[ananta, 2017/07/18 00:19:03]

Moved this code to MaybeCreateJobForFallback().

+    job = CreateJob(network_delegate);
+  } else {
+    DCHECK(job_.get());
+    job.reset(job_.get());
+  }
+
   DeliverAppCachedResponse(found_fallback_entry_, found_cache_id_,
                            found_manifest_url_, true,
                            found_namespace_entry_url_);
   return job.release();
 }
 
 void AppCacheRequestHandler::GetExtraResponseInfo(int64_t* cache_id,
                                                   GURL* manifest_url) {
   *cache_id = cache_id_;
   *manifest_url = manifest_url_;
@@ skipping 123 matching lines @@
   job_.reset();
 }
 
 std::unique_ptr<AppCacheJob> AppCacheRequestHandler::CreateJob(
     net::NetworkDelegate* network_delegate) {
   std::unique_ptr<AppCacheJob> job = AppCacheJob::Create(
       is_main_resource(), host_, storage(), request_.get(), network_delegate,
       base::Bind(&AppCacheRequestHandler::OnPrepareToRestart,
                  base::Unretained(this)));
   job_ = job->GetWeakPtr();
-  if (!is_main_resource() &&
+  if (job_.get() && !is_main_resource() &&

[michaeln, 2017/07/17 21:04:55]

can job_.get() test false here?

[ananta, 2017/07/18 00:19:03]

Yeah. Removed it.

       base::FeatureList::IsEnabled(features::kNetworkService)) {
     AppCacheURLLoaderJob* loader_job = job_->AsURLLoaderJob();
 
     loader_job->SetSubresourceLoadInfo(
         std::move(subresource_load_info_),
         network_url_loader_factory_getter_.get());
   }
 
   return job;
 }
@@ skipping 225 matching lines @@
   mojom::URLLoaderFactoryPtr factory_ptr = nullptr;
 
   // The factory is destroyed when the renderer drops the connection.
   AppCacheSubresourceURLFactory::CreateURLLoaderFactory(
       network_url_loader_factory_getter_.get(), appcache_host_, &factory_ptr);
 
   return factory_ptr;
 }
 
 }  // namespace content