Extract WebFrameClient implementation out of WebTestProxyBase.

Extract WebFrameClient implementation out of WebTestProxyBase.

Overview
========

This CL extracts WebFrameClient-specific parts of WebTestProxyBase into
a new WebFrameTestClient class.

This CL is part of a sequence of CLs that make progress toward:

1) Splitting WebTestProxyBase into separate WebFrameClient /
   WebWidgetClient / WebViewClient focused classes

2) Removing WebTestProxyBase from public API of components/test_runner
   (and having WebTestProxy and WebFrameTestProxy consume instead
   abstract WebFrameClient / WebWidgetClient / WebViewClient interfaces).

Details
=======

Most of the change is mechanical move of code from WebTestProxyBase
into the new WebFrameTestClient (slightly tweaking method signatures
as needed to make them fit WebFrameClient interface - note camelCasing
and extra parameters here and there).

Additionally some helper methods (i.e. URLDescription and
WebNavigationPolicyToString) are now needed by both WebTestProxyBase and
by WebFrameTestClient.  Consequently, to facilitate code reuse, these
helper methods were moved to test_common.cc.

Finally, in order to decouple WebFrameTestClient from view-specific
WebTestProxyBase the CL does the following:

- It moves counting currently active color choosers from
  WebTestProxyBase into TestRunner (and adjusts MockColorChooser
  accordingly).

- It moves ownership of MockWebUserMediaClient from
  WebTestProxyBase into TestRunner.  This is quite natural, as
  MockWebUserMediaClient depends only on WebTestDelegate::PostTask and
  WebTestDelegate::AddMediaStreamAudioSourceAndTrack (which are not
  view-specific).

- Replacing WebFrameTestProxy::set_base_proxy with
  WebFrameTestProxy::set_test_client requires refactoring how higher
  layers provide the necessary object - in particular after the CL the
  WebFrame[Test]Client cannot be extracted from WebTestProxy/RenderView
  (because the whole point of this CL is to remove the coupling between
  view-specific WebTestProxy and frame-focused WebFrameTestProxy).  This
  part of the refactoring is accomplished by adding a frame-proxy
  creation callback that can be provided to EnableWebTestProxyCreation
  (similarily how it already was accepting equivalent view-proxy
  creation callback).  This refactoring also necessitates extracting a
  WebFrameTestProxyBase (to have a concrete/non-templatized type that
  content/shell/.../layout_tests layer can take a dependency on).

BUG=595089
Committed: https://crrev.com/11e459b088ab4736f10776a3831d0b9ddf249517
Cr-Commit-Position: refs/heads/master@{#384044}

[Łukasz Anforowicz, 2016-03-24 16:40:39]

Description was changed from

==========
Extract WebFrameClient implementation out of WebTestProxyBase.

This CL makes progress toward
1) Splitting WebTestProxyBase into separate WebFrameClient /
   WebWidgetClient / WebViewClient focused classes
2) Remove WebTestProxyBase from public API of components/test_runner
   (and having WebTestProxy and WebFrameTestProxy consume instead
   abstract WebFrameClient / WebWidgetClient / WebViewClient interfaces).

This CL extracts WebFrameClient-specific parts of WebTestProxyBase into
a new WebFrameTestClient class.

BUG=595089
==========

to

==========
Extract WebFrameClient implementation out of WebTestProxyBase.

Overview
========

This CL extracts WebFrameClient-specific parts of WebTestProxyBase into
a new WebFrameTestClient class.

This CL is part of a sequence of CLs that make progress toward:

1) Splitting WebTestProxyBase into separate WebFrameClient /
   WebWidgetClient / WebViewClient focused classes

2) Removing WebTestProxyBase from public API of components/test_runner
   (and having WebTestProxy and WebFrameTestProxy consume instead
   abstract WebFrameClient / WebWidgetClient / WebViewClient interfaces).


Details
=======

Most of the change is mechanical move of code from WebTestProxyBase
into the new WebFrameTestClient (slightly tweaking method signatures
as needed to make them fit WebFrameClient interface - note camelCasing
and extra parameters here and there).

Additionally some helper methods (i.e. URLDescription and
WebNavigationPolicyToString) are now needed by both WebTestProxyBase and
by WebFrameTestClient.  Consequently, to facilitate code reuse, these
helper methods were moved to test_common.cc.

