Make sure that AppCache tests subresource URLrequests against NETWORK whitelist

webkit/appcache/appcache_request_handler.cc

Index: webkit/appcache/appcache_request_handler.cc
diff --git a/webkit/appcache/appcache_request_handler.cc b/webkit/appcache/appcache_request_handler.cc
index 0f123a134bb52fe16ad9d8da5966f7b9e4a89d9d..66a13a41e5ee8ee003e6d6161e8f010a6ad7cea2 100644
--- a/webkit/appcache/appcache_request_handler.cc
+++ b/webkit/appcache/appcache_request_handler.cc
@@ -17,7 +17,7 @@ AppCacheRequestHandler::AppCacheRequestHandler(
     : 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) {
+      request_response_found_(false), cache_entry_not_found_(false) {
   DCHECK(host_);
   host_->AddObserver(this);
 }
@@ -71,6 +71,7 @@ AppCacheURLRequestJob* AppCacheRequestHandler::MaybeLoadResource(
   found_cache_id_ = kNoCacheId;
   found_manifest_url_ = GURL();
   found_network_namespace_ = false;
+  request_response_found_ = false;
 
   if (is_main_resource())
     MaybeLoadMainResource(request);
@@ -99,6 +100,12 @@ AppCacheURLRequestJob* AppCacheRequestHandler::MaybeLoadFallbackForRedirect(
 
   DCHECK(!job_);  // our jobs never generate redirects
 

[michaeln, 2012/08/17 00:50:43]

I think there's an additional condition that has to be taken care of...

if (host_->is_selection_pending()) {
  // Stuff is being read out of the db on a background thread,
  // FindResponseForSubRequest() can't be used until it's loaded up.
  // See MaybeLoadSubResource() and OnCacheSelectionComplete() for
  // how that's handled when this class gets a chance to intercept
  // the request from the Start(). We provide a 'job' in that case,
  // but the 'job' just sits there and waits for OnCacheSelectionComplete()
  // prior to continuing.
}

[vabr (Chromium), 2012/08/17 16:22:35]

This should now be addressed by the call to MaybeLoadResources.

+  if (!request_response_found_) {
+    storage()->FindResponseForSubRequest(
+        host_->associated_cache(), request->url(),
+        &found_entry_, &found_fallback_entry_, &found_network_namespace_);

[michaeln, 2012/08/17 00:50:43]

What should we do if found_entry_.has_response_id() at this point? The
originally requested url is in the appcache, and we've been redirected via
WebRequest (presumably) to go elsewhere. There's no clause for that particular
case below. As coded, the appcache will synthesize an error and the page won't
work (despite there being a cached resource ready to go).

[vabr (Chromium), 2012/08/17 16:22:35]

Here I realised that I should use original_url(), not url().
Ideally, we should let the user know what is happening, but this is beyond this
CL and bug 141114 (it is within the scope of 121325). As a workaround, I chose
to let AppCache use the cached page. This would happen, if AppCache got its
hands on the request before it was hijacked via NetworkDelegate.

+    request_response_found_ = true;
+  }
   if (found_fallback_entry_.has_response_id()) {

[michaeln, 2012/08/17 00:50:43]

As coded we'll will load an appcached fallback resource and disregard the
redirect in this case where we've found a fallback resource. From the appcache
consumers point of view, the loading of a fallback resource is a network error
handling behavior. Not really sure that behavior is appropriate in this case.

[vabr (Chromium), 2012/08/17 16:22:35]

Through the use of MaybeLoadResource within MaybeLoadFallbackForRedirect we end
up serving fallback resource if the original_url is not cached, and also not
network-whitelisted (if I understood correctly that
found_fallback_entry_.has_response_id() and found_network_namespace_ cannot both
be true). Are there still situations here where serving the fallback would be
inappropriate?

     // 6.9.6, step 4: If this results in a redirect to another origin,
     // get the resource of the fallback entry.
@@ -310,6 +317,7 @@ void AppCacheRequestHandler::ContinueMaybeLoadSubResource() {
   storage()->FindResponseForSubRequest(
       host_->associated_cache(), url,
       &found_entry_, &found_fallback_entry_, &found_network_namespace_);
+  request_response_found_ = true;
 
   if (found_entry_.has_response_id()) {
     // Step 2: If there's an entry, get it instead.