Commit a4466478 authored by Lukasz Anforowicz's avatar Lukasz Anforowicz Committed by Commit Bot

Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see https://crbug.com/713888#c5).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship *iff* the new parameter - setSelfAsOpener is used.
Otherwise, the newly opened window is created in a separate browsing
instance.  This is required for later restricting named frame lookup
to the current browsing instance (see https://crbug.com/718489).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant automated tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

I have also verified that the Hangouts extension (current prod version
- 2017.1019.418.1) works fine before and after this CL (i.e. I have
verified that this CL doesn't regress https://crbug.com/597750).

Bug: 713888
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Test: See above.
Change-Id: If23ac7b72256bfcd695fc4bc3759a5bd73f4812f
Reviewed-on: https://chromium-review.googlesource.com/758801Reviewed-by: default avatarDevlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarCharlie Reis <creis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516081}
parent e634d9e7
......@@ -607,13 +607,12 @@ ExtensionFunction::ResponseAction WindowsCreateFunction::Run() {
ui::PAGE_TRANSITION_LINK);
navigate_params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
// The next 2 statements put the new contents in the same BrowsingInstance
// as their opener. Note that |force_new_process_for_new_contents = false|
// means that new contents might still end up in a new renderer
// (if they open a web URL and are transferred out of an extension
// renderer), but even in this case the flags below ensure findability via
// window.open.
navigate_params.force_new_process_for_new_contents = false;
// Depending on the |setSelfAsOpener| option, we need to put the new
// contents in the same BrowsingInstance as their opener. See also
// https://crbug.com/713888.
bool set_self_as_opener = create_data->set_self_as_opener && // present?
*create_data->set_self_as_opener; // set to true?
navigate_params.opener = set_self_as_opener ? render_frame_host() : nullptr;
navigate_params.source_site_instance =
render_frame_host()->GetSiteInstance();
......
......@@ -46,6 +46,7 @@
#include "content/public/common/page_zoom.h"
#include "content/public/common/url_constants.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/test_navigation_observer.h"
#include "extensions/browser/api_test_utils.h"
#include "extensions/browser/app_window/app_window.h"
#include "extensions/browser/app_window/app_window_registry.h"
......@@ -2178,10 +2179,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TemporaryAddressSpoof) {
EXPECT_EQ(url, second_web_contents->GetVisibleURL());
}
// Window created by chrome.windows.create should be in the same SiteInstance
// and BrowsingInstance as the opener - this is a regression test for
// https://crbug.com/597750.
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowsCreateVsSiteInstance) {
// Tests how chrome.windows.create behaves when setSelfAsOpener parameter is
// used. setSelfAsOpener was introduced as a fix for https://crbug.com/713888
// and https://crbug.com/718489. This is a (slightly morphed) regression test
// for https://crbug.com/597750.
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowsCreate_WithOpener) {
const extensions::Extension* extension =
LoadExtension(test_data_dir_.AppendASCII("../simple_with_file"));
ASSERT_TRUE(extension);
......@@ -2196,28 +2198,51 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowsCreateVsSiteInstance) {
content::WebContents* new_contents = nullptr;
{
content::WebContentsAddedObserver observer;
ASSERT_TRUE(content::ExecuteScript(old_contents,
"window.name = 'old-contents';\n"
"chrome.windows.create({url: '" +
extension_url.spec() + "'})"));
std::string script = base::StringPrintf(
R"( window.name = 'old-contents';
chrome.windows.create({url: '%s', setSelfAsOpener: true}); )",
extension_url.spec().c_str());
ASSERT_TRUE(content::ExecuteScript(old_contents, script));
new_contents = observer.GetWebContents();
ASSERT_TRUE(content::WaitForLoadStop(new_contents));
}
// Verify that the old and new tab are in the same process and SiteInstance.
// Note: both test assertions are important - one observed failure mode was
// having the same process, but different SiteInstance.
// Navigate the old and the new tab to a web URL.
ASSERT_TRUE(StartEmbeddedTestServer());
GURL web_url1 = embedded_test_server()->GetURL("/title1.html");
GURL web_url2 = embedded_test_server()->GetURL("/title2.html");
{
content::TestNavigationObserver nav_observer(new_contents, 1);
ASSERT_TRUE(content::ExecuteScript(
new_contents, "window.location = '" + web_url1.spec() + "';"));
nav_observer.Wait();
}
{
content::TestNavigationObserver nav_observer(old_contents, 1);
ASSERT_TRUE(content::ExecuteScript(
old_contents, "window.location = '" + web_url2.spec() + "';"));
nav_observer.Wait();
}
EXPECT_EQ(web_url1, new_contents->GetMainFrame()->GetLastCommittedURL());
EXPECT_EQ(web_url2, old_contents->GetMainFrame()->GetLastCommittedURL());
// Verify that the old and new tab are in the same process.
EXPECT_EQ(old_contents->GetMainFrame()->GetProcess(),
new_contents->GetMainFrame()->GetProcess());
EXPECT_EQ(old_contents->GetMainFrame()->GetSiteInstance(),
new_contents->GetMainFrame()->GetSiteInstance());
// Verify that the |new_contents| doesn't have a |window.opener| set.
bool window_opener_cast_to_bool = true;
EXPECT_TRUE(ExecuteScriptAndExtractBool(
new_contents, "window.domAutomationController.send(!!window.opener)",
&window_opener_cast_to_bool));
EXPECT_FALSE(window_opener_cast_to_bool);
// Verify the old and new contents are in the same BrowsingInstance.
EXPECT_TRUE(
old_contents->GetMainFrame()->GetSiteInstance()->IsRelatedSiteInstance(
new_contents->GetMainFrame()->GetSiteInstance()));
// Verify that the |new_contents| has |window.opener| set.
std::string location_of_opener;
EXPECT_TRUE(ExecuteScriptAndExtractString(
new_contents,
"window.domAutomationController.send(window.opener.location.href)",
&location_of_opener));
EXPECT_EQ(old_contents->GetMainFrame()->GetLastCommittedURL().spec(),
location_of_opener);
// Verify that |new_contents| can find |old_contents| using window.open/name.
std::string location_of_other_window;
......@@ -2230,4 +2255,47 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowsCreateVsSiteInstance) {
location_of_other_window);
}
// Tests how chrome.windows.create behaves when setSelfAsOpener parameter is not
// used. setSelfAsOpener was introduced as a fix for https://crbug.com/713888
// and https://crbug.com/718489.
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowsCreate_NoOpener) {
const Extension* extension =
LoadExtension(test_data_dir_.AppendASCII("../simple_with_file"));
ASSERT_TRUE(extension);
// Navigate a tab to an extension page.
GURL extension_url = extension->GetResourceURL("file.html");
ui_test_utils::NavigateToURL(browser(), extension_url);
content::WebContents* old_contents =
browser()->tab_strip_model()->GetActiveWebContents();
// Execute chrome.windows.create and store the new tab in |new_contents|.
content::WebContents* new_contents = nullptr;
{
content::WebContentsAddedObserver observer;
std::string script = base::StringPrintf(
R"( window.name = 'old-contents';
chrome.windows.create({url: '%s'}); )",
extension_url.spec().c_str());
ASSERT_TRUE(content::ExecuteScript(old_contents, script));
new_contents = observer.GetWebContents();
ASSERT_TRUE(content::WaitForLoadStop(new_contents));
}
// Verify the old and new contents are NOT in the same BrowsingInstance.
EXPECT_FALSE(
old_contents->GetMainFrame()->GetSiteInstance()->IsRelatedSiteInstance(
new_contents->GetMainFrame()->GetSiteInstance()));
// Verify that the |new_contents| doesn't have |window.opener| set.
bool opener_as_bool = true;
EXPECT_TRUE(ExecuteScriptAndExtractBool(
new_contents, "window.domAutomationController.send(!!window.opener)",
&opener_as_bool));
EXPECT_FALSE(opener_as_bool);
// TODO(lukasza): http://crbug.com/718489: Verify that |new_contents| can NOT
// find |old_contents| using window.open/name.
}
} // namespace extensions
......@@ -5,6 +5,8 @@
#include "chrome/browser/ui/browser_navigator.h"
#include <algorithm>
#include <memory>
#include <string>
#include "base/command_line.h"
#include "base/macros.h"
......@@ -32,6 +34,8 @@
#include "content/public/browser/browser_url_handler.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/browser/notification_service.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"
#include "content/public/browser/web_contents.h"
#include "extensions/features/features.h"
......@@ -352,12 +356,29 @@ class ScopedTargetContentsOwner {
content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
const GURL& url) {
// Always create the new WebContents in a new SiteInstance (and therefore a
// new BrowsingInstance), *unless* there's a |params.opener|.
//
// Note that the SiteInstance below is only for the "initial" placement of the
// new WebContents (i.e. if subsequent navigation [including the initial
// navigation] triggers a cross-process transfer, then the opener and new
// contents can end up in separate processes). This is fine, because even if
// subsequent navigation is cross-process (i.e. cross-SiteInstance), then it
// will stay in the same BrowsingInstance (creating frame proxies as needed)
// preserving the requested opener relationship along the way.
scoped_refptr<content::SiteInstance> initial_site_instance_for_new_contents =
params.opener
? params.opener->GetSiteInstance()
: tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url);
WebContents::CreateParams create_params(
params.browser->profile(),
params.source_site_instance && !params.force_new_process_for_new_contents
? params.source_site_instance
: tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url));
params.browser->profile(), initial_site_instance_for_new_contents);
create_params.main_frame_name = params.frame_name;
if (params.opener) {
create_params.opener_render_frame_id = params.opener->GetRoutingID();
create_params.opener_render_process_id =
params.opener->GetProcess()->GetID();
}
if (params.source_contents) {
create_params.initial_size =
params.source_contents->GetContainerBounds().size();
......@@ -471,6 +492,7 @@ void Navigate(NavigateParams* params) {
if (GetSourceProfile(params) != params->browser->profile()) {
// A tab is being opened from a link from a different profile, we must reset
// source information that may cause state to be shared.
params->opener = nullptr;
params->source_contents = nullptr;
params->source_site_instance = nullptr;
params->referrer = content::Referrer();
......
......@@ -26,7 +26,7 @@ NavigateParams::NavigateParams(WebContents* a_target_contents)
target_contents(a_target_contents),
source_contents(nullptr),
disposition(WindowOpenDisposition::CURRENT_TAB),
force_new_process_for_new_contents(false),
opener(nullptr),
trusted_source(false),
transition(ui::PAGE_TRANSITION_LINK),
is_renderer_initiated(false),
......@@ -50,7 +50,7 @@ NavigateParams::NavigateParams(Browser* a_browser,
target_contents(NULL),
source_contents(NULL),
disposition(WindowOpenDisposition::CURRENT_TAB),
force_new_process_for_new_contents(false),
opener(nullptr),
trusted_source(false),
transition(a_transition),
is_renderer_initiated(false),
......@@ -73,7 +73,7 @@ NavigateParams::NavigateParams(Browser* a_browser,
target_contents(a_target_contents),
source_contents(NULL),
disposition(WindowOpenDisposition::CURRENT_TAB),
force_new_process_for_new_contents(false),
opener(nullptr),
trusted_source(false),
transition(ui::PAGE_TRANSITION_LINK),
is_renderer_initiated(false),
......@@ -99,7 +99,7 @@ NavigateParams::NavigateParams(Profile* a_profile,
target_contents(NULL),
source_contents(NULL),
disposition(WindowOpenDisposition::NEW_FOREGROUND_TAB),
force_new_process_for_new_contents(false),
opener(nullptr),
trusted_source(false),
transition(a_transition),
is_renderer_initiated(false),
......@@ -130,8 +130,6 @@ void FillNavigateParamsFromOpenURLParams(NavigateParams* nav_params,
nav_params->redirect_chain = params.redirect_chain;
nav_params->extra_headers = params.extra_headers;
nav_params->disposition = params.disposition;
nav_params->force_new_process_for_new_contents =
params.force_new_process_for_new_contents;
nav_params->trusted_source = false;
nav_params->is_renderer_initiated = params.is_renderer_initiated;
nav_params->should_replace_current_entry =
......
......@@ -23,6 +23,7 @@ class Browser;
class Profile;
namespace content {
class RenderFrameHost;
class WebContents;
struct OpenURLParams;
}
......@@ -133,11 +134,9 @@ struct NavigateParams {
// |tabstrip_add_types|.
WindowOpenDisposition disposition;
// Controls creation of new web contents (in case |disposition| asks for a new
// tab or window). If |force_new_process_for_new_contents| is true, then we
// try to put the new contents in a new renderer, even if they are same-site
// as |source_site_instance| (this is subject to renderer process limits).
bool force_new_process_for_new_contents;
// Allows setting the opener for the case when new WebContents are created
// (i.e. when |disposition| asks for a new tab or window).
content::RenderFrameHost* opener;
// Sets browser->is_trusted_source. Default is false.
bool trusted_source;
......
......@@ -227,6 +227,11 @@
"$ref": "WindowState",
"optional": true,
"description": "The initial state of the window. The 'minimized', 'maximized' and 'fullscreen' states cannot be combined with 'left', 'top', 'width' or 'height'."
},
"setSelfAsOpener": {
"type": "boolean",
"optional": true,
"description": "If 'setSelfAsOpener' is true, then the newly created window will have its 'window.opener' set to the caller and will be in the same <a href=\"https://www.w3.org/TR/html51/browsers.html#unit-of-related-browsing-contexts\">unit of related browsing contexts</a> as the caller."
}
},
"optional": true
......
......@@ -54,12 +54,12 @@ class CONTENT_EXPORT Navigator : public base::RefCounted<Navigator> {
RenderFrameHostImpl* render_frame_host,
const GURL& url,
const std::vector<GURL>& redirect_chain,
const base::TimeTicks& navigation_start) {};
const base::TimeTicks& navigation_start) {}
// The RenderFrameHostImpl has failed a provisional load.
virtual void DidFailProvisionalLoadWithError(
RenderFrameHostImpl* render_frame_host,
const FrameHostMsg_DidFailProvisionalLoadWithError_Params& params) {};
const FrameHostMsg_DidFailProvisionalLoadWithError_Params& params) {}
// The RenderFrameHostImpl has failed to load the document.
virtual void DidFailLoadWithError(RenderFrameHostImpl* render_frame_host,
......@@ -120,7 +120,6 @@ class CONTENT_EXPORT Navigator : public base::RefCounted<Navigator> {
const std::string& extra_headers,
const Referrer& referrer,
WindowOpenDisposition disposition,
bool force_new_process_for_new_contents,
bool should_replace_current_entry,
bool user_gesture,
blink::WebTriggeringEventInfo triggering_event_info) {}
......@@ -176,8 +175,8 @@ class CONTENT_EXPORT Navigator : public base::RefCounted<Navigator> {
// TODO(carlosk): once PlzNavigate is the only navigation implementation
// remove the URL parameter and rename this method to better suit its naming
// conventions.
virtual void LogResourceRequestTime(
base::TimeTicks timestamp, const GURL& url) {};
virtual void LogResourceRequestTime(base::TimeTicks timestamp,
const GURL& url) {}
// Called to record the time it took to execute the before unload hook for the
// current navigation.
......
......@@ -690,7 +690,6 @@ void NavigatorImpl::RequestOpenURL(
const std::string& extra_headers,
const Referrer& referrer,
WindowOpenDisposition disposition,
bool force_new_process_for_new_contents,
bool should_replace_current_entry,
bool user_gesture,
blink::WebTriggeringEventInfo triggering_event_info) {
......@@ -734,8 +733,6 @@ void NavigatorImpl::RequestOpenURL(
OpenURLParams params(dest_url, referrer, frame_tree_node_id, disposition,
ui::PAGE_TRANSITION_LINK,
true /* is_renderer_initiated */);
params.force_new_process_for_new_contents =
force_new_process_for_new_contents;
params.uses_post = uses_post;
params.post_data = body;
params.extra_headers = extra_headers;
......
......@@ -71,7 +71,6 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
const std::string& extra_headers,
const Referrer& referrer,
WindowOpenDisposition disposition,
bool force_new_process_for_new_contents,
bool should_replace_current_entry,
bool user_gesture,
blink::WebTriggeringEventInfo triggering_event_info) override;
......
......@@ -1403,22 +1403,11 @@ void RenderFrameHostImpl::OnOpenURL(const FrameHostMsg_OpenURL_Params& params) {
TRACE_EVENT1("navigation", "RenderFrameHostImpl::OpenURL", "url",
validated_url.possibly_invalid_spec());
// When |params.disposition| asks OpenURL to create new contents, it always
// happens because of an explicit user request for new content creation (i.e.
// opening a link using ctrl-click or middle click). In such cases, the new
// content should be created in a new renderer to break opener relationship
// (see https://crbug.com/658386) and to conserve memory per renderer (see
// https://crbug.com/23815). Note that new content creation that wasn't
// explicitly requested by the user (i.e. left-clicking a link with
// target=_blank) goes through a different code path (through
// WebViewClient::CreateView).
bool kForceNewProcessForNewContents = true;
frame_tree_node_->navigator()->RequestOpenURL(
this, validated_url, params.uses_post, params.resource_request_body,
params.extra_headers, params.referrer, params.disposition,
kForceNewProcessForNewContents, params.should_replace_current_entry,
params.user_gesture, params.triggering_event_info);
params.should_replace_current_entry, params.user_gesture,
params.triggering_event_info);
}
void RenderFrameHostImpl::OnCancelInitialHistoryLoad() {
......
......@@ -99,7 +99,7 @@ RenderFrameHostImpl* PrepareToDuplicateHosts(Shell* shell,
wc->GetFrameTree()->root()->navigator()->RequestOpenURL(
wc->GetFrameTree()->root()->current_frame_host(), extension_url, false,
nullptr, std::string(), Referrer(), WindowOpenDisposition::CURRENT_TAB,
false, false, true, blink::WebTriggeringEventInfo::kFromTrustedEvent);
false, true, blink::WebTriggeringEventInfo::kFromTrustedEvent);
// Since the navigation above requires a cross-process swap, there will be a
// speculative/pending RenderFrameHost. Ensure it exists and is in a different
......
......@@ -16,7 +16,6 @@ OpenURLParams::OpenURLParams(const GURL& url,
uses_post(false),
frame_tree_node_id(-1),
disposition(disposition),
force_new_process_for_new_contents(false),
transition(transition),
is_renderer_initiated(is_renderer_initiated),
should_replace_current_entry(false),
......@@ -34,7 +33,6 @@ OpenURLParams::OpenURLParams(const GURL& url,
uses_post(false),
frame_tree_node_id(-1),
disposition(disposition),
force_new_process_for_new_contents(false),
transition(transition),
is_renderer_initiated(is_renderer_initiated),
should_replace_current_entry(false),
......@@ -52,7 +50,6 @@ OpenURLParams::OpenURLParams(const GURL& url,
uses_post(false),
frame_tree_node_id(frame_tree_node_id),
disposition(disposition),
force_new_process_for_new_contents(false),
transition(transition),
is_renderer_initiated(is_renderer_initiated),
should_replace_current_entry(false),
......@@ -63,7 +60,6 @@ OpenURLParams::OpenURLParams()
: uses_post(false),
frame_tree_node_id(-1),
disposition(WindowOpenDisposition::UNKNOWN),
force_new_process_for_new_contents(false),
transition(ui::PAGE_TRANSITION_LINK),
is_renderer_initiated(false),
should_replace_current_entry(false),
......
......@@ -10,6 +10,7 @@
#define CONTENT_PUBLIC_BROWSER_PAGE_NAVIGATOR_H_
#include <string>
#include <vector>
#include "base/memory/ref_counted.h"
#include "content/common/content_export.h"
......@@ -83,12 +84,6 @@ struct CONTENT_EXPORT OpenURLParams {
// The disposition requested by the navigation source.
WindowOpenDisposition disposition;
// Controls creation of new web contents (in case |disposition| asks for a new
// tab or window). If |force_new_process_for_new_contents| is true, then we
// try to put the new contents in a new renderer, even if they are same-site
// as |source_site_instance| (this is subject to renderer process limits).
bool force_new_process_for_new_contents;
// The transition type of navigation.
ui::PageTransition transition;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment