Remove code duplication for copying OpenURLParams into LoadURLParams.

There was quite a bit of code duplication related to copying
OpenURLParams into LoadURLParams.  Some callsites didn't copy all the
navigation properties (leading to the DCHECK reported in
https://crbug.com/1007041 and possibly also to some of renderer
kills reported in https://crbug.com/1014483).

This CL tries to improve the situation, by moving the copying code into
a new constructor of LoadURLParams and reusing the constructor from as
many places as possible.

Bug: 1007041, 1014483
Change-Id: Ibeefd97118a78f5a2b3779e51d83731c546fd113
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1853910
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709132}

chrome/browser/devtools/devtools_window.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/devtools/devtools_window.h"
 
 #include <algorithm>
+#include <set>
 #include <utility>
 
 #include "base/base64.h"
@@ -170,8 +171,8 @@ content::WebContents* DevToolsToolboxDelegate::OpenURLFromTab(
   DCHECK(source == web_contents());
   if (!params.url.SchemeIs(content::kChromeDevToolsScheme))
     return NULL;
-  content::NavigationController::LoadURLParams load_url_params(params.url);
-  source->GetController().LoadURLWithParams(load_url_params);
+  source->GetController().LoadURLWithParams(
+      content::NavigationController::LoadURLParams(params));
   return source;
 }
 
@@ -218,7 +219,7 @@ GURL DecorateFrontendURL(const GURL& base_url) {
   std::string url_string(
       frontend_url +
       ((frontend_url.find("?") == std::string::npos) ? "?" : "&") +
-      "dockSide=undocked"); // TODO(dgozman): remove this support in M38.
+      "dockSide=undocked");  // TODO(dgozman): remove this support in M38.
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kEnableDevToolsExperiments))
     url_string += "&experiments=true";
@@ -810,7 +811,7 @@ void DevToolsWindow::Show(const DevToolsToggleAction& action) {
                                     &inspected_browser,
                                     &inspected_tab_index);
     DCHECK(inspected_browser);
-    DCHECK(inspected_tab_index != -1);
+    DCHECK_NE(-1, inspected_tab_index);
 
     RegisterModalDialogManager(inspected_browser);
 

chrome/browser/devtools/devtools_window.h

@@ -5,6 +5,9 @@
 #ifndef CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_
 #define CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_
 
+#include <memory>
+#include <string>
+
 #include "base/macros.h"
 #include "chrome/browser/devtools/devtools_contents_resizing_strategy.h"
 #include "chrome/browser/devtools/devtools_toggle_action.h"

chrome/browser/ui/ash/keyboard/chrome_keyboard_web_contents.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
 #include "chrome/browser/ui/ash/keyboard/chrome_keyboard_bounds_observer.h"
 #include "content/public/browser/host_zoom_map.h"
+#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
@@ -47,8 +48,8 @@ class ChromeKeyboardContentsDelegate : public content::WebContentsDelegate,
   content::WebContents* OpenURLFromTab(
       content::WebContents* source,
       const content::OpenURLParams& params) override {
-    source->GetController().LoadURL(params.url, params.referrer,
-                                    params.transition, params.extra_headers);
+    source->GetController().LoadURLWithParams(
+        content::NavigationController::LoadURLParams(params));
     Observe(source);
     return source;
   }

chrome/browser/ui/input_method/ime_window.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/input_method/ime_native_window.h"
 #include "chrome/browser/ui/input_method/ime_window_observer.h"
+#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -164,8 +165,8 @@ void ImeWindow::Observe(int type,
 content::WebContents* ImeWindow::OpenURLFromTab(
     content::WebContents* source,
     const content::OpenURLParams& params) {
-  source->GetController().LoadURL(params.url, params.referrer,
-                                  params.transition, params.extra_headers);
+  source->GetController().LoadURLWithParams(
+      content::NavigationController::LoadURLParams(params));
   return source;
 }
 

components/embedder_support/android/delegate/web_contents_delegate_android.cc

@@ -101,25 +101,8 @@ WebContents* WebContentsDelegateAndroid::OpenURLFromTab(
     return NULL;
   }
 
