[ MimeHandlerView ] Fix a crash at attaching time (strike x3)

The browser still crashes in ResumeAttachOrDestroy which is most likely
due to UaF on a removed GuestView. The new hypothesis is that at the
time embedder frame goes away, GuestView does not still have a valid
|element_instance_id_|.

This CL moves on to storing WeakPtr of the MimeHandlerViewGuest instead
of raw pointers to avoid crashing at attach time.

TBR=wjmaclean@chromium.org

Bug: 959572
Change-Id: I347e6f058098e08dcae9acc84eeb3636e90916c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1629289
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Ɓukasz Anforowicz <lukasza@chromium.org>
Auto-Submit: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663313}
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.cc b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.cc
index d39063da..9ac3e3f 100644
--- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.cc
+++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.cc
@@ -126,19 +126,12 @@
     int32_t element_instance_id,
     bool is_full_page_plugin) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  pending_guests_[element_instance_id] = guest_view;
+  pending_guests_[element_instance_id] = guest_view->GetWeakPtr();
   outer_contents_frame->PrepareForInnerWebContentsAttach(base::BindOnce(
       &MimeHandlerViewAttachHelper::ResumeAttachOrDestroy,
       weak_factory_.GetWeakPtr(), element_instance_id, is_full_page_plugin));
 }
 
-void MimeHandlerViewAttachHelper::GuestEmbedderFrameGone(
-    int32_t element_instance_id) {
-  auto it = pending_guests_.find(element_instance_id);
-  if (it != pending_guests_.end())
-    pending_guests_.erase(it);
-}
-
 // static
 void MimeHandlerViewAttachHelper::CreateFullPageMimeHandlerView(
     int32_t frame_tree_node_id,
@@ -163,7 +156,9 @@
     bool is_full_page_plugin,
     content::RenderFrameHost* plugin_rfh) {
   DCHECK(!plugin_rfh || (plugin_rfh->GetProcess() == render_process_host_));
-  auto* guest_view = pending_guests_[element_instance_id];
+  auto guest_view = pending_guests_[element_instance_id];
+  if (!guest_view)
+    return;
   pending_guests_.erase(element_instance_id);
   if (!plugin_rfh) {
     mojom::MimeHandlerViewContainerManagerAssociatedPtr container_manager;
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.h b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.h
index 7d65dfe..5721655 100644
--- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.h
+++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_attach_helper.h
@@ -13,6 +13,7 @@
 #include "base/bind_helpers.h"
 #include "base/containers/flat_map.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/render_process_host_observer.h"
 #include "extensions/common/mojo/guest_view.mojom.h"
 
@@ -69,16 +70,6 @@
                                 int32_t element_instance_id,
                                 bool is_full_page_plugin);
 
-  // Called when MimeHandlerViewGuest is about to be destroyed due to its
-  // embedder frame going away. This is specifically relevant during attaching;
-  // from the time the GuestView starts the attach process
-  // (AttachToOuterWebContents) to when the inner WebContents of GuestView
-  // attaches to the outer WebContents, there is a gap where the embedder
-  // RenderFrameHost (parent frame to the outer contents frame) can go away.
-  // MimeHandlerViewAttachHelper should be notified to remove the guest from
-  // |pending_guests_|.
-  void GuestEmbedderFrameGone(int32_t element_instance_id);
-
  private:
   // Called after the content layer finishes preparing a frame for attaching to
   // the embedder WebContents. If |plugin_rfh| is nullptr then attaching is not
@@ -101,7 +92,13 @@
 
   MimeHandlerViewAttachHelper(content::RenderProcessHost* render_process_host);
 
-  base::flat_map<int32_t, MimeHandlerViewGuest*> pending_guests_;
+  // From the time the MimeHandlerViewGuest starts the attach process
+  // (AttachToOuterWebContents) to when the inner WebContents of GuestView
+  // attaches to the outer WebContents, there is a gap where the embedder
+  // RenderFrameHost (parent frame to the outer contents frame) can go away.
+  // Therefore, we keep a weak pointer to the GuestViews here (see
+  // https://crbug.com/959572).
+  base::flat_map<int32_t, base::WeakPtr<MimeHandlerViewGuest>> pending_guests_;
 
   content::RenderProcessHost* const render_process_host_;
 
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
index 59f6f7b..ffee5a4 100644
--- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
+++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
@@ -105,7 +105,8 @@
           ExtensionsAPIClient::Get()->CreateMimeHandlerViewGuestDelegate(this)),
       embedder_frame_process_id_(content::ChildProcessHost::kInvalidUniqueID),
       embedder_frame_routing_id_(MSG_ROUTING_NONE),
-      embedder_widget_routing_id_(MSG_ROUTING_NONE) {}
+      embedder_widget_routing_id_(MSG_ROUTING_NONE),
+      weak_factory_(this) {}
 
 MimeHandlerViewGuest::~MimeHandlerViewGuest() {
   // Before attaching is complete, the instance ID is not valid.
@@ -372,8 +373,6 @@
                                                     int routing_id) {
   if (process_id == embedder_frame_process_id_ &&
       routing_id == embedder_frame_routing_id_) {
-    MimeHandlerViewAttachHelper::Get(embedder_frame_process_id_)
-        ->GuestEmbedderFrameGone(element_instance_id());
     Destroy(/*also_delete=*/true);
   }
 }
@@ -489,4 +488,8 @@
                                           embedder_frame_routing_id_);
 }
 
+base::WeakPtr<MimeHandlerViewGuest> MimeHandlerViewGuest::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 }  // namespace extensions
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
index 94e23fe..9bdd744 100644
--- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
+++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
@@ -109,6 +109,8 @@
   // parent frame of the embedder frame (for post message).
   bool maybe_has_frame_container() const { return maybe_has_frame_container_; }
 
+  base::WeakPtr<MimeHandlerViewGuest> GetWeakPtr();
+
  protected:
   explicit MimeHandlerViewGuest(content::WebContents* owner_web_contents);
   ~MimeHandlerViewGuest() override;
@@ -197,6 +199,8 @@
   bool maybe_has_frame_container_ = false;
   mime_handler::BeforeUnloadControlPtrInfo pending_before_unload_control_;
 
+  base::WeakPtrFactory<MimeHandlerViewGuest> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(MimeHandlerViewGuest);
 };