Finally, in order to decouple WebFrameTestClient from view-specific
WebTestProxyBase the CL does the following:

- It moves counting currently active color choosers from
  WebTestProxyBase into TestRunner (and adjusts MockColorChooser
  accordingly).

- It moves ownership of MockWebUserMediaClient from
  WebTestProxyBase into TestRunner.  This is quite natural, as
  MockWebUserMediaClient depends only on WebTestDelegate::PostTask and
  WebTestDelegate::AddMediaStreamAudioSourceAndTrack (which are not
  view-specific).

- Replacing WebFrameTestProxy::set_base_proxy with
  WebFrameTestProxy::set_test_client requires refactoring how higher
  layers provide the necessary object - in particular after the CL the
  WebFrame[Test]Client cannot be extracted from WebTestProxy/RenderView
  (because the whole point of this CL is to remove the coupling between
  view-specific WebTestProxy and frame-focused WebFrameTestProxy).  This
  part of the refactoring is accomplished by adding a frame-proxy
  creation callback that can be provided to EnableWebTestProxyCreation
  (similarily how it already was accepting equivalent view-proxy
  creation callback).  This refactoring also necessitates extracting a
  WebFrameTestProxyBase (to have a concrete/non-templatized type that
  content/shell/.../layout_tests layer can take a dependency on).


BUG=595089
==========

[Łukasz Anforowicz, 2016-03-24 22:48:54]

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

[Łukasz Anforowicz, 2016-03-24 22:48:55]

jochen@, can you please take a look?

[jochen (gone - plz use gerrit), 2016-03-30 16:23:52]

lgtm

[Łukasz Anforowicz, 2016-03-30 18:03:25]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-03-30 18:03:58]

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

[commit-bot: I haz the power, 2016-03-30 19:08:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-30 19:09:58]

Description was changed from

==========
Extract WebFrameClient implementation out of WebTestProxyBase.

Overview
========

This CL extracts WebFrameClient-specific parts of WebTestProxyBase into
a new WebFrameTestClient class.

This CL is part of a sequence of CLs that make progress toward:

1) Splitting WebTestProxyBase into separate WebFrameClient /
   WebWidgetClient / WebViewClient focused classes

2) Removing WebTestProxyBase from public API of components/test_runner
   (and having WebTestProxy and WebFrameTestProxy consume instead
   abstract WebFrameClient / WebWidgetClient / WebViewClient interfaces).


Details
=======

Most of the change is mechanical move of code from WebTestProxyBase
into the new WebFrameTestClient (slightly tweaking method signatures
as needed to make them fit WebFrameClient interface - note camelCasing
and extra parameters here and there).

Additionally some helper methods (i.e. URLDescription and
WebNavigationPolicyToString) are now needed by both WebTestProxyBase and
by WebFrameTestClient.  Consequently, to facilitate code reuse, these
helper methods were moved to test_common.cc.

Finally, in order to decouple WebFrameTestClient from view-specific
WebTestProxyBase the CL does the following:

- It moves counting currently active color choosers from
  WebTestProxyBase into TestRunner (and adjusts MockColorChooser
  accordingly).

- It moves ownership of MockWebUserMediaClient from
  WebTestProxyBase into TestRunner.  This is quite natural, as
  MockWebUserMediaClient depends only on WebTestDelegate::PostTask and
  WebTestDelegate::AddMediaStreamAudioSourceAndTrack (which are not
  view-specific).

- Replacing WebFrameTestProxy::set_base_proxy with
  WebFrameTestProxy::set_test_client requires refactoring how higher
  layers provide the necessary object - in particular after the CL the
  WebFrame[Test]Client cannot be extracted from WebTestProxy/RenderView
  (because the whole point of this CL is to remove the coupling between
  view-specific WebTestProxy and frame-focused WebFrameTestProxy).  This
  part of the refactoring is accomplished by adding a frame-proxy
  creation callback that can be provided to EnableWebTestProxyCreation
  (similarily how it already was accepting equivalent view-proxy
  creation callback).  This refactoring also necessitates extracting a
  WebFrameTestProxyBase (to have a concrete/non-templatized type that
  content/shell/.../layout_tests layer can take a dependency on).


