Track frame openers in FrameTreeNodes instead of WebContents

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index a1ad421e10aa4e62d4ae2b5a0b16297eb67bce28..027c8e8ccbdbe812ed113db67ae2870148c134d4 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -204,15 +204,23 @@ void SetAccessibilityModeOnFrame(AccessibilityMode mode,
 }  // namespace
 
 WebContents* WebContents::Create(const WebContents::CreateParams& params) {
-  return WebContentsImpl::CreateWithOpener(
-      params, static_cast<WebContentsImpl*>(params.opener));
+  FrameTreeNode* opener_node = nullptr;
+  CHECK(!params.opener || params.opener_render_frame_id != MSG_ROUTING_NONE);
+  if (params.opener_render_frame_id != MSG_ROUTING_NONE) {
+    RenderFrameHostImpl* opener_rfh = RenderFrameHostImpl::FromID(
+        params.opener->GetRenderProcessHost()->GetID(),

[alexmos, 2015/06/01 17:44:55]

I didn't remove "opener" from WebContents::CreateParams because I need it here
for the RPH ID.  Alternatively, I could change opener ->
opener_render_process_id in CreateParams, or do the FTN resolution earlier (in
WebContentsImpl::CreateNewWindow) and make opener a  FrameTreeNode in the
CreateParams - any thoughts on this?

[Charlie Reis, 2015/06/03 20:01:37]

CreateParams is public, so we shouldn't put FrameTreeNode on it.  I think it
makes sense to replace WebContents* opener with opener_render_frame_id and
opener_render_process_id.

[alexmos, 2015/06/05 22:34:31]

Done.

+        params.opener_render_frame_id);
+    // TODO: Is this null check needed?  Seems like opener could navigate away.

[Charlie Reis, 2015/06/03 20:01:37]

Good question.  I *think* opener_rfh would be the one sending us the message
that causes us to arrive here, so it shouldn't be gone, but I'm not sure how
that call stack looks or if it's possible for us to get here from another path. 
The null check seems reasonable unless we can show it's safe.

[alexmos, 2015/06/05 22:34:31]

Acknowledged.

+    if (opener_rfh)
+      opener_node = opener_rfh->frame_tree_node();
+  }
+  return WebContentsImpl::CreateWithOpener(params, opener_node);
 }
 
 WebContents* WebContents::CreateWithSessionStorage(
     const WebContents::CreateParams& params,
     const SessionStorageNamespaceMap& session_storage_namespace_map) {
-  WebContentsImpl* new_contents = new WebContentsImpl(
-      params.browser_context, NULL);
+  WebContentsImpl* new_contents = new WebContentsImpl(params.browser_context);
 
   for (SessionStorageNamespaceMap::const_iterator it =
            session_storage_namespace_map.begin();
@@ -288,13 +296,11 @@ WebContentsImpl::ColorChooserInfo::~ColorChooserInfo() {
 
 // WebContentsImpl -------------------------------------------------------------
 
-WebContentsImpl::WebContentsImpl(BrowserContext* browser_context,
-                                 WebContentsImpl* opener)
+WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
     : delegate_(NULL),
       controller_(this, browser_context),
       render_view_host_delegate_view_(NULL),
-      opener_(opener),
-      created_with_opener_(!!opener),
+      created_with_opener_(false),
 #if defined(OS_WIN)
       accessible_parent_(NULL),
 #endif
@@ -422,10 +428,14 @@ WebContentsImpl::~WebContentsImpl() {
 
 WebContentsImpl* WebContentsImpl::CreateWithOpener(
     const WebContents::CreateParams& params,
-    WebContentsImpl* opener) {
+    FrameTreeNode* opener) {
   TRACE_EVENT0("browser", "WebContentsImpl::CreateWithOpener");
-  WebContentsImpl* new_contents = new WebContentsImpl(
-      params.browser_context, params.opener_suppressed ? NULL : opener);
+  WebContentsImpl* new_contents = new WebContentsImpl(params.browser_context);
+
+  if (!params.opener_suppressed && opener) {
+    new_contents->GetFrameTree()->root()->SetOpener(opener);
+    new_contents->created_with_opener_ = true;
+  }
 
   if (params.created_with_opener)
     new_contents->created_with_opener_ = true;

[Charlie Reis, 2015/06/03 20:01:37]

Why do we need both this and the case above?  If they're both needed, we should
probably explain in a comment.

[alexmos, 2015/06/05 22:34:32]

I added a comment.  It's needed in the case of allowing blocked popups (which
now have a null opener, but still need the created_with_opener_ bit set).

@@ -473,6 +483,13 @@ WebContentsImpl* WebContentsImpl::FromFrameTreeNode(
       WebContents::FromRenderFrameHost(frame_tree_node->current_frame_host()));
 }
 
+WebContentsImpl* WebContentsImpl::opener() const {
+  FrameTreeNode* opener_ftn = frame_tree_.root()->opener();
+  // TODO(alexmos): what if opener_ftn->current_frame_host() is null?  This
+  // will return nullptr, but can we ever have a valid WebContents w/o a RFH?

[alexmos, 2015/06/01 17:44:55]

I think that's impossible, but just wanted to double check.  I.e., is there any
case where getting to the opener WebContents via RFHs and FTN wouldn't work, but
the old way of just storing the link to opener contents would.

[Charlie Reis, 2015/06/03 20:01:37]

I *think* the RFH is only null before WebContentsImpl::Init is called.  Not sure
if anyone would call this before Init.

[alexmos, 2015/06/05 22:34:32]

Yes, CreateWithOpener only has a few lines between creating the WebContents and
calling Init, which don't call this, so I think this is safe.

+  return opener_ftn ? FromFrameTreeNode(opener_ftn) : nullptr;
+}
+
 RenderFrameHostManager* WebContentsImpl::GetRenderManagerForTesting() {
   return GetRenderManager();
 }
@@ -1160,11 +1177,12 @@ void WebContentsImpl::Stop() {
 
 WebContents* WebContentsImpl::Clone() {
   // We use our current SiteInstance since the cloned entry will use it anyway.
-  // We pass our own opener so that the cloned page can access it if it was
+  // We pass our own opener so that the cloned page can access it if it was set
   // before.
   CreateParams create_params(GetBrowserContext(), GetSiteInstance());
   create_params.initial_size = GetContainerBounds().size();
-  WebContentsImpl* tc = CreateWithOpener(create_params, opener_);
+  WebContentsImpl* tc =
+      CreateWithOpener(create_params, frame_tree_.root()->opener());

[alexmos, 2015/06/01 17:44:55]

Uncovered an interesting bug here which I haven't filed yet.  Create a new popup
from a subframe, then duplicate the popup tab.  The original popup's opener will
point to the subframe, but the duplicated popup's opener will point to the main
frame.  This CL isn't quite enough to fix this, as it needs to propagate the
subframe opener to the new renderer (one of my followup CLs).

[Charlie Reis, 2015/06/03 20:01:37]

Nice.  Worth filing it so that we make sure it gets fixed by the followup CLs.

[alexmos, 2015/06/05 22:34:32]

Done.  Filed issue 497382.

   tc->GetController().CopyStateFrom(controller_);
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,
@@ -1245,10 +1263,6 @@ void WebContentsImpl::Init(const WebContents::CreateParams& params) {
   gfx::Size initial_size = params.initial_size;
   view_->CreateView(initial_size, params.context);
 
-  // Listen for whether our opener gets destroyed.
-  if (opener_)
-    AddDestructionObserver(opener_);
-
 #if defined(ENABLE_PLUGINS)
   plugin_content_origin_whitelist_.reset(
       new PluginContentOriginWhitelist(this));
@@ -1294,11 +1308,6 @@ void WebContentsImpl::Init(const WebContents::CreateParams& params) {
 void WebContentsImpl::OnWebContentsDestroyed(WebContentsImpl* web_contents) {
   RemoveDestructionObserver(web_contents);
 
-  // Clear the opener if it has been closed.
-  if (web_contents == opener_) {
-    opener_ = NULL;
-    return;
-  }
   // Clear a pending contents that has been closed before being shown.
   for (PendingContents::iterator iter = pending_contents_.begin();
        iter != pending_contents_.end();
@@ -1601,6 +1610,7 @@ void WebContentsImpl::CreateNewWindow(
   create_params.main_frame_routing_id = main_frame_route_id;
   create_params.main_frame_name = base::UTF16ToUTF8(params.frame_name);
   create_params.opener = this;
+  create_params.opener_render_frame_id = params.opener_render_frame_id;
   create_params.opener_suppressed = params.opener_suppressed;
   if (params.disposition == NEW_BACKGROUND_TAB)
     create_params.initially_hidden = true;
@@ -2469,11 +2479,11 @@ bool WebContentsImpl::GotResponseToLockMouseRequest(bool allowed) {
 }
 
 bool WebContentsImpl::HasOpener() const {
-  return opener_ != NULL;
+  return opener() != NULL;
 }
 
 WebContents* WebContentsImpl::GetOpener() const {
-  return static_cast<WebContents*>(opener_);
+  return static_cast<WebContents*>(opener());
 }
 
 void WebContentsImpl::DidChooseColorInColorChooser(SkColor color) {
@@ -3808,22 +3818,6 @@ void WebContentsImpl::DidChangeName(RenderFrameHost* render_frame_host,
                     FrameNameChanged(render_frame_host, name));
 }
 
-void WebContentsImpl::DidDisownOpener(RenderFrameHost* render_frame_host) {
-  // No action is necessary if the opener has already been cleared.
-  if (!opener_)
-    return;
-
-  // Clear our opener so that future cross-process navigations don't have an
-  // opener assigned.
-  RemoveDestructionObserver(opener_);
-  opener_ = NULL;
-
-  // Notify all swapped out RenderViewHosts for this tab.  This is important
-  // in case we go back to them, or if another window in those processes tries
-  // to access window.opener.
-  GetRenderManager()->DidDisownOpener(render_frame_host);
-}
-
 void WebContentsImpl::DocumentOnLoadCompleted(
     RenderFrameHost* render_frame_host) {
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
@@ -4095,11 +4089,11 @@ void WebContentsImpl::NotifyMainFrameSwappedFromRenderManager(
 
 int WebContentsImpl::CreateOpenerRenderViewsForRenderManager(
     SiteInstance* instance) {
-  if (!opener_)
+  if (!opener())
     return MSG_ROUTING_NONE;
 
   // Recursively create RenderViews for anything else in the opener chain.
-  return opener_->CreateOpenerRenderViews(instance);
+  return opener()->CreateOpenerRenderViews(instance);
 }
 
 int WebContentsImpl::CreateOpenerRenderViews(SiteInstance* instance) {
@@ -4107,8 +4101,8 @@ int WebContentsImpl::CreateOpenerRenderViews(SiteInstance* instance) {
 
   // If this tab has an opener, ensure it has a RenderView in the given
   // SiteInstance as well.
-  if (opener_)
-    opener_route_id = opener_->CreateOpenerRenderViews(instance);
+  if (opener())
+    opener_route_id = opener()->CreateOpenerRenderViews(instance);
 
   // If any of the renderers (current, pending, or swapped out) for this
   // WebContents has the same SiteInstance, use it.