Track frame openers in FrameTreeNodes instead of WebContents

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/lazy_instance.h"
@@ skipping 186 matching lines @@
 }
 
 void SetAccessibilityModeOnFrame(AccessibilityMode mode,
                                  RenderFrameHost* frame_host) {
   static_cast<RenderFrameHostImpl*>(frame_host)->SetAccessibilityMode(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();
        it != session_storage_namespace_map.end();
        ++it) {
     new_contents->GetController()
         .SetSessionStorageNamespace(it->first, it->second.get());
   }
 
   new_contents->Init(params);
@@ skipping 55 matching lines @@
       render_frame_id(render_frame_id),
       chooser(chooser),
       identifier(identifier) {
 }
 
 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
       frame_tree_(new NavigatorImpl(&controller_, this),
                   this,
                   this,
                   this,
                   this),
       is_loading_(false),
       is_load_to_different_document_(false),
@@ skipping 107 matching lines @@
                     ResetWebContents());
 
   SetDelegate(NULL);
 
   STLDeleteContainerPairSecondPointers(destruction_observers_.begin(),
                                        destruction_observers_.end());
 }
 
 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).

 
   if (params.guest_delegate) {
     // This makes |new_contents| act as a guest.
     // For more info, see comment above class BrowserPluginGuest.
     BrowserPluginGuest::Create(new_contents, params.guest_delegate);
     // We are instantiating a WebContents for browser plugin. Set its subframe
     // bit to true.
     new_contents->is_subframe_ = true;
   }
   new_contents->Init(params);
@@ skipping 24 matching lines @@
   return result;
 }
 
 // static
 WebContentsImpl* WebContentsImpl::FromFrameTreeNode(
     FrameTreeNode* frame_tree_node) {
   return static_cast<WebContentsImpl*>(
       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();
 }
 
 bool WebContentsImpl::OnMessageReceived(RenderViewHost* render_view_host,
                                         const IPC::Message& message) {
   return OnMessageReceived(render_view_host, NULL, message);
 }
 
 bool WebContentsImpl::OnMessageReceived(RenderViewHost* render_view_host,
@@ skipping 667 matching lines @@
   GetMainFrame()->DispatchBeforeUnload(for_cross_site_transition);
 }
 
 void WebContentsImpl::Stop() {
   GetRenderManager()->Stop();
   FOR_EACH_OBSERVER(WebContentsObserver, observers_, NavigationStopped());
 }
 
 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_,
                     DidCloneToNewWebContents(this, tc));
   return tc;
 }
 
 void WebContentsImpl::Observe(int type,
                               const NotificationSource& source,
                               const NotificationDetails& details) {
@@ skipping 60 matching lines @@
     // Regular WebContentsView.
     view_.reset(CreateWebContentsView(
         this, delegate, &render_view_host_delegate_view_));
   }
   CHECK(render_view_host_delegate_view_);
   CHECK(view_.get());
 
   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));
 #endif
 
   registrar_.Add(this,
                  NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
                  NotificationService::AllBrowserContextsAndSources());
 
   screen_orientation_dispatcher_host_.reset(
@@ skipping 25 matching lines @@
   // main RenderFrameHost. It must be done here for main frames, since the
   // NotifySwappedFromRenderManager expects view_ to already be created and that
   // happens after RenderFrameHostManager::Init.
   NotifySwappedFromRenderManager(
       nullptr, GetRenderManager()->current_frame_host(), true);
 }
 
 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();
        ++iter) {
     if (iter->second != web_contents)
       continue;
     pending_contents_.erase(iter);
     return;
   }
   NOTREACHED();
@@ skipping 282 matching lines @@
     return;
   }
 
   // Create the new web contents. This will automatically create the new
   // WebContentsView. In the future, we may want to create the view separately.
   CreateParams create_params(GetBrowserContext(), site_instance.get());
   create_params.routing_id = route_id;
   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;
   create_params.renderer_initiated_creation = true;
 
   WebContentsImpl* new_contents = NULL;
   if (!is_guest) {
     create_params.context = view_->GetNativeView();
     create_params.initial_size = GetContainerBounds().size();
     new_contents = static_cast<WebContentsImpl*>(
@@ skipping 848 matching lines @@
 bool WebContentsImpl::GotResponseToLockMouseRequest(bool allowed) {
   if (GetBrowserPluginGuest())
     return GetBrowserPluginGuest()->LockMouse(allowed);
 
   return GetRenderViewHost()
              ? GetRenderViewHost()->GotResponseToLockMouseRequest(allowed)
              : false;
 }
 
 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) {
   if (!color_chooser_info_.get())
     return;
   RenderFrameHost* rfh = RenderFrameHost::FromID(
       color_chooser_info_->render_process_id,
       color_chooser_info_->render_frame_id);
   if (!rfh)
     return;
@@ skipping 1314 matching lines @@
   // Update the URL display.
   NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
 }
 
 void WebContentsImpl::DidChangeName(RenderFrameHost* render_frame_host,
                                     const std::string& name) {
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                     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_,
                     DocumentOnLoadCompletedInMainFrame());
 
   // TODO(avi): Remove. http://crbug.com/170921
   NotificationService::current()->Notify(
       NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
       Source<WebContents>(this),
       NotificationService::NoDetails());
@@ skipping 251 matching lines @@
 }
 
 void WebContentsImpl::NotifyMainFrameSwappedFromRenderManager(
     RenderViewHost* old_host,
     RenderViewHost* new_host) {
   NotifyViewSwapped(old_host, new_host);
 }
 
 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) {
   int opener_route_id = MSG_ROUTING_NONE;
 
   // 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.
   if (GetRenderManager()->current_host()->GetSiteInstance() == instance)
     return GetRenderManager()->current_host()->GetRoutingID();
 
   if (GetRenderManager()->pending_render_view_host() &&
       GetRenderManager()->pending_render_view_host()->GetSiteInstance() ==
           instance)
     return GetRenderManager()->pending_render_view_host()->GetRoutingID();
@@ skipping 289 matching lines @@
   player_map->erase(it);
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content