BUG=595089
==========

to

==========
Extract WebFrameClient implementation out of WebTestProxyBase.

Overview
========

This CL extracts WebFrameClient-specific parts of WebTestProxyBase into
a new WebFrameTestClient class.

This CL is part of a sequence of CLs that make progress toward:

1) Splitting WebTestProxyBase into separate WebFrameClient /
   WebWidgetClient / WebViewClient focused classes

2) Removing WebTestProxyBase from public API of components/test_runner
   (and having WebTestProxy and WebFrameTestProxy consume instead
   abstract WebFrameClient / WebWidgetClient / WebViewClient interfaces).

Details
=======

Most of the change is mechanical move of code from WebTestProxyBase
into the new WebFrameTestClient (slightly tweaking method signatures
as needed to make them fit WebFrameClient interface - note camelCasing
and extra parameters here and there).

Additionally some helper methods (i.e. URLDescription and
WebNavigationPolicyToString) are now needed by both WebTestProxyBase and
by WebFrameTestClient.  Consequently, to facilitate code reuse, these
helper methods were moved to test_common.cc.

Finally, in order to decouple WebFrameTestClient from view-specific
WebTestProxyBase the CL does the following:

- It moves counting currently active color choosers from
  WebTestProxyBase into TestRunner (and adjusts MockColorChooser
  accordingly).

- It moves ownership of MockWebUserMediaClient from
  WebTestProxyBase into TestRunner.  This is quite natural, as
  MockWebUserMediaClient depends only on WebTestDelegate::PostTask and
  WebTestDelegate::AddMediaStreamAudioSourceAndTrack (which are not
  view-specific).

- Replacing WebFrameTestProxy::set_base_proxy with
  WebFrameTestProxy::set_test_client requires refactoring how higher
  layers provide the necessary object - in particular after the CL the
  WebFrame[Test]Client cannot be extracted from WebTestProxy/RenderView
  (because the whole point of this CL is to remove the coupling between
  view-specific WebTestProxy and frame-focused WebFrameTestProxy).  This
  part of the refactoring is accomplished by adding a frame-proxy
  creation callback that can be provided to EnableWebTestProxyCreation
  (similarily how it already was accepting equivalent view-proxy
  creation callback).  This refactoring also necessitates extracting a
  WebFrameTestProxyBase (to have a concrete/non-templatized type that
  content/shell/.../layout_tests layer can take a dependency on).

BUG=595089

