Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(410)

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

Created:
1 year ago by wjmaclean
Modified:
12 months 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -7 lines) Patch
M components/guest_view/renderer/guest_view_container.h View 1 2 2 chunks +1 line, -1 line 0 comments Download
M extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc View 1 2 3 4 5 6 chunks +33 lines, -6 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 42 (29 generated)
wjmaclean
I'm not sure if we should be concerned about other null pointers in AttachIFrameGuest(), but ...
1 year 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 ...
1 year 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 year 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 ...
1 year 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 year 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 year 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 year ago (2017-02-16 22:48:39 UTC) #20
wjmaclean
Ptal?
1 year 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 year 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 ...
1 year ago (2017-02-17 16:18:47 UTC) #33
lfg
lgtm
1 year 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 year ago (2017-02-17 17:20:37 UTC) #38
commit-bot: I haz the power
1 year 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...

Powered by Google App Engine
This is Rietveld 408576698