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..2614fa71673c9316a5fb29a6b2b2ce48a8b14dc7 100644
--- a/chrome/browser/ui/browser_navigator.cc
+++ b/chrome/browser/ui/browser_navigator.cc
@@ -36,7 +36,11 @@
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_url_handler.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/session_storage_namespace.h"
+#include "content/public/browser/site_instance.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
 
@@ -249,6 +253,13 @@ void LoadURLInContents(WebContents* target_contents,
   load_url_params.extra_headers = params->extra_headers;
   load_url_params.should_replace_current_entry =
       params->should_replace_current_entry;
+  // Even if we only copy the session storage namespace and do not set the
+  // opener, we need to use the SiteInstance that was created in
+  // CreateTargetContents to make sure we created the SessionStorageNamespace
+  // in the right StoragePartition.
+  load_url_params.should_stay_in_site_instance =

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

This is incorrect.  Setting the opener will not always imply that you should
stay in the same SiteInstance.  More comments below.

[jochen (gone - plz use gerrit), 2013/07/31 18:30:52]

Removed.

+      params->should_copy_session_storage_namespace ||
+      params->should_set_opener;
 
   if (params->transferred_global_request_id != GlobalRequestID()) {
     load_url_params.is_renderer_initiated = params->is_renderer_initiated;
@@ -317,6 +328,10 @@ content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
   WebContents::CreateParams create_params(
       params.browser->profile(),
       tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url));
+  // In order for window.opener to be accessible, the target contents needs to

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

This reasoning is false.  The reason that we create opener RVHs is so that other
processes will have valid window.openers.

Perhaps there's additional reasoning that makes this ok for the time being?

Today, we treat "w=window.open();w.opener=null;w.location.href=url" as a
cross-process navigation that opens in a different SiteInstance but the same
BrowsingInstance.  In that case, the opener will be suppressed and perhaps we
won't hit this logic?

For rel=noreferrer target=_blank links, we also suppress the opener and open the
link in new SiteInstance and new BrowsingInstance.  The opener won't be set
there either.

Have you tested those cases to ensure they still work?  I see a test for
rel=noreferrer, but it's not using target=_blank, so I don't think
suppress_opener will be true.

A bigger problem is that our site isolation work is making it possible for the
new window to open in a new SiteInstance even with just a normal cross-site
window.open call.  I don't want to paint us into a corner here, so I don't want
to add logic that depends on something else.  

If we do have to change something here, we should keep the process decisions in
fewer places.  The tab_util::GetSiteInstanceForNewTab function above seems like
a better place to handle it.

[jochen (gone - plz use gerrit), 2013/07/31 18:30:52]

I removed the SiteInstance related logic as discussed via VC.

To summarize, by not forcing a specific site instance, we might have corner
cases where we could have a scriptable window.opener but won't have one. I guess
that's fine.

Also, not forcing the SiteInstance works fine with the current StoragePartition
model but we might need to revise that when we use more than one storage
partition. Added a comment about that.

+  // run in the same process as the source contents.
+  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();
@@ -329,7 +344,37 @@ content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
   }
 #endif
 
-  content::WebContents* target_contents = WebContents::Create(create_params);
+  content::SessionStorageNamespaceMap namespace_map;
+  scoped_refptr<content::SiteInstance> site_instance;
+  if (params.source_contents && params.should_copy_session_storage_namespace) {
+    Profile* source_profile = Profile::FromBrowserContext(
+        params.source_contents->GetBrowserContext());
+    bool is_guest =
+        params.source_contents->GetRenderViewHost()->GetProcess()->IsGuest();
+    site_instance =
+        !params.should_set_opener && !is_guest
+            ? content::SiteInstance::CreateForURL(source_profile, url)
+            : params.source_contents->GetSiteInstance();
+    create_params.site_instance = site_instance;
+
+    std::string partition_id = source_profile->GetStoragePartitionIdForSite(
+        site_instance->GetSiteURL());
+    content::StoragePartition* partition =
+        content::BrowserContext::GetStoragePartition(source_profile,
+                                                     site_instance);
+    content::SessionStorageNamespaceMap::const_iterator old_namespace =
+        params.source_contents->GetController().GetSessionStorageNamespaceMap()
+            .find(partition_id);
+    scoped_refptr<content::SessionStorageNamespace> cloned_namespace =
+        content::SessionStorageNamespace::Clone(
+            partition->GetDOMStorageContext(), old_namespace->second.get());
+    namespace_map.insert(make_pair(partition_id, cloned_namespace));
+  }
+  WebContents* target_contents = WebContents::CreateWithSessionStorage(
+      create_params,
+      namespace_map,
+      params.should_set_opener ? params.source_contents : NULL);
+
   // 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 +438,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 +459,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 +482,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() {}