Block redirects to renderer-debug urls.

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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/frame_host/navigation_handle_impl.h"
 
 #include <iterator>
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
@@ skipping 106 matching lines @@
       is_download_(false),
       is_stream_(false),
       started_from_context_menu_(started_from_context_menu),
       reload_type_(ReloadType::NONE),
       restore_type_(RestoreType::NONE),
       navigation_type_(NAVIGATION_TYPE_UNKNOWN),
       should_check_main_world_csp_(should_check_main_world_csp),
       is_form_submission_(is_form_submission),
       expected_render_process_host_id_(ChildProcessHost::kInvalidUniqueID),
       weak_factory_(this) {
-  is_in_constructor = true;
   TRACE_EVENT_ASYNC_BEGIN2("navigation", "NavigationHandle", this,
                            "frame_tree_node",
                            frame_tree_node_->frame_tree_node_id(), "url",
                            url_.possibly_invalid_spec());
   DCHECK(!navigation_start.is_null());
 
   site_url_ = SiteInstance::GetSiteForURL(frame_tree_node_->current_frame_host()
                                               ->GetSiteInstance()
                                               ->GetBrowserContext(),
                                           url_);
@@ skipping 33 matching lines @@
   if (IsInMainFrame()) {
     TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1(
         "navigation", "Navigation StartToCommit", this,
         navigation_start, "Initial URL", url_.spec());
   }
 
   if (is_same_document_) {
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                  "Same document");
   }
-  is_in_constructor = false;
 }
 
 NavigationHandleImpl::~NavigationHandleImpl() {
-  // TODO(arthursonzogni): Remove this when we understand the root cause behind
-  // crbug.com/704892.
-  if (is_in_constructor)
-    base::debug::DumpWithoutCrashing();
-
   // Inform the RenderProcessHost to no longer expect a navigation.
   if (expected_render_process_host_id_ != ChildProcessHost::kInvalidUniqueID) {
     RenderProcessHost* process =
         RenderProcessHost::FromID(expected_render_process_host_id_);
     if (process) {
       RenderProcessHostImpl::RemoveExpectedNavigationToSite(
           frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
           process, site_url_);
     }
   }
@@ skipping 423 matching lines @@
     const GURL& new_referrer_url,
     bool new_is_external_protocol,
     scoped_refptr<net::HttpResponseHeaders> response_headers,
     net::HttpResponseInfo::ConnectionInfo connection_info,
     RenderProcessHost* post_redirect_process,
     const ThrottleChecksFinishedCallback& callback) {
   TRACE_EVENT_ASYNC_STEP_INTO1("navigation", "NavigationHandle", this,
                                "WillRedirectRequest", "url",
                                new_url.possibly_invalid_spec());
 
-  // TODO(arthursonzogni): Remove this when we understand the root cause behind
-  // crbug.com/704892 and crbug.com/736658
+  // |new_url| is not expected to be a "renderer debug" url. It should be
+  // blocked in NavigationRequest::OnRequestRedirected or in
+  // ResourceLoader::OnReceivedRedirect. If it is not the case,
+  // DidFinishNavigation will not be called. It could confuse some
+  // WebContentsObserver because DidStartNavigation was called.
   if (IsRendererDebugURL(new_url))
     base::debug::DumpWithoutCrashing();

[Charlie Reis, 2017/07/07 17:12:59]

We shouldn't leave DumpWithoutCrashing calls in the codebase.  We should either
have a TODO to remove this (as in the earlier case) or change it to a CHECK if
we really don't expect it to happen.

[arthursonzogni, 2017/07/10 16:07:04]

Done.

 
   // Update the navigation parameters.
   url_ = new_url;
   method_ = new_method;
   UpdateSiteURL(post_redirect_process);
 
   if (!(transition_ & ui::PAGE_TRANSITION_CLIENT_REDIRECT)) {
     sanitized_referrer_.url = new_referrer_url;
     sanitized_referrer_ =
         Referrer::SanitizeForRequest(url_, sanitized_referrer_);
@@ skipping 537 matching lines @@
   // Stop expecting a navigation to the current site URL in the current expected
   // process.
   SetExpectedProcess(nullptr);
 
   // Update the site URL and the expected process.
   site_url_ = new_site_url;
   SetExpectedProcess(post_redirect_process);
 }
 
 }  // namespace content