PlzNavigate: properly update NavigationHandle for 204/205 and download responses

content/browser/loader/navigation_resource_handler.cc

Index: content/browser/loader/navigation_resource_handler.cc
diff --git a/content/browser/loader/navigation_resource_handler.cc b/content/browser/loader/navigation_resource_handler.cc
index dc73b854bea71788845ddf9f8114a42eb1427d92..384248bbc2ddef24fdc733c23d343ed9f9f0fd86 100644
--- a/content/browser/loader/navigation_resource_handler.cc
+++ b/content/browser/loader/navigation_resource_handler.cc
@@ -94,16 +94,6 @@ bool NavigationResourceHandler::OnResponseStarted(ResourceResponse* response,
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
-  // If the MimeTypeResourceHandler intercepted this request and converted it
-  // into a download, it will still call OnResponseStarted and immediately
-  // cancel. Ignore the call; OnReadCompleted will happen shortly.
-  //
-  // TODO(davidben): Move the dispatch out of MimeTypeResourceHandler. Perhaps
-  // all the way to the UI thread. Downloads, user certificates, etc., should be
-  // dispatched at the navigation layer.
-  if (info->IsDownload())
-    return true;
-
   StreamContext* stream_context =
       GetStreamContextForResourceContext(info->GetContext());
   writer_.InitializeStream(stream_context->registry(),
@@ -129,14 +119,15 @@ bool NavigationResourceHandler::OnResponseStarted(ResourceResponse* response,
   }
 
   core_->NotifyResponseStarted(response, writer_.stream()->CreateHandle(),
-                               ssl_status, std::move(cloned_data));
-  // Don't defer stream based requests. This includes requests initiated via
-  // mime type sniffing, etc.
+                               ssl_status, std::move(cloned_data),
+                               info->IsDownload());
+  // Don't defer stream based or download requests. This includes requests
+  // initiated via mime type sniffing, etc.
   // TODO(ananta)
   // Make sure that the requests go through the throttle checks. Currently this
   // does not work as the InterceptingResourceHandler is above us and hence it
   // does not expect the old handler to defer the request.
-  if (!info->is_stream())
+  if (!info->is_stream() && !info->IsDownload())

[clamy, 2016/11/25 10:46:00]

I think we should call DetachFromCore if the request is a download at this
point. Otherwise we may end up trying to cancel the request if the
NavigationURLLoader is deleted.

[yzshen1, 2016/11/28 22:03:59]

In practice it won't cause trouble because before returning to the message loop
this NavigationResourceHandler object will be destructed -> it will try to
notify the UI thread about the request failure -> when that task is run on the
UI thread, the NavigationURLLoaderImpl has already been destroyed so it is a
no-op.

I agree that it is better not to rely on that. Thanks for the suggestion!

     *defer = true;
   return true;
 }