Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

BUG=590284
Committed: https://crrev.com/e5787005a9004d7be289cc649c6ae4f3051996cd
Cr-Commit-Position: refs/heads/master@{#378844}

[gzobqq, 2016-03-01 08:43:39]

Description was changed from

==========
Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

BUG=590284
==========

to

==========
Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

BUG=590284
==========

[gzobqq, 2016-03-01 08:43:56]

gzobqq@gmail.com changed reviewers:
+ nasko@chromium.org

[gzobqq, 2016-03-01 08:44:41]

nasko, PTAL

[nasko, 2016-03-01 17:13:44]

https://codereview.chromium.org/1747183002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1747183002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1473:
CHECK(!owner_delegate_);
Is the goal of this to help us discover further UaF bugs?

[gzobqq, 2016-03-01 18:30:13]

https://codereview.chromium.org/1747183002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1747183002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1473:
CHECK(!owner_delegate_);
On 2016/03/01 17:13:43, nasko wrote:
> Is the goal of this to help us discover further UaF bugs?


I would guess that if there is a way to fail this check, it won't happen by
accident. At least bug 590284 required deliberate steps from a compromised
renderer. RWHI ownership logic is fairly complicated. RWHI can be owned by RVHI,
RFHI or a WCI. Many paths call RWHI::Destroy(true). Should something get
confused and call it on a RWHI owned by RVHI, we'll UaF. I think the CHECK is a
good idea for protection.

Of course on the other hand it's not nice to clutter the code with CHECKs and
there's a perf hit. If you think we shouldn't have it, that's ok. I'll try to
verify the Destroy(true) calls at some point.

[nasko, 2016-03-01 22:47:47]

nasko@chromium.org changed reviewers:
+ avi@chromium.org

[nasko, 2016-03-01 22:47:48]

avi@ did all the work for refactoring RWH/RVH, so adding him to confirm it make
sense. Otherwise LGTM.

[Avi (use Gerrit), 2016-03-02 17:23:07]

lgtm

Yep, this makes sense, and it is worth the paranoia.

[gzobqq, 2016-03-02 20:33:51]

The CQ bit was checked by gzobqq@gmail.com

[commit-bot: I haz the power, 2016-03-02 20:34:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1747183002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1747183002/1

[commit-bot: I haz the power, 2016-03-02 21:50:25]

Description was changed from

==========
Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

BUG=590284
==========

to

==========
Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

BUG=590284
==========

[commit-bot: I haz the power, 2016-03-02 21:50:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-02 21:52:50]

Description was changed from

==========
Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

BUG=590284
==========

to

==========
Check that RWHI isn't deleted manually while owned by a scoped_ptr in RVHI

BUG=590284

Committed: https://crrev.com/e5787005a9004d7be289cc649c6ae4f3051996cd
Cr-Commit-Position: refs/heads/master@{#378844}
==========

[commit-bot: I haz the power, 2016-03-02 21:52:51]

Patchset 1 (id:??) landed as
https://crrev.com/e5787005a9004d7be289cc649c6ae4f3051996cd
Cr-Commit-Position: refs/heads/master@{#378844}