Fix front-end host creation upon navigation

- when navigating, add host bindings to the pending frame rather than old frame;
- force renderer swap if front-end URL is invalid;
- move front-end URL validation to DevToolsUIBindingds

This also re-lands https://codereview.chromium.org/2607833002

BUG=662859,678035

Review-Url: https://codereview.chromium.org/2620153002
Cr-Commit-Position: refs/heads/master@{#442781}

chrome/browser/devtools/BUILD.gn

@@ -130,6 +130,8 @@ static_library("devtools") {
       "global_confirm_info_bar.h",
       "remote_debugging_server.cc",
       "remote_debugging_server.h",
+      "url_constants.cc",
+      "url_constants.h",
     ]
     if (enable_service_discovery) {
       sources += [

chrome/browser/devtools/devtools_ui_bindings.h

@@ -29,6 +29,7 @@ class Profile;
 class PortForwardingStatusSerializer;
 
 namespace content {
+class NavigationHandle;
 class WebContents;
 }
 
@@ -42,6 +43,9 @@ class DevToolsUIBindings : public DevToolsEmbedderMessageDispatcher::Delegate,
   static DevToolsUIBindings* ForWebContents(
       content::WebContents* web_contents);
 
+  static GURL SanitizeFrontendURL(const GURL& url);
+  static bool IsValidFrontendURL(const GURL& url);
+
   class Delegate {
    public:
     virtual ~Delegate() {}
@@ -198,6 +202,7 @@ class DevToolsUIBindings : public DevToolsEmbedderMessageDispatcher::Delegate,
   typedef base::Callback<void(bool)> InfoBarCallback;
   void ShowDevToolsConfirmInfoBar(const base::string16& message,
                                   const InfoBarCallback& callback);
+  void UpdateFrontendHost(content::NavigationHandle* navigation_handle);
 
   // Extensions support.
   void AddDevToolsExtensionsToClient();

chrome/browser/ui/webui/devtools_ui_unittest.cc → chrome/browser/devtools/devtools_ui_bindings_unittest.cc

@@ -2,13 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/webui/devtools_ui.h"
+#include "chrome/browser/devtools/devtools_ui_bindings.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-class DevToolsUITest : public testing::Test {
+class DevToolsUIBindingsTest : public testing::Test {
 };
 
-TEST_F(DevToolsUITest, SanitizeFrontendURL) {
+TEST_F(DevToolsUIBindingsTest, SanitizeFrontendURL) {
   std::vector<std::pair<std::string, std::string>> tests = {
     {"random-string",
      "chrome-devtools://devtools/"},
@@ -96,7 +96,7 @@ TEST_F(DevToolsUITest, SanitizeFrontendURL) {
 
   for (const auto& pair : tests) {
     GURL url = GURL(pair.first);
-    url = DevToolsUI::SanitizeFrontendURL(url);
+    url = DevToolsUIBindings::SanitizeFrontendURL(url);
     EXPECT_EQ(pair.second, url.spec());
   }
 }

chrome/browser/devtools/devtools_window.cc

@@ -926,7 +926,7 @@ GURL DevToolsWindow::GetDevToolsURL(Profile* profile,
     url_string += "&can_dock=true";
   if (panel.size())
     url_string += "&panel=" + panel;
-  return DevToolsUI::SanitizeFrontendURL(GURL(url_string));
+  return DevToolsUIBindings::SanitizeFrontendURL(GURL(url_string));
 }
 
 // static

chrome/browser/devtools/url_constants.cc

+// Copyright 2016 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 "chrome/browser/devtools/url_constants.h"
+
+const char kRemoteFrontendDomain[] = "chrome-devtools-frontend.appspot.com";
+const char kRemoteFrontendBase[] =
+    "https://chrome-devtools-frontend.appspot.com/";
+const char kRemoteFrontendPath[] = "serve_file";

chrome/browser/devtools/url_constants.h

+// Copyright 2016 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.
+
+#ifndef CHROME_BROWSER_DEVTOOLS_URL_CONSTANTS_H_
+#define CHROME_BROWSER_DEVTOOLS_URL_CONSTANTS_H_
+
+extern const char kRemoteFrontendDomain[];
+extern const char kRemoteFrontendBase[];
+extern const char kRemoteFrontendPath[];
+
+#endif  // CHROME_BROWSER_DEVTOOLS_URL_CONSTANTS_H_

chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc

@@ -13,6 +13,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/about_flags.h"
+#include "chrome/browser/devtools/devtools_ui_bindings.h"
 #include "chrome/browser/dom_distiller/dom_distiller_service_factory.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -525,9 +526,11 @@ WebUIFactoryFunction GetWebUIFactoryFunction(WebUI* web_ui,
     return &NewWebUI<VrShellUIUI>;
 #endif
 #else
-  if (url.SchemeIs(content::kChromeDevToolsScheme))
+  if (url.SchemeIs(content::kChromeDevToolsScheme)) {
+    if (!DevToolsUIBindings::IsValidFrontendURL(url))
+      return nullptr;
     return &NewWebUI<DevToolsUI>;
-
+  }
   // chrome://inspect isn't supported on Android nor iOS. Page debugging is
   // handled by a remote devtools on the host machine, and other elements, i.e.
   // extensions aren't supported.

chrome/browser/ui/webui/devtools_ui.cc

@@ -7,9 +7,9 @@
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted_memory.h"
-#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "chrome/browser/devtools/url_constants.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
@@ -21,10 +21,8 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/common/user_agent.h"
-#include "net/base/escape.h"
 #include "net/base/filename_util.h"
 #include "net/base/load_flags.h"
-#include "net/base/url_util.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -41,10 +39,6 @@ std::string PathWithoutParams(const std::string& path) {
       .path().substr(1);
 }
 
-const char kRemoteFrontendDomain[] = "chrome-devtools-frontend.appspot.com";
-const char kRemoteFrontendBase[] =
-    "https://chrome-devtools-frontend.appspot.com/";
-const char kRemoteFrontendPath[] = "serve_file";
 const char kHttpNotFound[] = "HTTP/1.1 404 Not Found\n\n";
 
 #if BUILDFLAG(DEBUG_DEVTOOLS)
@@ -57,131 +51,6 @@ const char kFallbackFrontendURL[] =
     "data:text/plain,Cannot load DevTools frontend from an untrusted origin";
 #endif  // BUILDFLAG(DEBUG_DEVTOOLS)
 
-GURL SanitizeFrontendURL(
-    const GURL& url,
-    const std::string& scheme,
-    const std::string& host,
-    const std::string& path,
-    bool allow_query);
-
-std::string SanitizeRevision(const std::string& revision) {
-  for (size_t i = 0; i < revision.length(); i++) {
-    if (!(revision[i] == '@' && i == 0)
-        && !(revision[i] >= '0' && revision[i] <= '9')
-        && !(revision[i] >= 'a' && revision[i] <= 'z')
-        && !(revision[i] >= 'A' && revision[i] <= 'Z')) {
-      return std::string();
-    }
-  }
-  return revision;
-}
-
-std::string SanitizeFrontendPath(const std::string& path) {
-  for (size_t i = 0; i < path.length(); i++) {
-    if (path[i] != '/' && path[i] != '-' && path[i] != '_'
-        && path[i] != '.' && path[i] != '@'
-        && !(path[i] >= '0' && path[i] <= '9')
-        && !(path[i] >= 'a' && path[i] <= 'z')
-        && !(path[i] >= 'A' && path[i] <= 'Z')) {
-      return std::string();
-    }
-  }
-  return path;
-}
-
-std::string SanitizeEndpoint(const std::string& value) {
-  if (value.find('&') != std::string::npos
-      || value.find('?') != std::string::npos)
-    return std::string();
-  return value;
-}
-
-std::string SanitizeRemoteBase(const std::string& value) {
-  GURL url(value);
-  std::string path = url.path();
-  std::vector<std::string> parts = base::SplitString(
-      path, "/", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
-  std::string revision = parts.size() > 2 ? parts[2] : "";
-  revision = SanitizeRevision(revision);
-  path = base::StringPrintf("/%s/%s/", kRemoteFrontendPath, revision.c_str());
-  return SanitizeFrontendURL(url, url::kHttpsScheme,
-                             kRemoteFrontendDomain, path, false).spec();
-}
-
-std::string SanitizeRemoteFrontendURL(const std::string& value) {
-  GURL url(net::UnescapeURLComponent(value,
-      net::UnescapeRule::SPACES | net::UnescapeRule::PATH_SEPARATORS |
-      net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS |
-      net::UnescapeRule::REPLACE_PLUS_WITH_SPACE));
-  std::string path = url.path();
-  std::vector<std::string> parts = base::SplitString(
-      path, "/", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
-  std::string revision = parts.size() > 2 ? parts[2] : "";
-  revision = SanitizeRevision(revision);
-  std::string filename = parts.size() ? parts[parts.size() - 1] : "";
-  if (filename != "devtools.html")
-    filename = "inspector.html";
-  path = base::StringPrintf("/serve_rev/%s/%s",
-                            revision.c_str(), filename.c_str());
-  std::string sanitized = SanitizeFrontendURL(url, url::kHttpsScheme,
-      kRemoteFrontendDomain, path, true).spec();
-  return net::EscapeQueryParamValue(sanitized, false);
-}
-
-std::string SanitizeFrontendQueryParam(
-    const std::string& key,
-    const std::string& value) {
-  // Convert boolean flags to true.
-  if (key == "can_dock" || key == "debugFrontend" || key == "experiments" ||
-      key == "isSharedWorker" || key == "v8only" || key == "remoteFrontend")
-    return "true";
-
-  // Pass connection endpoints as is.
-  if (key == "ws" || key == "service-backend")
-    return SanitizeEndpoint(value);
-
-  // Only support undocked for old frontends.
-  if (key == "dockSide" && value == "undocked")
-    return value;
-
-  if (key == "panel" && (value == "elements" || value == "console"))
-    return value;
-
-  if (key == "remoteBase")
-    return SanitizeRemoteBase(value);
-
-  if (key == "remoteFrontendUrl")
-    return SanitizeRemoteFrontendURL(value);
-
-  return std::string();
-}
-
-GURL SanitizeFrontendURL(
-    const GURL& url,
-    const std::string& scheme,
-    const std::string& host,
-    const std::string& path,
-    bool allow_query) {
-  std::vector<std::string> query_parts;
-  if (allow_query) {
-    for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) {
-      std::string value = SanitizeFrontendQueryParam(it.GetKey(),
-          it.GetValue());
-      if (!value.empty()) {
-        query_parts.push_back(
-            base::StringPrintf("%s=%s", it.GetKey().c_str(), value.c_str()));
-      }
-    }
-  }
-  std::string query =
-      query_parts.empty() ? "" : "?" + base::JoinString(query_parts, "&");
-  std::string constructed = base::StringPrintf("%s://%s%s%s",
-      scheme.c_str(), host.c_str(), path.c_str(), query.c_str());
-  GURL result = GURL(constructed);
-  if (!result.is_valid())
-    return GURL();
-  return result;
-}
 
 // DevToolsDataSource ---------------------------------------------------------
 
@@ -429,30 +298,20 @@ GURL DevToolsUI::GetRemoteBaseURL() {
       content::GetWebKitRevision().c_str()));
 }
 
-// static
-GURL DevToolsUI::SanitizeFrontendURL(const GURL& url) {
-  return ::SanitizeFrontendURL(url, content::kChromeDevToolsScheme,
-      chrome::kChromeUIDevToolsHost, SanitizeFrontendPath(url.path()), true);
-}
-
 DevToolsUI::DevToolsUI(content::WebUI* web_ui)
-    : WebUIController(web_ui) {
+    : WebUIController(web_ui), bindings_(web_ui->GetWebContents()) {
   web_ui->SetBindings(0);
   Profile* profile = Profile::FromWebUI(web_ui);
   content::URLDataSource::Add(
       profile,
       new DevToolsDataSource(profile->GetRequestContext()));
 
-  GURL url = web_ui->GetWebContents()->GetVisibleURL();
-  if (url.spec() != SanitizeFrontendURL(url).spec())
+  if (!profile->IsOffTheRecord())
     return;
-
-  if (profile->IsOffTheRecord()) {
-    GURL site = content::SiteInstance::GetSiteForURL(profile, url);
-    content::BrowserContext::GetStoragePartitionForSite(profile, site)->
-        GetFileSystemContext()->EnableTemporaryFileSystemInIncognito();
-  }
-  bindings_.reset(new DevToolsUIBindings(web_ui->GetWebContents()));
+  GURL url = web_ui->GetWebContents()->GetVisibleURL();
+  GURL site = content::SiteInstance::GetSiteForURL(profile, url);
+  content::BrowserContext::GetStoragePartitionForSite(profile, site)->
+      GetFileSystemContext()->EnableTemporaryFileSystemInIncognito();
 }
 
 DevToolsUI::~DevToolsUI() {

chrome/browser/ui/webui/devtools_ui.h

@@ -5,9 +5,6 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_DEVTOOLS_UI_H_
 #define CHROME_BROWSER_UI_WEBUI_DEVTOOLS_UI_H_
 
-#include <memory>
-
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "chrome/browser/devtools/devtools_ui_bindings.h"
 #include "content/public/browser/web_ui_controller.h"
@@ -16,13 +13,12 @@ class DevToolsUI : public content::WebUIController {
  public:
   static GURL GetProxyURL(const std::string& frontend_url);
   static GURL GetRemoteBaseURL();
-  static GURL SanitizeFrontendURL(const GURL& url);
 
   explicit DevToolsUI(content::WebUI* web_ui);
   ~DevToolsUI() override;
 
  private:
-  std::unique_ptr<DevToolsUIBindings> bindings_;
+  DevToolsUIBindings bindings_;
   DISALLOW_COPY_AND_ASSIGN(DevToolsUI);
 };
 

chrome/test/BUILD.gn

@@ -3708,6 +3708,7 @@ test("unit_tests") {
 
   if (!is_ios && !is_android) {
     sources += [
+      "../browser/devtools/devtools_ui_bindings_unittest.cc",
       "../browser/download/download_dir_policy_handler_unittest.cc",
       "../browser/lifetime/keep_alive_registry_unittest.cc",
       "../browser/renderer_context_menu/render_view_context_menu_test_util.cc",
@@ -3716,7 +3717,6 @@ test("unit_tests") {
       "../browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc",
       "../browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc",
       "../browser/ui/passwords/manage_passwords_ui_controller_unittest.cc",
-      "../browser/ui/webui/devtools_ui_unittest.cc",
     ]
     deps += [
       ":unit_tests_js",