Try to restore window.opener when opening blocked popups

chrome/browser/ui/browser_navigator.cc

Index: chrome/browser/ui/browser_navigator.cc
diff --git a/chrome/browser/ui/browser_navigator.cc b/chrome/browser/ui/browser_navigator.cc
index 72505692db50fb265dc6bf352d0ffa1524e72157..00d9224145bcf5a9d20c2848018cca89b0e9a9b7 100644
--- a/chrome/browser/ui/browser_navigator.cc
+++ b/chrome/browser/ui/browser_navigator.cc
@@ -37,6 +37,7 @@
 #include "content/public/browser/browser_url_handler.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
 
@@ -317,10 +318,21 @@ content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
   WebContents::CreateParams create_params(
       params.browser->profile(),
       tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url));
+
+  // If we want to set the opener, we need to use the same SiteInstance.

[Charlie Reis, 2013/07/30 01:26:30]

I'm skeptical about this, since it shouldn't always be the same SiteInstance. 
We have careful logic in WebContentsImpl::CreateNewWindow about which
SiteInstance to use, and this approach loses that.  We probably need to find a
better way to handle this.

[jochen (gone - plz use gerrit), 2013/07/30 14:50:39]

The popup needs to be in the same process as the opener, otherwise it can't
access window.opener.

In fact, setting the site_instance here is a no-op:
NavigationController::LoadURL will create a NavigationEntry with a NULL site
instance anyway, and so
RenderViewHostController::PrepareRenderViewHostForNavigation creates a new one
if the URL differs from the site url.

[Charlie Reis, 2013/07/31 17:16:16]

That's not true.  Creating opener RVHs allows it to access a placeholder
window.opener when it's cross-site.
What is RenderViewHostController::PrepareRenderViewHostForNavigation?  Do you
mean RenderViewHostManager::UpdateRendererStateForNavigate?  If we're going
through that, we'll have the opposite problem-- creating a new SiteInstance more
frequently than we should.  Most cross-site window.open calls should stay in the
same SiteInstance-- only special cases like rel=noreferrer or the new
site-per-process logic should swap.

+  if (params.source_contents && params.should_set_opener)
+    create_params.site_instance = params.source_contents->GetSiteInstance();
   if (params.source_contents) {
     create_params.initial_size =
         params.source_contents->GetView()->GetContainerSize();
   }
+  if (params.source_contents && params.should_copy_session_storage_namespace) {
+    // To clone the session storage namespace, we need a site instance.
+    if (!create_params.site_instance) {
+      create_params.site_instance =
+          content::SiteInstance::CreateForURL(params.browser->profile(), url);
+    }
+  }
 #if defined(USE_AURA)
   if (params.browser->window() &&
       params.browser->window()->GetNativeWindow()) {
@@ -328,8 +340,18 @@ content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
         params.browser->window()->GetNativeWindow();
   }
 #endif
+  WebContents* target_contents;
+  if (params.source_contents &&
+      (params.should_set_opener ||
+       params.should_copy_session_storage_namespace)) {
+    DCHECK(!params.should_set_opener ||

[Bernhard Bauer, 2013/07/30 08:34:50]

Wait, you have here

  if (a || b) {
    DCHECK(!a || b);
  }

Is that really the behavior you want?

+           params.should_copy_session_storage_namespace);
+    target_contents = WebContents::CreateWithOpener(
+        create_params, params.source_contents, params.should_set_opener);
+  } else {
+    target_contents = WebContents::Create(create_params);
+  }
 
-  content::WebContents* target_contents = WebContents::Create(create_params);
   // New tabs can have WebUI URLs that will make calls back to arbitrary
   // tab helpers, so the entire set of tab helpers needs to be set up
   // immediately.
@@ -393,7 +415,9 @@ NavigateParams::NavigateParams(Browser* a_browser,
       browser(a_browser),
       initiating_profile(NULL),
       host_desktop_type(GetHostDesktop(a_browser)),
-      should_replace_current_entry(false) {
+      should_replace_current_entry(false),
+      should_copy_session_storage_namespace(false),
+      should_set_opener(false) {
 }
 
 NavigateParams::NavigateParams(Browser* a_browser,
@@ -412,7 +436,9 @@ NavigateParams::NavigateParams(Browser* a_browser,
       browser(a_browser),
       initiating_profile(NULL),
       host_desktop_type(GetHostDesktop(a_browser)),
-      should_replace_current_entry(false) {
+      should_replace_current_entry(false),
+      should_copy_session_storage_namespace(false),
+      should_set_opener(false) {
 }
 
 NavigateParams::NavigateParams(Profile* a_profile,
@@ -433,7 +459,9 @@ NavigateParams::NavigateParams(Profile* a_profile,
       browser(NULL),
       initiating_profile(a_profile),
       host_desktop_type(chrome::GetActiveDesktop()),
-      should_replace_current_entry(false) {
+      should_replace_current_entry(false),
+      should_copy_session_storage_namespace(false),
+      should_set_opener(false) {
 }
 
 NavigateParams::~NavigateParams() {}