Re-check existence of GuestViewContainer before requesting attach.

extensions/renderer/guest_view/guest_view_internal_custom_bindings.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 "extensions/renderer/guest_view/guest_view_internal_custom_bindings.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "components/guest_view/common/guest_view_constants.h"
 #include "components/guest_view/common/guest_view_messages.h"
 #include "components/guest_view/renderer/guest_view_request.h"
 #include "components/guest_view/renderer/iframe_guest_view_container.h"
 #include "components/guest_view/renderer/iframe_guest_view_request.h"
 #include "content/public/child/v8_value_converter.h"
 #include "content/public/renderer/render_frame.h"
+#include "content/public/renderer/render_frame_observer.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/guest_view/extensions_guest_view_messages.h"
 #include "extensions/renderer/guest_view/extensions_guest_view_container.h"
 #include "extensions/renderer/script_context.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebRemoteFrame.h"
@@ skipping 22 matching lines @@
   v8::Local<v8::Context> context =
       v8::Local<v8::Object>::Cast(value)->CreationContext();
   if (context.IsEmpty())
     return nullptr;
   blink::WebLocalFrame* frame = blink::WebLocalFrame::frameForContext(context);
   if (!frame)
     return nullptr;
   return content::RenderFrame::FromWebFrame(frame);
 }
 
+class RenderFrameStatus : public content::RenderFrameObserver {
+ public:
+  explicit RenderFrameStatus(content::RenderFrame* render_frame)
+      : content::RenderFrameObserver(render_frame) {}
+  ~RenderFrameStatus() final {}
+
+  bool is_ok() { return render_frame() != nullptr; }
+
+  // RenderFrameObserver implementation.
+  void OnDestruct() final {}
+};
+
 }  // namespace
 
 GuestViewInternalCustomBindings::GuestViewInternalCustomBindings(
     ScriptContext* context)
     : ObjectBackedNativeHandler(context) {
   RouteFunction("AttachGuest",
                 base::Bind(&GuestViewInternalCustomBindings::AttachGuest,
                            base::Unretained(this)));
   RouteFunction("DetachGuest",
                 base::Bind(&GuestViewInternalCustomBindings::DetachGuest,
@@ skipping 67 matching lines @@
 
   int element_instance_id = args[0]->Int32Value();
   // An element instance ID uniquely identifies a GuestViewContainer.
   auto* guest_view_container =
       guest_view::GuestViewContainer::FromID(element_instance_id);
 
   // TODO(fsamuel): Should we be reporting an error if the element instance ID
   // is invalid?
   if (!guest_view_container)
     return;
+  // Retain a weak pointer so we can easily test if the container goes away.
+  auto weak_ptr = guest_view_container->GetWeakPtr();
 
   int guest_instance_id = args[1]->Int32Value();
 
   std::unique_ptr<base::DictionaryValue> params;
   {
     std::unique_ptr<V8ValueConverter> converter(V8ValueConverter::create());
     std::unique_ptr<base::Value> params_as_value(
         converter->FromV8Value(args[2], context()->v8_context()));
     params = base::DictionaryValue::From(std::move(params_as_value));
     CHECK(params);
   }
+  // We should be careful that some malicious JS in the GuestView's embedder
+  // hasn't destroyed |guest_view_container| during the enumeration of the
+  // properties of the guest's object during extraction of |params| above
+  // (see https://crbug.com/683523).
+  if (!weak_ptr)
+    return;
 
   // Add flag to |params| to indicate that the element size is specified in
   // logical units.
   params->SetBoolean(guest_view::kElementSizeIsLogical, true);
 
   std::unique_ptr<guest_view::GuestViewRequest> request(
       new guest_view::GuestViewAttachRequest(
           guest_view_container, guest_instance_id, std::move(params),
           args.Length() == 4 ? args[3].As<v8::Function>()
                              : v8::Local<v8::Function>(),
@@ skipping 46 matching lines @@
   CHECK(args[2]->IsObject());
   // <iframe>.contentWindow.
   CHECK(args[3]->IsObject());
   // Optional Callback Function.
   CHECK(args.Length() <= num_required_params ||
         args[num_required_params]->IsFunction());
 
   int element_instance_id = args[0]->Int32Value();
   int guest_instance_id = args[1]->Int32Value();
 
+  // Get the WebLocalFrame before (possibly) executing any user-space JS while
+  // getting the |params|. We track the status of the RenderFrame via an
+  // observer in case it is deleted during user code execution.
+  content::RenderFrame* render_frame = GetRenderFrame(args[3]);
+  RenderFrameStatus render_frame_status(render_frame);
+
   std::unique_ptr<base::DictionaryValue> params;
   {
     std::unique_ptr<V8ValueConverter> converter(V8ValueConverter::create());
     std::unique_ptr<base::Value> params_as_value(
         converter->FromV8Value(args[2], context()->v8_context()));
     params = base::DictionaryValue::From(std::move(params_as_value));
     CHECK(params);
   }
+  if (!render_frame_status.is_ok())
+    return;
+
+  blink::WebLocalFrame* frame = render_frame->GetWebFrame();
+  // Parent must exist.
+  blink::WebFrame* parent_frame = frame->parent();
+  DCHECK(parent_frame);
+  DCHECK(parent_frame->isWebLocalFrame());
 
   // Add flag to |params| to indicate that the element size is specified in
   // logical units.
   params->SetBoolean(guest_view::kElementSizeIsLogical, true);
 
-  content::RenderFrame* render_frame = GetRenderFrame(args[3]);
-  blink::WebLocalFrame* frame = render_frame->GetWebFrame();
-
-  // Parent must exist.
-  blink::WebFrame* parent_frame = frame->parent();
-  DCHECK(parent_frame);
-  DCHECK(parent_frame->isWebLocalFrame());
-
   content::RenderFrame* embedder_parent_frame =
       content::RenderFrame::FromWebFrame(parent_frame);
 
+  // It's possible that code executed during the |params| enumeration above
+  // could delete the guest view's RenderFrame, so we shouldn't blindly assume
+  // it still exists.
+  if (!embedder_parent_frame)

[lfg, 2017/02/17 16:06:10]

This shouldn't be needed anymore.

[wjmaclean, 2017/02/17 16:18:47]

Removed.

+    return;
+
   // Create a GuestViewContainer if it does not exist.
   // An element instance ID uniquely identifies an IframeGuestViewContainer
   // within a RenderView.
   auto* guest_view_container =
       guest_view::GuestViewContainer::FromID(element_instance_id);
   // This is the first time we hear about the |element_instance_id|.
   DCHECK(!guest_view_container);
   // The <webview> element's GC takes ownership of |guest_view_container|.
   guest_view_container =
       new guest_view::IframeGuestViewContainer(embedder_parent_frame);
@@ skipping 173 matching lines @@
   // EnterFullscreen() and do it directly rather than having a generic "run with
   // user gesture" function.
   blink::WebScopedUserGesture user_gesture(context()->web_frame());
   CHECK_EQ(args.Length(), 1);
   CHECK(args[0]->IsFunction());
   context()->SafeCallFunction(
       v8::Local<v8::Function>::Cast(args[0]), 0, nullptr);
 }
 
 }  // namespace extensions