Make window.open() IPCs be frame-based

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 75654d2822d81030745104737bd04d316b615cdb..d19b7b13de0fc07491ff10c73cd4f110210fda85 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -64,9 +64,11 @@
 #include "content/common/input_messages.h"
 #include "content/common/inter_process_time_ticks_converter.h"
 #include "content/common/navigation_params.h"
+#include "content/common/render_message_filter.mojom.h"
 #include "content/common/renderer.mojom.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/swapped_out_messages.h"
+#include "content/common/view_messages.h"

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

Why is this needed?

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

Removed. Good catch.

 #include "content/public/browser/ax_event_notification_details.h"
 #include "content/public/browser/browser_accessibility_state.h"
 #include "content/public/browser/browser_context.h"
@@ -756,6 +758,7 @@ bool RenderFrameHostImpl::OnMessageReceived(const IPC::Message &msg) {
     IPC_MESSAGE_HANDLER(FrameHostMsg_ShowPopup, OnShowPopup)
     IPC_MESSAGE_HANDLER(FrameHostMsg_HidePopup, OnHidePopup)
 #endif
+    IPC_MESSAGE_HANDLER(FrameHostMsg_ShowCreatedWindow, OnShowCreatedWindow)
   IPC_END_MESSAGE_MAP()
 
   // No further actions here, since we may have been deleted.
@@ -1011,6 +1014,33 @@ void RenderFrameHostImpl::OnCreateChildFrame(
                         frame_owner_properties);
 }
 
+void RenderFrameHostImpl::OnCreateNewWindow(
+    int32_t render_view_route_id,
+    int32_t main_frame_route_id,
+    int32_t main_frame_widget_route_id,
+    const mojom::CreateNewWindowParams& params,
+    SessionStorageNamespace* session_storage_namespace) {
+  mojom::CreateNewWindowParamsPtr validated_params(params.Clone());
+  GetProcess()->FilterURL(false, &validated_params->target_url);
+
+  // TODO(nick): |opener_url|, |opener_security_origin|, and
+  // |opener_top_level_frame_url| should not be parameters; we can just use
+  // last_committed_url(), etc. Of these, |opener_top_level_frame_url| is
+  // particularly egregious, since an oopif isn't expected to know its top URL.

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

This cleanup seems important.  Is it worth filing a bug so we don't forget to
deal with it, or maybe we already have one?  (There's 466297 that I used for
opener_top_level_frame_url, but it has a different focus.)

Also, we never do FilterURL on the opener_top_level_frame_url, do we?  (IIRC,
it's used for content settings lookups on the IO thread, so don't know how much
of a problem that is.  But the IO thread work that precedes this is why it's not
going to be easy to just switch these to last_committed_url(), etc.)

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

I've filed a bug. The usage on the IO thread is what makes this tricky.

+  GetProcess()->FilterURL(false, &validated_params->opener_url);
+  GetProcess()->FilterURL(true, &validated_params->opener_security_origin);
+
+  // Ignore creation when sent from a frame that's not current.
+  if (frame_tree_node()->current_frame_host() == this) {

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

Can this be shortened to just is_active()?  (That covers pending delete, and we
can't ever get here from a pending RFH, can we?)

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

Good idea. Done.

+    delegate_->CreateNewWindow(GetSiteInstance(), render_view_route_id,
+                               main_frame_route_id, main_frame_widget_route_id,
+                               *validated_params, session_storage_namespace);
+  }
+
+  // Our caller (RenderWidgetHelper::OnCreateNewWindowOnUI) will send
+  // ViewMsg_Close if the above step did not adopt |main_frame_route_id|.
+}
+
 void RenderFrameHostImpl::OnDetach() {
   frame_tree_->RemoveFrame(frame_tree_node_);
 }
@@ -2178,6 +2208,14 @@ void RenderFrameHostImpl::OnHidePopup() {
 }
 #endif
 
+void RenderFrameHostImpl::OnShowCreatedWindow(int pending_widget_routing_id,
+                                              WindowOpenDisposition disposition,
+                                              const gfx::Rect& initial_rect,
+                                              bool user_gesture) {
+  delegate_->ShowCreatedWindow(GetProcess()->GetID(), pending_widget_routing_id,
+                               disposition, initial_rect, user_gesture);
+}
+
 void RenderFrameHostImpl::RegisterMojoInterfaces() {
   device::GeolocationServiceContext* geolocation_service_context =
       delegate_ ? delegate_->GetGeolocationServiceContext() : NULL;