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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by wjmaclean
Modified:
2 months ago
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
I'm not sure if we should be concerned about other null pointers in AttachIFrameGuest(), but ...
2 months, 1 week ago (2017-02-16 17:00:57 UTC) #9
wjmaclean
On 2017/02/16 17:00:57, wjmaclean wrote: > I'm not sure if we should be concerned about ...
2 months, 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, ...
2 months, 1 week ago (2017-02-16 20:00:58 UTC) #13
wjmaclean
lfg@ - I think I've addressed your suggestions (though I couldn't find a WeakPtr already ...
2 months, 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| ...
2 months, 1 week ago (2017-02-16 22:09:14 UTC) #18
dcheng (OOO through May 2)
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 ...
2 months, 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 ...
2 months, 1 week ago (2017-02-16 22:48:39 UTC) #20
wjmaclean
Ptal?
2 months, 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.
2 months, 1 week ago (2017-02-17 16:06:10 UTC) #30
wjmaclean
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 ...
2 months, 1 week ago (2017-02-17 16:18:47 UTC) #33
lfg
lgtm
2 months, 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
2 months, 1 week ago (2017-02-17 17:20:37 UTC) #38
commit-bot: I haz the power
2 months, 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 cc6ac46