Try to restore window.opener when opening blocked popups

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 269 matching lines @@
        it != session_storage_namespace_map.end();
        ++it) {
     new_contents->GetController()
         .SetSessionStorageNamespace(it->first, it->second.get());
   }
 
   new_contents->Init(params);
   return new_contents;
 }
 
+// static
+WebContents* WebContents::CreateWithOpener(
+    const WebContents::CreateParams& params,
+    WebContents* opener,
+    bool propagate_opener) {

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

Why switch from suppress_opener, which is used commonly elsewhere?

+  WebContentsImpl* new_contents = new WebContentsImpl(
+      params.browser_context,
+      propagate_opener ? static_cast<WebContentsImpl*>(opener) : NULL);
+
+  if (opener) {
+    // To clone the session storage namespace, we need a site instance.
+    CHECK(params.site_instance);

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

I assume you have good reasons for using a CHECK over a DCHECK here? If it's
just "this shouldn't happen", I'd prefer a DCHECK for that.

+
+    // It's not possible to use a guest as opener.

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

That's not true.  Fady changed WebContentsImpl::CreateNewWindow to support
window.open from guests.

+    CHECK(!opener->GetRenderProcessHost()->IsGuest());
+
+    // Clone the session storage namespace of the opener.

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

This code only copies the session storage namespace for the current
SiteInstance, which will be a problem if the popup navigates to a different
SiteInstance that had previously been open in the opener window.

The logic in CreateWithSessionStorage already handles this by copying over the
whole map.  Is there some way that we can make these various functions share
code?  It's getting a little unwieldy with Create, CreateWithSessionStorage, and
two CreateWithOpeners, though I'm not sure if they're easy to combine.  (They
all take in CreateParams, so it seems like it might be possible.)

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

No. it clones it (see line 320).
John suggested to move the cloning logic to browser navigator and use
CreateWithSessionStorage.

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

You're only cloning a single SessionStorageNamespace below.  Each WebContents
has an entire map, and the whole map needs to be cloned, as in
CreateWithSessionStorage.

+    const std::string& partition_id =
+      GetContentClient()->browser()->

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

Nit: This needs to be indented four spaces more.

+          GetStoragePartitionIdForSite(opener->GetBrowserContext(),
+                                       params.site_instance->GetSiteURL());
+    StoragePartition* partition = BrowserContext::GetStoragePartition(
+        opener->GetBrowserContext(), params.site_instance);
+    DOMStorageContextImpl* dom_storage_context =
+        static_cast<DOMStorageContextImpl*>(partition->GetDOMStorageContext());
+    SessionStorageNamespaceMap::const_iterator old_namespace = opener
+        ->GetController().GetSessionStorageNamespaceMap().find(partition_id);

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

nit: Please be consistent about -> being at the end or beginning of a line (vs
line 308).  Personally, I prefer -> at the end (like line 308) and . at the
beginning (like line 323).

+    CHECK(old_namespace !=
+          opener->GetController().GetSessionStorageNamespaceMap().end());

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

If the partition ID isn't found, you'll crash later anyway.

+    scoped_refptr<SessionStorageNamespaceImpl> cloned_namespace =
+        new SessionStorageNamespaceImpl(dom_storage_context,
+                                        old_namespace->second->id());
+    new_contents->GetController()
+        .SetSessionStorageNamespace(partition_id, cloned_namespace.get());
+  }
+
+  new_contents->Init(params);
+  return new_contents;
+}
+
 void WebContents::AddCreatedCallback(const CreatedCallback& callback) {
   g_created_callbacks.Get().push_back(callback);
 }
 
 void WebContents::RemoveCreatedCallback(const CreatedCallback& callback) {
   for (size_t i = 0; i < g_created_callbacks.Get().size(); ++i) {
     if (g_created_callbacks.Get().at(i).Equals(callback)) {
       g_created_callbacks.Get().erase(g_created_callbacks.Get().begin() + i);
       return;
     }
@@ skipping 102 matching lines @@
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,
                     WebContentsImplDestroyed());
 
   SetDelegate(NULL);
 
   STLDeleteContainerPairSecondPointers(destruction_observers_.begin(),
                                        destruction_observers_.end());
 }
 
 WebContentsImpl* WebContentsImpl::CreateWithOpener(

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

I'm finding it confusing that we have two different CreateWithOpener functions.

     const WebContents::CreateParams& params,
     WebContentsImpl* opener) {
   TRACE_EVENT0("browser", "WebContentsImpl::CreateWithOpener");
   WebContentsImpl* new_contents = new WebContentsImpl(
       params.browser_context, opener);
 
   new_contents->Init(params);
   return new_contents;
 }
 
@@ skipping 3305 matching lines @@
 }
 
 void WebContentsImpl::ClearAllPowerSaveBlockers() {
   for (PowerSaveBlockerMap::iterator i(power_save_blockers_.begin());
        i != power_save_blockers_.end(); ++i)
     STLDeleteValues(&power_save_blockers_[i->first]);
   power_save_blockers_.clear();
 }
 
 }  // namespace content