Destroy WebWidgetTestClient when its WebWidget is destroyed.

Destroy WebWidgetTestClient when its WebWidget is destroyed.

This fixes a UaF trying to access the WebWidget from the WebWidgetTestClient.

BUG=610513
Committed: https://crrev.com/fdd53d6357138bcd0ad557bc5995f6e55952758c
Cr-Commit-Position: refs/heads/master@{#392928}

[lfg, 2016-05-10 18:45:50]

lfg@chromium.org changed reviewers:
+ lukasza@chromium.org

[lfg, 2016-05-10 18:45:50]

Hey Lukasz, please take a look.

[Łukasz Anforowicz, 2016-05-10 19:44:40]

WebTestProxy subclasses RenderViewImpl which implements *both* WebWidgetClient
and WebViewClient.  Lifetime of WebTestProxy is the same as the lifetime of
RenderViewImpl and WebViewTestClient and WebWidgetTestClient.  I think the core
problem we missed in the previous CL is that right now WebWidgetTestClient ends
up using a widget with a different  lifetime (i.e. lifetime that is not tied to
the lifetime of WebTestProxy/RenderView).  So (as you say in the comment in the
bug) - maybe for now (until RenderView and/or WebTestProxy are further split
widget/view-wise) we should set the WebView as the WebWidget in the WebTestProxy
(instead of the WebViewFrameWidget).

WDYT?

PS. I don't quite understand what widgets are and what their lifetimes are - I
am not sure if what I am saying above makes sense...  If there is a widgets 101
document/cheatsheet somewhere, then I would love to take a look :-)

https://codereview.chromium.org/1962393002/diff/1/components/test_runner/web_...
File components/test_runner/web_test_proxy.h (left):

https://codereview.chromium.org/1962393002/diff/1/components/test_runner/web_...
components/test_runner/web_test_proxy.h:84: DCHECK(!widget_test_client_);
This doesn't seem right - WebTestProxy dereferences widget_test_client without
any null checks (i.e. in screenInfo, scheduleAnimation, requestPointerLock
methods).

https://codereview.chromium.org/1962393002/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/layout_test_render_frame_observer.h
(right):

https://codereview.chromium.org/1962393002/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/layout_test_render_frame_observer.h:30: void
FrameDetached() override;
I wonder if this notification could be handled entirely within the
//components/test_runner layer (i.e. by overriding
blink::WebFrameClient::frameDetached method in test_runner::WebFrameTestClient +
test_runner::WebFrameTestProxy).

[lfg, 2016-05-10 21:13:02]

Please take another look.

On 2016/05/10 19:44:40, Łukasz Anforowicz wrote:
> WebTestProxy subclasses RenderViewImpl which implements *both* WebWidgetClient
> and WebViewClient.  Lifetime of WebTestProxy is the same as the lifetime of
> RenderViewImpl and WebViewTestClient and WebWidgetTestClient.  I think the
core
> problem we missed in the previous CL is that right now WebWidgetTestClient
ends
> up using a widget with a different  lifetime (i.e. lifetime that is not tied
to
> the lifetime of WebTestProxy/RenderView).  So (as you say in the comment in
the
> bug) - maybe for now (until RenderView and/or WebTestProxy are further split
> widget/view-wise) we should set the WebView as the WebWidget in the
WebTestProxy
> (instead of the WebViewFrameWidget).
> 
> WDYT?

Yes, I do think this is safer. Let's do it like this until we can work on a new
design for the test runner to keep track of all the widgets.

> PS. I don't quite understand what widgets are and what their lifetimes are - I
> am not sure if what I am saying above makes sense...  If there is a widgets
101
> document/cheatsheet somewhere, then I would love to take a look :-)

Not that I know :(

[Łukasz Anforowicz, 2016-05-10 22:15:37]

lgtm.  Thank you for adding linux_site_isolation bot to the mix of tryjobs :-)

[lfg, 2016-05-10 22:23:49]

lfg@chromium.org changed reviewers:
+ jochen@chromium.org

[lfg, 2016-05-10 22:23:50]

jochen, please take a look.

[jochen (gone - plz use gerrit), 2016-05-11 08:56:46]

lgtm

[lfg, 2016-05-11 15:23:36]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-05-11 15:23:36]

The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/1962393002/#ps40001
(title: "keep the TODO")

[commit-bot: I haz the power, 2016-05-11 15:23:56]

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

[commit-bot: I haz the power, 2016-05-11 15:27:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-11 15:28:19]

Description was changed from

==========
Destroy WebWidgetTestClient when its WebWidget is destroyed.

This fixes a UaF trying to access the WebWidget from the WebWidgetTestClient.

BUG=610513
==========

to

==========
Destroy WebWidgetTestClient when its WebWidget is destroyed.

This fixes a UaF trying to access the WebWidget from the WebWidgetTestClient.

BUG=610513

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

[commit-bot: I haz the power, 2016-05-11 15:28:20]

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