Chromium Code Reviews
Help | Chromium Project | Sign in
(14)

Issue 2695633008: Re-check existence of GuestViewContainer before requesting attach. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by wjmaclean (ooo Mar 15-26)
Modified:
1 month ago
Reviewers:
kenrb, dcheng, lfg
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Re-check existence of GuestViewContainer before requesting attach. During GuestView attach, it's possible for the GuestViewContainer pointer to become stale while enumerating the GuestView object's properties, resulting in a use-after-free when the attach request is queued. This CL forces a re-check that the container pointer is valid after the property enumeration takes place, and also checks that the container hasn't changed. In the case of AttachIFrameGuest(), it verifies that the RenderFrame containing the GuestView object doesn't disappear during |params| enumeration. BUG=683523 Review-Url: https://codereview.chromium.org/2695633008 Cr-Commit-Position: refs/heads/master@{#451330} Committed: https://chromium.googlesource.com/chromium/src/+/87dd40c9b8ae1fb8c3eb15bf527ee464bd1c1bdd

Patch Set 1 #

Patch Set 2 : Add checks for RenderFrame in AttachIFrameGuest. #

Total comments: 7

Patch Set 3 : Address suggestions. #

Patch Set 4 : Use RenderFrameObserver instead. #

Patch Set 5 : Fix Windows compile. #

Total comments: 2

Patch Set 6 : Remove unnecessary check. #

Messages

Total messages: 42 (29 generated)
wjmaclean (ooo Mar 15-26)
I'm not sure if we should be concerned about other null pointers in AttachIFrameGuest(), but ...
1 month, 1 week ago (2017-02-16 17:00:57 UTC) #9
wjmaclean (ooo Mar 15-26)
On 2017/02/16 17:00:57, wjmaclean wrote: > I'm not sure if we should be concerned about ...
1 month, 1 week ago (2017-02-16 17:01:32 UTC) #10
lfg
https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc (right): https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc#newcode180 extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:180: guest_view::GuestViewContainer::FromID(element_instance_id)) { GuestViewContainer already has a weak ptr factory, ...
1 month, 1 week ago (2017-02-16 20:00:58 UTC) #13
wjmaclean (ooo Mar 15-26)
lfg@ - I think I've addressed your suggestions (though I couldn't find a WeakPtr already ...
1 month, 1 week ago (2017-02-16 21:26:16 UTC) #15
lfg
https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc (right): https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc#newcode252 extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:252: // It's possible that code executed during the |params| ...
1 month, 1 week ago (2017-02-16 22:09:14 UTC) #18
dcheng
On 2017/02/16 22:09:14, lfg wrote: > https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc > File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc > (right): > > https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc#newcode252 ...
1 month, 1 week ago (2017-02-16 22:29:14 UTC) #19
lfg
On 2017/02/16 22:29:14, dcheng wrote: > > I think we probably don't want to rely ...
1 month, 1 week ago (2017-02-16 22:48:39 UTC) #20
wjmaclean (ooo Mar 15-26)
Ptal?
1 month, 1 week ago (2017-02-17 15:01:08 UTC) #24
lfg
https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc (right): https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc#newcode278 extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:278: if (!embedder_parent_frame) This shouldn't be needed anymore.
1 month, 1 week ago (2017-02-17 16:06:10 UTC) #30
wjmaclean (ooo Mar 15-26)
https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc (right): https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc#newcode278 extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:278: if (!embedder_parent_frame) On 2017/02/17 16:06:10, lfg wrote: > This ...
1 month, 1 week ago (2017-02-17 16:18:47 UTC) #33
lfg
lgtm
1 month, 1 week ago (2017-02-17 16:29:54 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2695633008/100001
1 month, 1 week ago (2017-02-17 17:20:37 UTC) #38
commit-bot: I haz the power
1 month, 1 week ago (2017-02-17 17:30:55 UTC) #41
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/87dd40c9b8ae1fb8c3eb15bf527e...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62