Make window.open() IPCs be frame-based

content/renderer/render_view_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/renderer/render_view_impl.h"
 
 #include <algorithm>
 #include <cmath>
 #include <memory>
 
@@ skipping 1346 matching lines @@
 }
 
 void RenderViewImpl::ShowCreatedFullscreenWidget(
     RenderWidget* fullscreen_widget,
     WebNavigationPolicy policy,
     const gfx::Rect& initial_rect) {
   Send(new ViewHostMsg_ShowFullscreenWidget(GetRoutingID(),
                                             fullscreen_widget->routing_id()));
 }
 
-void RenderViewImpl::ShowCreatedViewWidget(bool opened_by_user_gesture,
-                                           RenderWidget* render_view_to_show,
-                                           WebNavigationPolicy policy,
-                                           const gfx::Rect& initial_rect) {
-  // |render_view_to_show| represents a pending view opened (e.g. a popup opened
-  // via window.open()) by this object, but not yet shown (it's offscreen, and
-  // still owned by the opener). Sending |ViewHostMsg_ShowView| will move it off
-  // the opener's pending list, into e.g. its own tab or window.
-  Send(new ViewHostMsg_ShowView(GetRoutingID(),
-                                render_view_to_show->routing_id(),
-                                NavigationPolicyToDisposition(policy),
-                                initial_rect, opened_by_user_gesture));
-}
-
 void RenderViewImpl::SendFrameStateUpdates() {
   // We only use this path in OOPIF-enabled modes.
   DCHECK(SiteIsolationPolicy::UseSubframeNavigationEntries());
 
   // Tell each frame with pending state to send its UpdateState message.
   for (int render_frame_routing_id : frames_with_pending_state_) {
     RenderFrameImpl* frame =
         RenderFrameImpl::FromRoutingID(render_frame_routing_id);
     if (frame)
       frame->SendUpdateState();
@@ skipping 17 matching lines @@
 }
 
 // blink::WebViewClient ------------------------------------------------------
 
 WebView* RenderViewImpl::createView(WebLocalFrame* creator,
                                     const WebURLRequest& request,
                                     const WebWindowFeatures& features,
                                     const WebString& frame_name,
                                     WebNavigationPolicy policy,
                                     bool suppress_opener) {
+  RenderFrameImpl* creator_frame = RenderFrameImpl::FromWebFrame(creator);
   mojom::CreateNewWindowParamsPtr params = mojom::CreateNewWindowParams::New();
-  params->opener_id = GetRoutingID();
+  params->opener_render_frame_id = creator_frame->GetRoutingID();
   params->user_gesture = WebUserGestureIndicator::isProcessingUserGesture();
   if (GetContentClient()->renderer()->AllowPopup())
     params->user_gesture = true;
   params->window_container_type = WindowFeaturesToContainerType(features);
   params->session_storage_namespace_id = session_storage_namespace_id_;
   if (frame_name != "_blank")
     params->frame_name = base::UTF16ToUTF8(base::StringPiece16(frame_name));
-  params->opener_render_frame_id =
-      RenderFrameImpl::FromWebFrame(creator)->GetRoutingID();
   params->opener_url = creator->document().url();
 
   // The browser process uses the top frame's URL for a content settings check
   // to determine whether the popup is allowed.  If the top frame is remote,
   // its URL is not available, so use its replicated origin instead.
   //
   // TODO(alexmos): This works fine for regular origins but may break path
   // matching for file URLs with OOP subframes that open popups.  This should
   // be fixed by either moving this lookup to the browser process or removing
   // path-based matching for file URLs from content settings.  See
@@ skipping 41 matching lines @@
   ResizeParams initial_size = ResizeParams();
   initial_size.screen_info = screen_info_;
 
   // The initial hidden state for the RenderViewImpl here has to match what the
   // browser will eventually decide for the given disposition. Since we have to
   // return from this call synchronously, we just have to make our best guess
   // and rely on the browser sending a WasHidden / WasShown message if it
   // disagrees.
   mojom::CreateViewParams view_params;
 
-  RenderFrameImpl* creator_frame = RenderFrameImpl::FromWebFrame(creator);
   view_params.opener_frame_route_id = creator_frame->GetRoutingID();
   DCHECK_EQ(GetRoutingID(), creator_frame->render_view()->GetRoutingID());
 
   view_params.window_was_created_with_opener = true;
   view_params.renderer_preferences = renderer_preferences_;
   view_params.web_preferences = webkit_preferences_;
   view_params.view_id = reply->route_id;
   view_params.main_frame_routing_id = reply->main_frame_route_id;
   view_params.main_frame_widget_routing_id = reply->main_frame_widget_route_id;
   view_params.session_storage_namespace_id =
       reply->cloned_session_storage_namespace_id;
   view_params.swapped_out = false;
   // WebCore will take care of setting the correct name.
   view_params.replicated_frame_state = FrameReplicationState();
   view_params.hidden = is_background_tab;
   view_params.never_visible = never_visible;
   view_params.initial_size = initial_size;
   view_params.enable_auto_resize = false;
   view_params.min_size = gfx::Size();
   view_params.max_size = gfx::Size();
   view_params.page_zoom_level = page_zoom_level_;
 
+  // Unretained() is safe here because our calling function will also call
+  // show().

[alexmos, 2016/12/13 18:41:41]

Ack.  Earlier in the CL I was thinking whether we could now get a race where the
main frame creates a new window and swaps out to a proxy before the new window
is shown, but that doesn't seem possible, for this reason. 
blink::createNewWindow will always call show() synchronously after calling
createWindow, and it doesn't look like anything in between can destroy the
creator frame.

[ncarter (slow), 2016/12/15 00:33:17]

SYN/ACK. I actually spent a lot of time worrying about the lifetime here before
I realized how simple it actually was. I'd thought that the message loop might
run between create and show().

I'm now confident that we could probably get rid of the show() step altogether
for all widgets; but I'm not intending on trying myself.

   RenderWidget::ShowCallback show_callback =
-      base::Bind(&RenderViewImpl::ShowCreatedViewWidget, this->AsWeakPtr(),
-                 opened_by_user_gesture);
+      base::Bind(&RenderFrameImpl::ShowCreatedWindow,
+                 base::Unretained(creator_frame), opened_by_user_gesture);
 
   RenderViewImpl* view =
       RenderViewImpl::Create(compositor_deps_, view_params, show_callback);
 
   return view->webview();
 }
 
 WebWidget* RenderViewImpl::createPopupMenu(blink::WebPopupType popup_type) {
   RenderWidget* widget = RenderWidget::CreateForPopup(this, compositor_deps_,
                                                       popup_type, screen_info_);
@@ skipping 1289 matching lines @@
                             INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
   }
 
   std::unique_ptr<InputEventAck> ack(
       new InputEventAck(InputEventAckSource::MAIN_THREAD, input_event->type,
                         INPUT_EVENT_ACK_STATE_NOT_CONSUMED));
   OnInputEventAck(std::move(ack));
 }
 
 }  // namespace content