Allow MimeHandlerViewGuest be embedded inside OOPIFs

components/guest_view/browser/guest_view_manager.cc

 // Copyright 2014 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 "base/debug/stack_trace.h"

[Charlie Reis, 2016/10/18 22:52:20]

This belongs below.

[EhsanK, 2016/10/20 21:41:17]

Sorry for leaving this behind. Will remove it.

 #include "components/guest_view/browser/guest_view_manager.h"
 
 #include <tuple>
 #include <utility>
 
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "components/guest_view/browser/guest_view_base.h"
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/browser/guest_view_manager_factory.h"
@@ skipping 49 matching lines @@
 // static
 GuestViewManagerFactory* GuestViewManager::factory_ = nullptr;
 
 GuestViewManager::GuestViewManager(
     content::BrowserContext* context,
     std::unique_ptr<GuestViewManagerDelegate> delegate)
     : current_instance_id_(0),
       last_instance_id_removed_(0),
       context_(context),
       delegate_(std::move(delegate)),
+      attaching_guest_embedder_routing_id_(MSG_ROUTING_NONE),
       weak_ptr_factory_(this) {}
 
 GuestViewManager::~GuestViewManager() {}
 
 // static
 GuestViewManager* GuestViewManager::CreateWithDelegate(
     BrowserContext* context,
     std::unique_ptr<GuestViewManagerDelegate> delegate) {
   GuestViewManager* guest_manager = FromBrowserContext(context);
   if (!guest_manager) {
@@ skipping 19 matching lines @@
     int guest_instance_id,
     int embedder_render_process_id) {
   if (!CanEmbedderAccessInstanceIDMaybeKill(embedder_render_process_id,
                                             guest_instance_id)) {
     return nullptr;
   }
   return GetGuestByInstanceID(guest_instance_id);
 }
 
 void GuestViewManager::AttachGuest(int embedder_process_id,
+                                   int embedder_routing_id,
                                    int element_instance_id,
                                    int guest_instance_id,
                                    const base::DictionaryValue& attach_params) {
+  DCHECK_EQ(attaching_guest_embedder_routing_id_, MSG_ROUTING_NONE);
+  attaching_guest_embedder_routing_id_ = embedder_routing_id;
+
   auto* guest_view =
       GuestViewBase::From(embedder_process_id, guest_instance_id);
   if (!guest_view)
     return;
 
   ElementInstanceKey key(embedder_process_id, element_instance_id);
   auto it = instance_id_map_.find(key);
   // If there is an existing guest attached to the element, then destroy the
   // existing guest.
   if (it != instance_id_map_.end()) {
@@ skipping 330 matching lines @@
   // We might get some late arriving messages at tear down. Let's let the
   // embedder tear down in peace.
   auto it = guest_web_contents_by_instance_id_.find(guest_instance_id);
   if (it == guest_web_contents_by_instance_id_.end())
     return true;
 
   auto* guest_view = GuestViewBase::FromWebContents(it->second);
   if (!guest_view)
     return false;
 
-  return embedder_render_process_id ==
-      guest_view->owner_web_contents()->GetRenderProcessHost()->GetID();
+  int render_frame_routing_id = attaching_guest_embedder_routing_id_;
+  if (render_frame_routing_id == MSG_ROUTING_NONE) {
+    render_frame_routing_id =
+        guest_view->owner_web_contents()->GetMainFrame()->GetRoutingID();
+  }
+
+  if (embedder_render_process_id !=
+          guest_view->owner_web_contents()->GetRenderProcessHost()->GetID() &&
+      !guest_view->IsViewType("mimehandler")) {
+    // Only MimeHandlerViewGuest can be embedded in a cross origin frame.
+    return false;
+  }
+
+  content::WebContents* web_contents = guest_view->host()->RegisterEmbedderID(
+      embedder_render_process_id, render_frame_routing_id);

[Charlie Reis, 2016/10/18 22:52:20]

This doesn't seem obviously safe to me-- we're mixing a routing ID stored as
state with a process ID passed in by the caller.  It would be very easy to have
bugs where these don't correspond to the same RenderFrameHost.

[EhsanK, 2016/10/20 21:41:17]

This variable and logic is removed form here. I now use
MimeHandlerViewGuestCreatedCallback() to set these values as both embedder
process and frame id are provided there.

+  attaching_guest_embedder_routing_id_ = MSG_ROUTING_NONE;
+
+  return web_contents->GetRenderProcessHost()->GetID() ==
+         guest_view->owner_web_contents()->GetRenderProcessHost()->GetID();

[Charlie Reis, 2016/10/18 22:52:20]

nit: No need for the GetID() calls, since you can just compare the RPHs
directly.

Also, it seems like it's worth including a comment on what this block (from 484
down) is trying to do.

[EhsanK, 2016/10/20 21:41:17]

Acknowledged.

 }
 
 GuestViewManager::ElementInstanceKey::ElementInstanceKey()
     : embedder_process_id(content::ChildProcessHost::kInvalidUniqueID),
       element_instance_id(content::ChildProcessHost::kInvalidUniqueID) {
 }
 
 GuestViewManager::ElementInstanceKey::ElementInstanceKey(
     int embedder_process_id,
     int element_instance_id)
@@ skipping 17 matching lines @@
     const GuestViewCreateFunction& create_function,
     const GuestViewCleanUpFunction& cleanup_function)
     : create_function(create_function), cleanup_function(cleanup_function) {}
 
 GuestViewManager::GuestViewData::GuestViewData(const GuestViewData& other) =
     default;
 
 GuestViewManager::GuestViewData::~GuestViewData() {}
 
 }  // namespace guest_view