Fix missing DevTool message for cross-site subframe download
The issue: Devtools msg missing when (1)Devtools is open before page load &&
(2)the message is logged in OnResponseStarted::OnResponseStarted && (3)the
response is a download && (4)the frame logging the message is a cross-site
subframe
This is rooted from calling the detach_callback in
TargetAutoAttacher::AutoAttachToFrame. Specifically, for a navigation to
download that occurs in a cross-site subframe, in
TargetAutoAttacher::AutoAttachToFrame, the |old_cross_process| is always true,
and the |new_cross_process| is false because RFH was set to null for download in
OnResponseStarted::OnResponseStarted.
The simplest fix seems to be we skip detaching the host when it's not going to
commit by checking if RFH is nullptr.
This CL also has another small fix: the check for |is_download_| is changed to
|is_download| in 2 sites in NavigationRequest::OnResponseStarted, as in those
2 sites, it should check whether the navigation has turned into a download,
regardless whether he download was intervened or not.
Bug: 961486
Change-Id: I2cc08dfd02bfe4d097c2ed3a3f1b8cfe425b8f01
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614156
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Yao Xiao <yaoxia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661417}
diff --git a/content/browser/devtools/protocol/target_auto_attacher.cc b/content/browser/devtools/protocol/target_auto_attacher.cc
index 053ea04e..97a14a1 100644
--- a/content/browser/devtools/protocol/target_auto_attacher.cc
+++ b/content/browser/devtools/protocol/target_auto_attacher.cc
@@ -211,16 +211,23 @@
FrameTreeNode* frame_tree_node = navigation_handle->frame_tree_node();
RenderFrameHostImpl* new_host = navigation_handle->GetRenderFrameHost();
+
+ // |new_host| can be nullptr for navigation that doesn't commmit
+ // (e.g. download). Skip possibly detaching the old agent host so the DevTools
+ // message logged via the old RFH can be seen.
+ if (!new_host)
+ return nullptr;
+
scoped_refptr<DevToolsAgentHost> agent_host =
RenderFrameDevToolsAgentHost::FindForDangling(frame_tree_node);
bool old_cross_process = !!agent_host;
bool is_portal_main_frame =
- frame_tree_node->IsMainFrame() && new_host &&
+ frame_tree_node->IsMainFrame() &&
static_cast<WebContentsImpl*>(WebContents::FromRenderFrameHost(new_host))
->IsPortal();
bool new_cross_process =
- (new_host && new_host->IsCrossProcessSubframe()) || is_portal_main_frame;
+ new_host->IsCrossProcessSubframe() || is_portal_main_frame;
if (old_cross_process == new_cross_process)
return nullptr;
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index e89a2121..a0d29a2 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -1245,9 +1245,9 @@
// Check if the response should be sent to a renderer.
response_should_be_rendered_ =
- !is_download_ && (!response->head.headers.get() ||
- (response->head.headers->response_code() != 204 &&
- response->head.headers->response_code() != 205));
+ !is_download && (!response->head.headers.get() ||
+ (response->head.headers->response_code() != 204 &&
+ response->head.headers->response_code() != 205));
// Response that will not commit should be marked as aborted in the
// NavigationHandle.
@@ -1393,8 +1393,8 @@
// know how to display the content. We follow Firefox here and show our
// own error page instead of intercepting the request as a stream or a
// download.
- if (is_download_ && (response->head.headers.get() &&
- (response->head.headers->response_code() / 100 != 2))) {
+ if (is_download && (response->head.headers.get() &&
+ (response->head.headers->response_code() / 100 != 2))) {
OnRequestFailedInternal(
network::URLLoaderCompletionStatus(net::ERR_INVALID_RESPONSE),
false /* skip_throttles */, base::nullopt /* error_page_content */,