-  // content::OpenURLParams -> content::NavigationController::LoadURLParams
-  content::NavigationController::LoadURLParams load_params(url);
-  load_params.referrer = params.referrer;
-  load_params.frame_tree_node_id = params.frame_tree_node_id;
-  load_params.redirect_chain = params.redirect_chain;
-  load_params.transition_type = params.transition;
-  load_params.extra_headers = params.extra_headers;
-  load_params.should_replace_current_entry =
-      params.should_replace_current_entry;
-  load_params.is_renderer_initiated = params.is_renderer_initiated;
-  load_params.has_user_gesture = params.user_gesture;
-  load_params.initiator_origin = params.initiator_origin;
-
-  if (params.uses_post) {
-    load_params.load_type = content::NavigationController::LOAD_TYPE_HTTP_POST;
-    load_params.post_data = params.post_data;
-  }
-
-  source->GetController().LoadURLWithParams(load_params);
+  source->GetController().LoadURLWithParams(
+      content::NavigationController::LoadURLParams(params));
 
   return source;
 }

content/public/browser/navigation_controller.cc

@@ -6,28 +6,68 @@
 
 #include "base/memory/ref_counted_memory.h"
 #include "build/build_config.h"
+#include "content/public/browser/page_navigator.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/common/was_activated_option.mojom.h"
 
 namespace content {
 
 NavigationController::LoadURLParams::LoadURLParams(const GURL& url)
-    : url(url),
-      load_type(LOAD_TYPE_DEFAULT),
-      transition_type(ui::PAGE_TRANSITION_LINK),
-      frame_tree_node_id(RenderFrameHost::kNoFrameTreeNodeId),
-      is_renderer_initiated(false),
-      override_user_agent(UA_OVERRIDE_INHERIT),
-      post_data(nullptr),
-      can_load_local_resources(false),
-      should_replace_current_entry(false),
-      has_user_gesture(false),
-      should_clear_history_list(false),
-      started_from_context_menu(false),
-      navigation_ui_data(nullptr),
-      from_download_cross_origin_redirect(false),
-      was_activated(mojom::WasActivatedOption::kUnknown),
-      reload_type(ReloadType::NONE) {}
+    : url(url), is_renderer_initiated(false) {}
+
+NavigationController::LoadURLParams::LoadURLParams(const OpenURLParams& input)
+    : url(input.url),
+      initiator_origin(input.initiator_origin),
+      source_site_instance(input.source_site_instance),
+      load_type(input.uses_post ? LOAD_TYPE_HTTP_POST : LOAD_TYPE_DEFAULT),
+      transition_type(input.transition),
+      frame_tree_node_id(input.frame_tree_node_id),
+      referrer(input.referrer),
+      redirect_chain(input.redirect_chain),
+      extra_headers(input.extra_headers),
+      is_renderer_initiated(input.is_renderer_initiated),
+      post_data(input.post_data),
+      should_replace_current_entry(input.should_replace_current_entry),
+      has_user_gesture(input.user_gesture),
+      started_from_context_menu(input.started_from_context_menu),
+      blob_url_loader_factory(input.blob_url_loader_factory),
+      href_translate(input.href_translate),
+      reload_type(input.reload_type) {
+  // |post_data| should be present iff |uses_post| is true.
+  //
+  // TODO(lukasza): Consider removing |uses_post| (redundant with |post_data|).
+  DCHECK_EQ(input.uses_post, !!input.post_data);
+
+  // A non-null |source_site_instance| is important for picking the right
+  // renderer process for hosting about:blank and/or data: URLs (their origin's
+  // precursor is based on |initiator_origin|).
+  if (url.IsAboutBlank() || url.SchemeIs(url::kDataScheme)) {
+    DCHECK_EQ(initiator_origin.has_value(),
+              static_cast<bool>(source_site_instance));
+  }
+
+  // Implementation notes:
+  //   The following LoadURLParams don't have an equivalent in OpenURLParams:
+  //     base_url_for_data_url
+  //     virtual_url_for_data_url
+  //     data_url_as_string
+  //
+  //     can_load_local_resources
+  //     frame_name
+  //     from_download_cross_origin_redirect
+  //     input_start
+  //     navigation_ui_data
+  //     override_user_agent
+  //     should_clear_history_list
+  //     was_activated
+  //
+  //   The following OpenURLParams don't have an equivalent in LoadURLParams:
+  //     disposition
+  //     open_app_window_if_possible
+  //     source_render_frame_id
+  //     source_render_process_id
+  //     triggering_event_info
+}
 
 NavigationController::LoadURLParams::~LoadURLParams() {
 }

content/public/browser/navigation_controller.h

@@ -20,6 +20,7 @@
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/reload_type.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/restore_type.h"
 #include "content/public/browser/session_storage_namespace.h"
 #include "content/public/browser/site_instance.h"
@@ -44,6 +45,7 @@ class BrowserContext;
 class NavigationEntry;
 class RenderFrameHost;
 class WebContents;
+struct OpenURLParams;
 
 // A NavigationController maintains the back-forward list for a WebContents and
 // manages all navigation within that list.
@@ -132,15 +134,15 @@ class NavigationController {
     scoped_refptr<SiteInstance> source_site_instance;
 
     // See LoadURLType comments above.
-    LoadURLType load_type;
+    LoadURLType load_type = LOAD_TYPE_DEFAULT;
 
     // PageTransition for this load. See PageTransition for details.
     // Note the default value in constructor below.
-    ui::PageTransition transition_type;
+    ui::PageTransition transition_type = ui::PAGE_TRANSITION_LINK;
 
     // The browser-global FrameTreeNode ID for the frame to navigate, or
     // RenderFrameHost::kNoFrameTreeNodeId for the main frame.
-    int frame_tree_node_id;
+    int frame_tree_node_id = RenderFrameHost::kNoFrameTreeNodeId;
 
     // Referrer for this load. Empty if none.
     Referrer referrer;
@@ -158,7 +160,7 @@ class NavigationController {
 
     // User agent override for this load. See comments in
     // UserAgentOverrideOption definition.
-    UserAgentOverrideOption override_user_agent;
+    UserAgentOverrideOption override_user_agent = UA_OVERRIDE_INHERIT;
 
     // Used in LOAD_TYPE_DATA loads only. Used for specifying a base URL
     // for pages loaded via data URLs.
@@ -182,18 +184,18 @@ class NavigationController {
     scoped_refptr<network::ResourceRequestBody> post_data;
 
     // True if this URL should be able to access local resources.
-    bool can_load_local_resources;
+    bool can_load_local_resources = false;
 
     // Indicates whether this navigation should replace the current
     // navigation entry.
-    bool should_replace_current_entry;
+    bool should_replace_current_entry = false;
 
     // Used to specify which frame to navigate. If empty, the main frame is
     // navigated. This is currently only used in tests.
     std::string frame_name;
 
     // Indicates that the navigation was triggered by a user gesture.
-    bool has_user_gesture;
+    bool has_user_gesture = false;
 
     // Indicates that during this navigation, the session history should be
     // cleared such that the resulting page is the first and only entry of the
@@ -201,10 +203,10 @@ class NavigationController {
     //
     // The clearing is done asynchronously, and completes when this navigation
     // commits.
-    bool should_clear_history_list;
+    bool should_clear_history_list = false;
 
     // Indicates whether or not this navigation was initiated via context menu.
-    bool started_from_context_menu;
+    bool started_from_context_menu = false;
 
     // Optional URLLoaderFactory to facilitate navigation to a blob URL.
     scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory;
@@ -216,7 +218,7 @@ class NavigationController {
 
     // Whether this navigation was triggered by a x-origin redirect following a
     // prior (most likely <a download>) download attempt.
-    bool from_download_cross_origin_redirect;
+    bool from_download_cross_origin_redirect = false;
 
     // Time at which the input leading to this navigation occurred. This field
     // is set for links clicked by the user; the embedder is recommended to set
@@ -225,7 +227,8 @@ class NavigationController {
 
     // Set to |kYes| if the navigation should propagate user activation. This
     // is used by embedders where the activation has occurred outside the page.
-    mojom::WasActivatedOption was_activated;
+    mojom::WasActivatedOption was_activated =
+        mojom::WasActivatedOption::kUnknown;
 
     // If this navigation was initiated from a link that specified the
     // hrefTranslate attribute, this contains the attribute's value (a BCP47
@@ -236,6 +239,12 @@ class NavigationController {
     ReloadType reload_type = ReloadType::NONE;
 
     explicit LoadURLParams(const GURL& url);
+
+    // Copies |open_url_params| into LoadURLParams, attempting to copy all
+    // fields that are present in both structs (some properties are ignored
+    // because they are unique to LoadURLParams or OpenURLParams).
+    explicit LoadURLParams(const OpenURLParams& open_url_params);
+
     ~LoadURLParams();
 
     DISALLOW_COPY_AND_ASSIGN(LoadURLParams);

content/shell/browser/shell.cc

@@ -442,26 +442,8 @@ WebContents* Shell::OpenURLFromTab(WebContents* source,
       return nullptr;
   }
 
-  NavigationController::LoadURLParams load_url_params(params.url);
-  load_url_params.initiator_origin = params.initiator_origin;
-  load_url_params.source_site_instance = params.source_site_instance;
-  load_url_params.transition_type = params.transition;
-  load_url_params.frame_tree_node_id = params.frame_tree_node_id;
-  load_url_params.referrer = params.referrer;
-  load_url_params.redirect_chain = params.redirect_chain;
-  load_url_params.extra_headers = params.extra_headers;
-  load_url_params.is_renderer_initiated = params.is_renderer_initiated;
-  load_url_params.should_replace_current_entry =
-      params.should_replace_current_entry;
-  load_url_params.blob_url_loader_factory = params.blob_url_loader_factory;
-  load_url_params.reload_type = params.reload_type;
-
-  if (params.uses_post) {
-    load_url_params.load_type = NavigationController::LOAD_TYPE_HTTP_POST;
-    load_url_params.post_data = params.post_data;
-  }
-
-  target->GetController().LoadURLWithParams(load_url_params);
+  target->GetController().LoadURLWithParams(
+      NavigationController::LoadURLParams(params));
   return target;
 }
 
@@ -605,11 +587,10 @@ std::unique_ptr<WebContents> Shell::SwapWebContents(
   return new_contents;
 }
 
-bool Shell::ShouldAllowRunningInsecureContent(
-    content::WebContents* web_contents,
-    bool allowed_per_prefs,
-    const url::Origin& origin,
-    const GURL& resource_url) {
+bool Shell::ShouldAllowRunningInsecureContent(WebContents* web_contents,
+                                              bool allowed_per_prefs,
+                                              const url::Origin& origin,
+                                              const GURL& resource_url) {
   bool allowed_by_test = false;
   BlinkTestController* blink_test_controller = BlinkTestController::Get();
   if (blink_test_controller && switches::IsRunWebTestsSwitchPresent()) {
@@ -622,7 +603,7 @@ bool Shell::ShouldAllowRunningInsecureContent(
 }
 
 PictureInPictureResult Shell::EnterPictureInPicture(
-    content::WebContents* web_contents,
+    WebContents* web_contents,
     const viz::SurfaceId& surface_id,
     const gfx::Size& natural_size) {
   // During tests, returning success to pretend the window was created and allow

headless/lib/browser/headless_web_contents_impl.cc

@@ -20,6 +20,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_termination_info.h"
 #include "content/public/browser/devtools_agent_host.h"
+#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -154,26 +155,8 @@ class HeadlessWebContentsImpl::Delegate : public content::WebContentsDelegate {
         return nullptr;
     }
 
-    content::NavigationController::LoadURLParams load_url_params(params.url);
-    load_url_params.initiator_origin = params.initiator_origin;
-    load_url_params.source_site_instance = params.source_site_instance;
-    load_url_params.transition_type = params.transition;
-    load_url_params.frame_tree_node_id = params.frame_tree_node_id;
-    load_url_params.referrer = params.referrer;
-    load_url_params.redirect_chain = params.redirect_chain;
-    load_url_params.extra_headers = params.extra_headers;
-    load_url_params.is_renderer_initiated = params.is_renderer_initiated;
-    load_url_params.should_replace_current_entry =
-        params.should_replace_current_entry;
-    load_url_params.reload_type = params.reload_type;
-
-    if (params.uses_post) {
-      load_url_params.load_type =
-          content::NavigationController::LOAD_TYPE_HTTP_POST;
-      load_url_params.post_data = params.post_data;
-    }
-
-    target->GetController().LoadURLWithParams(load_url_params);
+    target->GetController().LoadURLWithParams(
+        content::NavigationController::LoadURLParams(params));
     return target;
   }