diff options
9 files changed, 106 insertions, 13 deletions
diff --git a/chrome/browser/extensions/app_process_apitest.cc b/chrome/browser/extensions/app_process_apitest.cc index 7b56929..9602831 100644 --- a/chrome/browser/extensions/app_process_apitest.cc +++ b/chrome/browser/extensions/app_process_apitest.cc @@ -9,6 +9,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_list.h" +#include "chrome/browser/ui/browser_window.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/extensions/extension.h" #include "chrome/test/base/ui_test_utils.h" @@ -340,6 +341,61 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, OpenAppFromIframe) { process()->is_extension_process()); } +// Tests that if we have an app process (path1/container.html) with a non-app +// iframe (path3/iframe.html), then opening a link from that iframe to a new +// window to a same-origin non-app URL (path3/empty.html) should keep the window +// in the app process. +// This is in contrast to OpenAppFromIframe, since here the popup will not be +// missing special permissions and should be scriptable from the iframe. +// See http://crbug.com/92669 for more details. +IN_PROC_BROWSER_TEST_F(AppApiTest, OpenWebPopupFromWebIframe) { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kDisablePopupBlocking); + + host_resolver()->AddRule("*", "127.0.0.1"); + ASSERT_TRUE(test_server()->Start()); + + GURL base_url = GetTestBaseURL("app_process"); + + // Load app and start URL (in the app). + const Extension* app = + LoadExtension(test_data_dir_.AppendASCII("app_process")); + ASSERT_TRUE(app); + ui_test_utils::NavigateToURLWithDisposition( + browser(), + base_url.Resolve("path1/container.html"), + CURRENT_TAB, + ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION | + ui_test_utils::BROWSER_TEST_WAIT_FOR_BROWSER); + RenderProcessHost* process = + browser()->GetTabContentsAt(0)->render_view_host()->process(); + EXPECT_TRUE(process->is_extension_process()); + + // Wait for popup window to appear. The new Browser may not have been + // added with SetLastActive, in which case we need to show it first. + // This is necessary for popup windows without a cross-site transition. + if (browser() == BrowserList::GetLastActive()) { + // Grab the second window and show it. + ASSERT_TRUE(BrowserList::size() == 2); + Browser* popup_browser = *(++BrowserList::begin()); + popup_browser->window()->Show(); + } + Browser* last_active_browser = BrowserList::GetLastActive(); + EXPECT_TRUE(last_active_browser); + ASSERT_NE(browser(), last_active_browser); + TabContents* newtab = last_active_browser->GetSelectedTabContents(); + EXPECT_TRUE(newtab); + GURL non_app_url = base_url.Resolve("path3/empty.html"); + if (!newtab->controller().GetLastCommittedEntry() || + newtab->controller().GetLastCommittedEntry()->url() != non_app_url) + ui_test_utils::WaitForNavigation(&newtab->controller()); + + // Popup window should be in the app's process. + RenderProcessHost* popup_process = + last_active_browser->GetTabContentsAt(0)->render_view_host()->process(); + EXPECT_EQ(process, popup_process); +} + IN_PROC_BROWSER_TEST_F(AppApiTest, ReloadAppAfterCrash) { host_resolver()->AddRule("*", "127.0.0.1"); ASSERT_TRUE(test_server()->Start()); diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc index 614835e..25ae18d 100644 --- a/chrome/renderer/chrome_content_renderer_client.cc +++ b/chrome/renderer/chrome_content_renderer_client.cc @@ -72,6 +72,7 @@ #include "third_party/WebKit/Source/WebKit/chromium/public/WebDocument.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebPluginParams.h" +#include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityPolicy.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebURL.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLError.h" @@ -89,8 +90,10 @@ using WebKit::WebDataSource; using WebKit::WebFrame; using WebKit::WebPlugin; using WebKit::WebPluginParams; +using WebKit::WebSecurityOrigin; using WebKit::WebSecurityPolicy; using WebKit::WebString; +using WebKit::WebURL; using WebKit::WebURLError; using WebKit::WebURLRequest; using WebKit::WebURLResponse; @@ -574,13 +577,14 @@ bool ChromeContentRendererClient::AllowPopup(const GURL& creator) { bool ChromeContentRendererClient::ShouldFork(WebFrame* frame, const GURL& url, bool is_content_initiated, + bool is_initial_navigation, bool* send_referrer) { // If the navigation would cross an app extent boundary, we also need // to defer to the browser to ensure process isolation. // TODO(erikkay) This is happening inside of a check to is_content_initiated // which means that things like the back button won't trigger it. Is that // OK? - if (!CrossesExtensionExtents(frame, url)) + if (!CrossesExtensionExtents(frame, url, is_initial_navigation)) return false; // Include the referrer in this case since we're going from a hosted web @@ -682,25 +686,40 @@ void ChromeContentRendererClient::SetExtensionDispatcher( extension_dispatcher_.reset(extension_dispatcher); } -bool ChromeContentRendererClient::CrossesExtensionExtents(WebFrame* frame, - const GURL& new_url) { +bool ChromeContentRendererClient::CrossesExtensionExtents( + WebFrame* frame, + const GURL& new_url, + bool is_initial_navigation) { const ExtensionSet* extensions = extension_dispatcher_->extensions(); - // If the URL is still empty, this is a window.open navigation. Check the - // opener's URL. In all cases we use the top frame's URL (as opposed to our - // frame's) since that's what determines the type of process. - // TODO(abarth): This code is super sketchy! Are you sure looking at the - // opener is correct here? This appears to let me steal my opener's - // privileges if I can make my URL be "empty." + bool is_extension_url = !!extensions->GetByURL(new_url); GURL old_url(frame->top()->document().url()); - if (old_url.is_empty() && frame->opener()) + + // If old_url is still empty and this is an initial navigation, then this is + // a window.open operation. We should look at the opener URL. + if (is_initial_navigation && old_url.is_empty() && frame->opener()) { + // If we're about to open a normal web page from a same-origin opener stuck + // in an extension process, we want to keep it in process to allow the + // opener to script it. + GURL opener_url = frame->opener()->document().url(); + bool opener_is_extension_url = !!extensions->GetByURL(opener_url); + WebSecurityOrigin opener = frame->opener()->document().securityOrigin(); + if (!is_extension_url && + !opener_is_extension_url && + extension_dispatcher_->is_extension_process() && + opener.canRequest(WebURL(new_url))) + return false; + + // In all other cases, we want to compare against the top frame's URL (as + // opposed to the opener frame's), since that's what determines the type of + // process. This allows iframes outside an app to open a popup in the app. old_url = frame->top()->opener()->top()->document().url(); + } // If this is a reload, check whether it has the wrong process type. We // should send it to the browser if it's an extension URL (e.g., hosted app) // in a normal process, or if it's a process for an extension that has been // uninstalled. if (old_url == new_url) { - bool is_extension_url = !!extensions->GetByURL(new_url); if (is_extension_url != extension_dispatcher_->is_extension_process()) return true; } diff --git a/chrome/renderer/chrome_content_renderer_client.h b/chrome/renderer/chrome_content_renderer_client.h index 2b9a309..2ce9639 100644 --- a/chrome/renderer/chrome_content_renderer_client.h +++ b/chrome/renderer/chrome_content_renderer_client.h @@ -55,6 +55,7 @@ class ChromeContentRendererClient : public content::ContentRendererClient { virtual bool ShouldFork(WebKit::WebFrame* frame, const GURL& url, bool is_content_initiated, + bool is_initial_navigation, bool* send_referrer) OVERRIDE; virtual bool WillSendRequest(WebKit::WebFrame* frame, const GURL& url, @@ -99,7 +100,9 @@ class ChromeContentRendererClient : public content::ContentRendererClient { // Returns true if the frame is navigating to an URL either into or out of an // extension app's extent. - bool CrossesExtensionExtents(WebKit::WebFrame* frame, const GURL& new_url); + bool CrossesExtensionExtents(WebKit::WebFrame* frame, + const GURL& new_url, + bool is_initial_navigation); scoped_ptr<ChromeRenderProcessObserver> chrome_observer_; scoped_ptr<ExtensionDispatcher> extension_dispatcher_; diff --git a/chrome/test/data/extensions/api_test/app_process/path1/container.html b/chrome/test/data/extensions/api_test/app_process/path1/container.html new file mode 100644 index 0000000..05c32c1 --- /dev/null +++ b/chrome/test/data/extensions/api_test/app_process/path1/container.html @@ -0,0 +1,2 @@ +<!-- Iframe to a normal web page, outside app manifest. --> +<iframe src="/files/extensions/api_test/app_process/path3/iframe.html"></iframe> diff --git a/chrome/test/data/extensions/api_test/app_process/path3/iframe.html b/chrome/test/data/extensions/api_test/app_process/path3/iframe.html new file mode 100644 index 0000000..e152dab --- /dev/null +++ b/chrome/test/data/extensions/api_test/app_process/path3/iframe.html @@ -0,0 +1,8 @@ +<script> + // Open popup to another normal web page, same-origin and non-app. + window.open( + '/files/extensions/api_test/app_process/path3/empty.html', + '', + // Small enough that a popup will be created even on Chrome OS + 'width=200,height=200'); +</script> diff --git a/content/renderer/content_renderer_client.h b/content/renderer/content_renderer_client.h index 2bf6477..2ebbf98 100644 --- a/content/renderer/content_renderer_client.h +++ b/content/renderer/content_renderer_client.h @@ -80,6 +80,7 @@ class ContentRendererClient { virtual bool ShouldFork(WebKit::WebFrame* frame, const GURL& url, bool is_content_initiated, + bool is_initial_navigation, bool* send_referrer) = 0; // Notifies the embedder that the given frame is requesting the resource at diff --git a/content/renderer/mock_content_renderer_client.cc b/content/renderer/mock_content_renderer_client.cc index d3a378d..dc931c8 100644 --- a/content/renderer/mock_content_renderer_client.cc +++ b/content/renderer/mock_content_renderer_client.cc @@ -58,6 +58,7 @@ bool MockContentRendererClient::AllowPopup(const GURL& creator) { bool MockContentRendererClient::ShouldFork(WebKit::WebFrame* frame, const GURL& url, bool is_content_initiated, + bool is_initial_navigation, bool* send_referrer) { return false; } diff --git a/content/renderer/mock_content_renderer_client.h b/content/renderer/mock_content_renderer_client.h index 0bda9dc..5c0668f 100644 --- a/content/renderer/mock_content_renderer_client.h +++ b/content/renderer/mock_content_renderer_client.h @@ -35,6 +35,7 @@ class MockContentRendererClient : public ContentRendererClient { virtual bool ShouldFork(WebKit::WebFrame* frame, const GURL& url, bool is_content_initiated, + bool is_initial_navigation, bool* send_referrer) OVERRIDE; virtual bool WillSendRequest(WebKit::WebFrame* frame, const GURL& url, diff --git a/content/renderer/render_view.cc b/content/renderer/render_view.cc index 033247d..d112be8 100644 --- a/content/renderer/render_view.cc +++ b/content/renderer/render_view.cc @@ -2150,8 +2150,10 @@ WebNavigationPolicy RenderView::decidePolicyForNavigation( if (!should_fork) { // Give the embedder a chance. + bool is_initial_navigation = page_id_ == -1; should_fork = content::GetContentClient()->renderer()->ShouldFork( - frame, url, is_content_initiated, &send_referrer); + frame, url, is_content_initiated, is_initial_navigation, + &send_referrer); } if (should_fork) { |