Explicitly prevent URL-to-app redirection for forced downloads

There are 2 parts:
1) Replace ResourceRequestInfoImpl::is_download() with virtual ResourceRequestInfo::IsDownload().
2) Use it in app_url_redirector.cpp to explicitly avoid redirection of forced downloads.

@darin: In my testing, forced downloads were fine even without this (apparently, resource throttling is disabled in this case). Let me know if you still think explicit prevention is a good idea (I do think so).

BUG=302904

Review URL: https://codereview.chromium.org/62463004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237570 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 43 insertions, 10 deletions

diff --git a/chrome/browser/apps/app_url_redirector.cc b/chrome/browser/apps/app_url_redirector.cc
index c72f982..8a6934a 100644
--- a/chrome/browser/apps/app_url_redirector.cc
+++ b/chrome/browser/apps/app_url_redirector.cc
@@ -16,6 +16,7 @@
 #include "components/navigation_interception/navigation_params.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/resource_throttle.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/info_map.h"
@@ -23,6 +24,7 @@
 #include "net/url_request/url_request.h"
 
 using content::BrowserThread;
+using content::ResourceRequestInfo;
 using content::WebContents;
 using extensions::Extension;
 using extensions::UrlHandlers;
@@ -39,8 +41,10 @@ bool LaunchAppWithUrl(
 
   // Redirect top-level navigations only. This excludes iframes and webviews
   // in particular.
-  if (source->IsSubframe())
+  if (source->IsSubframe()) {
+    DVLOG(1) << "Cancel redirection: source is a subframe";
     return false;
+  }
 
   // These are guaranteed by CreateThrottleFor below.
   DCHECK(!params.is_post());
@@ -53,6 +57,9 @@ bool LaunchAppWithUrl(
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
 
+  DVLOG(0) << "Launching app handler with URL: "
+           << params.url().spec() << " -> "
+           << app->name() << "(" << app->id() << "):" << handler_id;
   apps::LaunchPlatformAppWithUrl(
       profile, app, handler_id, params.url(), params.referrer().url);
 
@@ -65,18 +72,34 @@ bool LaunchAppWithUrl(
 content::ResourceThrottle*
 AppUrlRedirector::MaybeCreateThrottleFor(net::URLRequest* request,
                                          ProfileIOData* profile_io_data) {
+  DVLOG(0) << "Considering URL for redirection: "
+           << request->method() << " " << request->url().spec();
+
   // Support only GET for now.
-  if (request->method() != "GET")
+  if (request->method() != "GET") {
+    DVLOG(1) << "Skip redirection: method is not GET";
+    return NULL;
+  }
+
+  if (!request->url().SchemeIsHTTPOrHTTPS()) {
+    DVLOG(1) << "Skip redirection: scheme is not HTTP or HTTPS";
     return NULL;
+  }
 
-  if (!request->url().SchemeIsHTTPOrHTTPS())
+  // The user has indicated that a URL should be force downloaded. Turn off
+  // URL redirection in this case.
+  if (ResourceRequestInfo::ForRequest(request)->IsDownload()) {
+    DVLOG(1) << "Skip redirection: request is a forced download";
     return NULL;
+  }
 
   // Never redirect URLs to apps in incognito. Technically, apps are not
   // supported in incognito, but that may change in future.
   // See crbug.com/240879, which tracks incognito support for v2 apps.
-  if (profile_io_data->is_incognito())
+  if (profile_io_data->is_incognito()) {
+    DVLOG(1) << "Skip redirection: unsupported in incognito";
     return NULL;
+  }
 
   const ExtensionSet& extensions =
       profile_io_data->GetExtensionInfoMap()->extensions();
@@ -86,6 +109,9 @@ AppUrlRedirector::MaybeCreateThrottleFor(net::URLRequest* request,
     const UrlHandlerInfo* handler =
         UrlHandlers::FindMatchingUrlHandler(*iter, request->url());
     if (handler) {
+      DVLOG(1) << "Found matching app handler for redirection: "
+               << (*iter)->name() << "(" << (*iter)->id() << "):"
+               << handler->id;
       return new navigation_interception::InterceptNavigationResourceThrottle(
           request,
           base::Bind(&LaunchAppWithUrl,
@@ -94,5 +120,6 @@ AppUrlRedirector::MaybeCreateThrottleFor(net::URLRequest* request,
     }
   }
 
+  DVLOG(1) << "Skipping redirection: no matching app handler found";
   return NULL;
 }
diff --git a/content/browser/loader/cross_site_resource_handler.cc b/content/browser/loader/cross_site_resource_handler.cc
index 64af035..51d830f 100644
--- a/content/browser/loader/cross_site_resource_handler.cc
+++ b/content/browser/loader/cross_site_resource_handler.cc
@@ -134,7 +134,7 @@ bool CrossSiteResourceHandler::OnResponseStarted(
   // navigation) will stick around until the next cross-site navigation, since
   // we are unable to tell when to destroy it.
   // See RenderViewHostManager::RendererAbortedProvisionalLoad.
-  if (!swap_needed || info->is_download() ||
+  if (!swap_needed || info->IsDownload() ||
       (response->head.headers.get() &&
        response->head.headers->response_code() == 204)) {
     return next_handler_->OnResponseStarted(request_id, response, defer);
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index 5f6733d..84cfb57 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -429,7 +429,7 @@ void ResourceDispatcherHostImpl::CancelRequestsForContext(
     // away. It may be OK for this invariant to change in the future, but if
     // this assertion fires without the invariant changing, then it's indicative
     // of a leak.
-    DCHECK((*i)->GetRequestInfo()->is_download() ||
+    DCHECK((*i)->GetRequestInfo()->IsDownload() ||
            (*i)->GetRequestInfo()->is_stream() ||
            ((*i)->GetRequestInfo()->detachable_handler() &&
             (*i)->GetRequestInfo()->detachable_handler()->is_detached()) ||
@@ -1387,10 +1387,9 @@ void ResourceDispatcherHostImpl::CancelRequestsForRoute(int child_id,
     // Don't cancel navigations that are expected to live beyond this process.
     if (IsTransferredNavigation(id))
       any_requests_transferring = true;
-
     if (info->detachable_handler()) {
       info->detachable_handler()->Detach();
-    } else if (!info->is_download() && !info->is_stream() &&
+    } else if (!info->IsDownload() && !info->is_stream() &&
                !IsTransferredNavigation(id) &&
                (route_id == -1 || route_id == info->GetRouteID())) {
       matching_requests.push_back(id);
diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc
index 4993c82..01f95a1 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -437,7 +437,7 @@ void ResourceLoader::CancelRequestInternal(int error, bool from_renderer) {
   // WebKit will send us a cancel for downloads since it no longer handles
   // them.  In this case, ignore the cancel since we handle downloads in the
   // browser.
-  if (from_renderer && (info->is_download() || info->is_stream()))
+  if (from_renderer && (info->IsDownload() || info->is_stream()))
     return;
 
   if (from_renderer && info->detachable_handler()) {
diff --git a/content/browser/loader/resource_request_info_impl.cc b/content/browser/loader/resource_request_info_impl.cc
index ddd4f5e..3d9ba35 100644
--- a/content/browser/loader/resource_request_info_impl.cc
+++ b/content/browser/loader/resource_request_info_impl.cc
@@ -218,6 +218,10 @@ bool ResourceRequestInfoImpl::IsAsync() const {
   return is_async_;
 }
 
+bool ResourceRequestInfoImpl::IsDownload() const {
+  return is_download_;
+}
+
 void ResourceRequestInfoImpl::AssociateWithRequest(net::URLRequest* request) {
   request->SetUserData(NULL, this);
   int render_process_id;
diff --git a/content/browser/loader/resource_request_info_impl.h b/content/browser/loader/resource_request_info_impl.h
index 0db6259..093c22f 100644
--- a/content/browser/loader/resource_request_info_impl.h
+++ b/content/browser/loader/resource_request_info_impl.h
@@ -80,6 +80,7 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo,
   virtual bool GetAssociatedRenderView(int* render_process_id,
                                        int* render_view_id) const OVERRIDE;
   virtual bool IsAsync() const OVERRIDE;
+  virtual bool IsDownload() const OVERRIDE;
 
 
   CONTENT_EXPORT void AssociateWithRequest(net::URLRequest* request);
@@ -134,7 +135,6 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo,
   bool allow_download() const { return allow_download_; }
 
   // Whether this is a download.
-  bool is_download() const { return is_download_; }
   void set_is_download(bool download) { is_download_ = download; }
 
   // Whether this is a stream.
diff --git a/content/public/browser/resource_request_info.h b/content/public/browser/resource_request_info.h
index c2e87a1..dfa0edd 100644
--- a/content/public/browser/resource_request_info.h
+++ b/content/public/browser/resource_request_info.h
@@ -100,6 +100,9 @@ class ResourceRequestInfo {
   // Returns true if this is associated with an asynchronous request.
   virtual bool IsAsync() const = 0;
 
+  // Whether this is a download.
+  virtual bool IsDownload() const = 0;
+
  protected:
   virtual ~ResourceRequestInfo() {}
 };