Make sure that AppCache tests subresource URLrequests against NETWORK whitelist

webkit/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 "webkit/appcache/appcache_request_handler.h"
 
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_job.h"
 #include "webkit/appcache/appcache.h"
 #include "webkit/appcache/appcache_policy.h"
 #include "webkit/appcache/appcache_url_request_job.h"
 
 namespace appcache {
 
 AppCacheRequestHandler::AppCacheRequestHandler(
     AppCacheHost* host, ResourceType::Type resource_type)
     : host_(host), resource_type_(resource_type),
       is_waiting_for_cache_selection_(false), found_group_id_(0),
       found_cache_id_(0), found_network_namespace_(false),
-      cache_entry_not_found_(false) {
+      cache_entry_not_found_(false), maybe_load_resource_executed_(false) {
   DCHECK(host_);
   host_->AddObserver(this);
 }
 
 AppCacheRequestHandler::~AppCacheRequestHandler() {
   if (host_) {
     storage()->CancelDelegateCallbacks(this);
     host_->RemoveObserver(this);
   }
 }
 
 AppCacheStorage* AppCacheRequestHandler::storage() const {
   DCHECK(host_);
   return host_->service()->storage();
 }
 
 void AppCacheRequestHandler::GetExtraResponseInfo(
     int64* cache_id, GURL* manifest_url) {
   if (job_ && job_->is_delivering_appcache_response()) {
     *cache_id = job_->cache_id();
     *manifest_url = job_->manifest_url();
   }
 }
 
 AppCacheURLRequestJob* AppCacheRequestHandler::MaybeLoadResource(
     net::URLRequest* request) {
+  maybe_load_resource_executed_ = true;
   if (!host_ || !IsSchemeAndMethodSupported(request) || cache_entry_not_found_)
     return NULL;
 
   // This method can get called multiple times over the life
   // of a request. The case we detect here is having scheduled
   // delivery of a "network response" using a job setup on an
   // earlier call thru this method. To send the request thru
   // to the network involves restarting the request altogether,
   // which will call thru to our interception layer again.
   // This time thru, we return NULL so the request hits the wire.
@@ skipping 29 matching lines @@
   }
 
   return job_;
 }
 
 AppCacheURLRequestJob* AppCacheRequestHandler::MaybeLoadFallbackForRedirect(
     net::URLRequest* request, const GURL& location) {
   if (!host_ || !IsSchemeAndMethodSupported(request) || cache_entry_not_found_)
     return NULL;
   if (is_main_resource())
     return NULL;

[michaeln, 2012/08/25 01:33:41]

a very small change that might workaround the immediate failure mode (if not
actually 'fix' the problem of how the heck to integrate these two things
together) could be to return NULL here if !maybe_load_resource_executed_ ?

[vabr (Chromium), 2012/08/30 13:01:06]

I like very much this suggestion.

However, it will mean that app-caching will be switched off for pages which get
redirected. Do you think it would make sense to put here something like the
following?

if (!maybe_load_resource_executed_ &&
IsNetworkWhitelisted(request->original_url()))
  return NULL;

[michaeln, 2012/09/05 19:24:21]

No worse than the current state of things where both appcache'ing and
webrequest'ing is busted. The result is that for now at least, webrequest based
redirection trumps whatever the appcache may have to say.
I'm not sure what would that accomplish really? Suppose the original url is
appcached (and not in a network or fallback namespace), if we proceed, the
request will result in an error response. I don't see that as an improvement.

[vabr (Chromium), 2012/09/06 17:34:21]

I agree that this is not making things worse.
When I suggested the "IsNetworkWhitelisted" I was thinking of not messing up
redirects from other sources than WebRequest. But for external redirects
|maybe_load_resource_executed_| will always be true, so that's not an issue.

   if (request->url().GetOrigin() == location.GetOrigin())
     return NULL;
 
   DCHECK(!job_);  // our jobs never generate redirects
 
+  if (!maybe_load_resource_executed_) {
+    // If MaybeLoadResource was not executed, we did not see this request
+    // before redirect. We need to call MaybeLoadResource to fill our 'found'
+    // data members and handle things we should have chance to handle before
+    // the redirect.
+    return MaybeLoadResource(request);

[michaeln, 2012/08/25 01:33:41]

How is the 'fallback' behavior triggered if we go down the main resource
codepath, i guess there's no case for which we should ever invoke the fallback?

+  }
+
   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_ = new AppCacheURLRequestJob(request, storage());
     DeliverAppCachedResponse(
         found_fallback_entry_, found_cache_id_, found_group_id_,
         found_manifest_url_,  true, found_namespace_entry_url_);
   } else if (!found_network_namespace_) {
     // 6.9.6, step 6: Fail the resource load.
     job_ = new AppCacheURLRequestJob(request, storage());
     DeliverErrorResponse();
   } else {
     // 6.9.6 step 3 and 5: Fetch the resource normally.
   }
 
   return job_;
 }
 
 AppCacheURLRequestJob* AppCacheRequestHandler::MaybeLoadFallbackForResponse(
     net::URLRequest* request) {
   if (!host_ || !IsSchemeAndMethodSupported(request) || cache_entry_not_found_)
     return NULL;

[michaeln, 2012/08/25 01:33:42]

ditto if (!maybe_load_resource_executed_) return NULL?

   if (!found_fallback_entry_.has_response_id())
     return NULL;
 
   if (request->status().status() == net::URLRequestStatus::CANCELED ||
       request->status().status() == net::URLRequestStatus::HANDLED_EXTERNALLY) {
     // 6.9.6, step 4: But not if the user canceled the download.
     return NULL;
   }
 
   // We don't fallback for responses that we delivered.
@@ skipping 164 matching lines @@
 }
 
 void AppCacheRequestHandler::ContinueMaybeLoadSubResource() {
   // 6.9.6 Changes to the networking model
   // If the resource is not to be fetched using the HTTP GET mechanism or
   // equivalent ... then fetch the resource normally.
   DCHECK(job_);
   DCHECK(host_->associated_cache() &&
          host_->associated_cache()->is_complete());
 
-  const GURL& url = job_->request()->url();
+  const GURL& url = job_->request()->original_url();

[michaeln, 2012/08/25 01:33:42]

It doesn't look like will work when following a chain of redirects. The
'handler' instance lives for the duration of of the URLRequest. When being
queried about intercepting the 2nd location in a chain, this logic will look for
a hit for the original url.

   AppCache* cache = host_->associated_cache();
   storage()->FindResponseForSubRequest(
       host_->associated_cache(), url,
       &found_entry_, &found_fallback_entry_, &found_network_namespace_);
 
   if (found_entry_.has_response_id()) {
     // Step 2: If there's an entry, get it instead.
     DCHECK(!found_network_namespace_ &&
            !found_fallback_entry_.has_response_id());
     found_cache_id_ = cache->cache_id();
     found_group_id_ = cache->owning_group()->group_id();
     found_manifest_url_ = cache->owning_group()->manifest_url();
     DeliverAppCachedResponse(
         found_entry_, found_cache_id_, found_group_id_, found_manifest_url_,
         false, GURL());
     return;
   }
 
   if (found_fallback_entry_.has_response_id()) {
+    // First check whether this is in fact a redirected request. If it is then
+    // we were got here through MaybeLoadFallbackForRedirect.
+    if (job_->request()->url_chain().size() > 1) {
+      // 6.9.6, step 4: If this results in a redirect to another origin,
+      // get the resource of the fallback entry.
+      DeliverAppCachedResponse(
+          found_fallback_entry_, found_cache_id_, found_group_id_,
+          found_manifest_url_,  true, found_namespace_entry_url_);
+      return;
+    }
     // Step 4: Fetch the resource normally, if this results
     // in certain conditions, then use the fallback.
     DCHECK(!found_network_namespace_ &&
            !found_entry_.has_response_id());
     found_cache_id_ = cache->cache_id();
     found_manifest_url_ = cache->owning_group()->manifest_url();
     DeliverNetworkResponse();
     return;
   }
 
@@ skipping 21 matching lines @@
   if (!host_->associated_cache() ||
       !host_->associated_cache()->is_complete()) {
     DeliverNetworkResponse();
     return;
   }
 
   ContinueMaybeLoadSubResource();
 }
 
 }  // namespace appcache