Do not trigger CrossSiteResourceHandler for streams or user certs.

content/browser/loader/cross_site_resource_handler.cc

 // Copyright (c) 2012 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/loader/cross_site_resource_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 143 matching lines @@
   // request.
   //
   // Similarly, HTTP 204 (No Content) responses leave us showing the previous
   // page.  We should allow the navigation to finish without running the unload
   // handler or swapping in the pending RenderViewHost.
   //
   // In both cases, any pending RenderViewHost (if one was created for this
   // navigation) will stick around until the next cross-site navigation, since
   // we are unable to tell when to destroy it.
   // See RenderFrameHostManager::RendererAbortedProvisionalLoad.
-  if (!swap_needed || info->IsDownload() ||
+  if (!swap_needed || info->IsDownload() || info->is_stream() ||

[davidben, 2014/02/12 19:57:56]

It's possible we should just set the IsDownload bit on streams too. I went with
this just because ResourceLoader::CancelRequestInternal also checks both.

[Zachary Kuznia, 2014/02/12 22:04:48]

IsDownload and is_stream should be mutually exclusive, so this should be the
right approach.

[davidben, 2014/02/12 22:18:56]

Right, but the question is whether consumers of the ResourceRequestInfo should
care about the distinction. Maybe we should remove is_stream() and always set
IsDownload() when the BRH detaches the handler. The current approach is
certainly more error-prone, as this bug demonstrates.

Checking on codesearch, everything that checks for is_stream() also checks for
IsDownload(). Everything that checks for IsDownload() also checks for
is_stream() except this code (a bug) and AppUrlRedirector which... couldn't
check is_stream() if it wanted to because it's not exported. But it doesn't care
either way because it only queries when a request starts. Haven't looked closely
at what it does and whether it should care.

[Charlie Reis, 2014/02/13 01:45:07]

I agree that the current approach is error prone.  It might be nice to have a
method that covers these similar cases if there's a concept that be used to
describe all of them (or their inverse, something like "IsDocument" or
"WillCommit").

In this case, whether the network response will commit as a new navigation is
the distinction the code is looking for.  Do the other callers of IsDownload()
and is_stream() need to care about 204 responses, since they lead to a similar
no-new-commit outcome?

[davidben, 2014/02/13 03:48:28]

The other callers seem to be more interested in the detaching aspect. There's
logic to allow them to outlive the route, and logic to ignore the renderer's
cancellations. And there's AppUrlRedirector which I still need to look at.

Calling a stream a "download" seems fairly sound to me. I'm already lumping
x-x509-user-cert here under "download" on grounds that it's a download with a
funny handler. Could we claim a stream is one too? Maybe Download isn't the best
word... "detached"? Detached is already used for DetachableResourceHandler, but
it's pretty similar actually. Maybe we could unifying those codepaths too, I
dunno. "Redirected" is the other word that comes to mind, but that also already
means something else. :-)

[Charlie Reis, 2014/02/13 19:33:05]

I'll defer to networking folks on that decision, since I don't have a great
sense for the behavior for streams, certificates, or what detaching means.  I'm
ok with lumping it all as downloads unless there's a better name for it. 
Certainly makes sense to do that as a followup CL.

       (response->head.headers.get() &&
        response->head.headers->response_code() == 204)) {
     return next_handler_->OnResponseStarted(request_id, response, defer);
   }
 
   // Now that we know a swap is needed and we have something to commit, we
   // pause to let the UI thread run the unload handler of the previous page
   // and set up a transfer if needed.
   StartCrossSiteTransition(request_id, response, should_transfer);
 
@@ skipping 135 matching lines @@
 }
 
 void CrossSiteResourceHandler::ResumeIfDeferred() {
   if (did_defer_) {
     did_defer_ = false;
     controller()->Resume();
   }
 }
 
 }  // namespace content