Index: extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc |
diff --git a/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc b/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc |
index bec7d1c852016119b6a3df2e7c81ffbda0047867..c060abad4b0c20bb272ab968a4a749c9924fc434 100644 |
--- a/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc |
+++ b/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc |
@@ -166,6 +166,20 @@ void GuestViewInternalCustomBindings::AttachGuest( |
args.Length() == 4 ? args[3].As<v8::Function>() |
: v8::Local<v8::Function>(), |
args.GetIsolate())); |
+ |
+ // 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). |
+ // TODO(wjmaclean): Evaluate if it's possible for malicious JS to *change* |
+ // the |guest_view_container| ... we guard against that below, but a simple |
+ // null check might suffice. |
+ // TODO(wjmaclean): Evaluate if it's worth removing the early-out above |
+ // and just do it here. |
+ if (guest_view_container != |
+ guest_view::GuestViewContainer::FromID(element_instance_id)) { |
lfg
2017/02/16 20:00:58
GuestViewContainer already has a weak ptr factory,
wjmaclean
2017/02/16 21:26:16
Done.
|
+ return; |
+ } |
guest_view_container->IssueRequest(std::move(request)); |
args.GetReturnValue().Set(v8::Boolean::New(context()->isolate(), true)); |
@@ -235,9 +249,17 @@ void GuestViewInternalCustomBindings::AttachIframeGuest( |
params->SetBoolean(guest_view::kElementSizeIsLogical, true); |
content::RenderFrame* render_frame = GetRenderFrame(args[3]); |
+ // It's possible that code executed during the |params| enumeration above |
lfg
2017/02/16 20:00:58
Can we use the same approach as above: Get the Ren
wjmaclean
2017/02/16 21:26:16
How about if we grab the parent_frame WebLocalFram
lfg
2017/02/16 22:09:14
I think this is fine, the WebFrame may be closed w
|
+ // could delete the guest view's RenderFrame, so we shouldn't blindly assume |
+ // it still exists. |
+ if (!render_frame) |
+ return; |
+ |
blink::WebLocalFrame* frame = render_frame->GetWebFrame(); |
// Parent must exist. |
+ // TODO(wjmaclean): Reviewers ... can you confirm that as long as the |
+ // GuestView object's RenderFrame exists, that it will have a local parent? |
lfg
2017/02/16 20:00:57
The frame comes from getIframeContentWindow in jav
wjmaclean
2017/02/16 21:26:16
Acknowledged.
|
blink::WebFrame* parent_frame = frame->parent(); |
DCHECK(parent_frame); |
DCHECK(parent_frame->isWebLocalFrame()); |