Committed: https://crrev.com/11e459b088ab4736f10776a3831d0b9ddf249517
Cr-Commit-Position: refs/heads/master@{#384044}
==========

[commit-bot: I haz the power, 2016-03-30 19:09:59]

Patchset 5 (id:??) landed as
https://crrev.com/11e459b088ab4736f10776a3831d0b9ddf249517
Cr-Commit-Position: refs/heads/master@{#384044}

[kochi, 2016-03-31 08:14:33]

kochi@chromium.org changed reviewers:
+ kochi@chromium.org

[kochi, 2016-03-31 08:14:34]

Hi Łukasz,

I bumped into a use-after-free today, probably caused by this change.
Attached an AddressSanitizer-enabled crash log in the comment.

I built the content_shell on Linux and run layout tests, like

% blink/tools/run_layout_tests.py -t Default --clobber-old-results
--time-out-ms=100000 --no-retry-failures --additional-driver-flag --no-sandbox
-f fast/dom/shadow

To reproduce the UAF, "-f" flag (run tests in parallel)
was necessary.

https://codereview.chromium.org/1821923003/diff/80001/components/test_runner/...
File components/test_runner/mock_color_chooser.cc (right):

https://codereview.chromium.org/1821923003/diff/80001/components/test_runner/...
components/test_runner/mock_color_chooser.cc:52: client_ = nullptr;
I'm seeing this line is causing a use-after-free.

crash log for unknown process name (pid <unknown>):
STDOUT: PASS testRunner.isChooserShown() is false
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS testRunner.isChooserShown() is true
STDOUT: PASS successfullyParsed is true
STDOUT: 
STDOUT: TEST COMPLETE
STDOUT: 
STDERR: OUTPUT CONTAINS "AddressSanitizer", so we are treating this test as if
it crashed, even though it did not.
STDERR: 
STDERR: =================================================================
STDERR: ==11084==ERROR: AddressSanitizer: heap-use-after-free on address
0x60600015d588 at pc 0x7f740913491a bp 0x7ffd3dc75750 sp 0x7ffd3dc75748
STDERR: WRITE of size 8 at 0x60600015d588 thread T0 (content_shell)
STDERR:     #0 0x7f7409134919 in ??
./out/Default/../../components/test_runner/mock_color_chooser.cc:52:11
STDERR:     #1 0x7f7409134d36 in ??
./out/Default/../../components/test_runner/mock_color_chooser.cc:21:32
STDERR:     #2 0x7f7409134f84 in ??
./out/Default/../../components/test_runner/web_task.h:61:7

[kochi, 2016-03-31 08:18:13]

> crash log for unknown process name (pid <unknown>):
> STDOUT: PASS testRunner.isChooserShown() is false
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS testRunner.isChooserShown() is true
> STDOUT: PASS successfullyParsed is true
> STDOUT: 
> STDOUT: TEST COMPLETE
> STDOUT: 
> STDERR: OUTPUT CONTAINS "AddressSanitizer", so we are treating this test as if
> it crashed, even though it did not.
> STDERR: 
> STDERR: =================================================================
> STDERR: ==11084==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x60600015d588 at pc 0x7f740913491a bp 0x7ffd3dc75750 sp 0x7ffd3dc75748
> STDERR: WRITE of size 8 at 0x60600015d588 thread T0 (content_shell)
> STDERR:     #0 0x7f7409134919 in ??
> ./out/Default/../../components/test_runner/mock_color_chooser.cc:52:11
> STDERR:     #1 0x7f7409134d36 in ??
> ./out/Default/../../components/test_runner/mock_color_chooser.cc:21:32
> STDERR:     #2 0x7f7409134f84 in ??
> ./out/Default/../../components/test_runner/web_task.h:61:7

FYI this log is for fast/dom/shadow/input-color-in-content.html

[kochi, 2016-03-31 08:42:08]

On 2016/03/31 08:18:13, kochi wrote:
> > crash log for unknown process name (pid <unknown>):
> > STDOUT: PASS testRunner.isChooserShown() is false
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS testRunner.isChooserShown() is true
> > STDOUT: PASS successfullyParsed is true
> > STDOUT: 
> > STDOUT: TEST COMPLETE
> > STDOUT: 
> > STDERR: OUTPUT CONTAINS "AddressSanitizer", so we are treating this test as
if
> > it crashed, even though it did not.
> > STDERR: 
> > STDERR: =================================================================
> > STDERR: ==11084==ERROR: AddressSanitizer: heap-use-after-free on address
> > 0x60600015d588 at pc 0x7f740913491a bp 0x7ffd3dc75750 sp 0x7ffd3dc75748
> > STDERR: WRITE of size 8 at 0x60600015d588 thread T0 (content_shell)
> > STDERR:     #0 0x7f7409134919 in ??
> > ./out/Default/../../components/test_runner/mock_color_chooser.cc:52:11
> > STDERR:     #1 0x7f7409134d36 in ??
> > ./out/Default/../../components/test_runner/mock_color_chooser.cc:21:32
> > STDERR:     #2 0x7f7409134f84 in ??
> > ./out/Default/../../components/test_runner/web_task.h:61:7
> 
> FYI this log is for fast/dom/shadow/input-color-in-content.html

And FYI2: I was synced at fe9ac99fce086f398f741722533e95e28ce2aaf8 on Chromium
git repository.

[jochen (gone - plz use gerrit), 2016-03-31 08:42:59]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1841833005/ by jochen@chromium.org.

The reason for reverting is: use after free in color chooser.

[chromium-reviews, 2016-03-31 14:21:03]

Thanks for the heads-up and the revert attempt.  I'll revert the chain of 3
CLs that depend on each other (and prevented jochen@'s revert from applying
cleanly).

On Thu, Mar 31, 2016 at 1:42 AM, <jochen@chromium.org> wrote:

> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/1841833005/ by jochen@chromium.org.
>
> The reason for reverting is: use after free in color chooser.
>
> https://codereview.chromium.org/1821923003/
>



-- 
Thanks,

Lukasz

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.