Fix client-side phishing detection test flakiness and ChromeOS failure.

Fix client-side phishing detection test flakiness and ChromeOS failure.

The problem is that RenderViewFakeResourcesTest creates a RenderThreadImpl, but without setting up the sandbox environment, specifically the domain socket used on Linux.  This caused the sandbox's pre-assigned file descriptor to be taken by other files or sockets, leading to a hang when the sandbox code tries to talk to the browser.

To fix this, add a way for tests to disable the WebSandboxSupport implementation in RendererWebKitPlatformSupportImpl.

The problem also affects WebRTCAudioDeviceTest, so fixed it there as well.

BUG=128705
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140631

[jochen (gone - plz use gerrit), 2012-06-01 12:11:37]

drive-by content nits

http://codereview.chromium.org/10449094/diff/9001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/10449094/diff/9001/content/content_tests.gypi#...
content/content_tests.gypi:87: 'test/render_test_utils.cc',
I'd name this file test_renderer_thread.{cc,h}

http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
File content/test/render_test_utils.h (right):

http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
content/test/render_test_utils.h:23: };
DISALLOW_COPY_AND_ASSING, here and below?

[agl, 2012-06-01 12:52:14]

Seems reasonable to me, but I have to defer to jam who actually knows something
about this code :)

[Brian Ryner, 2012-06-01 18:06:34]

http://codereview.chromium.org/10449094/diff/9001/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/10449094/diff/9001/content/content_tests.gypi#...
content/content_tests.gypi:87: 'test/render_test_utils.cc',
On 2012/06/01 12:11:38, jochen wrote:
> I'd name this file test_renderer_thread.{cc,h}

test_renderer_thread or test_render_thread?  (RenderThread and RenderThreadImpl
don't put the extra "er").

[jam, 2012-06-01 22:11:24]

http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
File content/test/render_test_utils.h (right):

http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
content/test/render_test_utils.h:15: #include
"content/renderer/renderer_webkitplatformsupport_impl.h"
we don't allow headers in content/test to "leak through" internal headers of
content. i.e. tests in src/chrome shouldn't need to include internal content
files, just like they shouldn't include webcore files

[Brian Ryner, 2012-06-01 23:06:07]

Hm, so render_view_test.h was already pulling in
renderer_webkitplatformsupport_impl.h.  The tests that I changed under chrome/
all make use of the RendererWebKitPlatformSupportImpl subclass that is being
defined here (which I'm just moving to a different header).  So, I'm not sure
how to break this dependency -- perhaps a factory function that just returns a
|WebKitPlatformSupportImpl*|?

The render_thread_impl.h situation could be solved the same way, or perhaps by
just separating that out into a new header under content/renderer/.

Thoughts?

On 2012/06/01 22:11:24, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> File content/test/render_test_utils.h (right):
> 
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> content/test/render_test_utils.h:15: #include
> "content/renderer/renderer_webkitplatformsupport_impl.h"
> we don't allow headers in content/test to "leak through" internal headers of
> content. i.e. tests in src/chrome shouldn't need to include internal content
> files, just like they shouldn't include webcore files

[jam, 2012-06-02 17:17:58]

On 2012/06/01 23:06:07, Brian Ryner wrote:
> Hm, so render_view_test.h was already pulling in
> renderer_webkitplatformsupport_impl.h.  The tests that I changed under chrome/
> all make use of the RendererWebKitPlatformSupportImpl subclass that is being
> defined here (which I'm just moving to a different header).  So, I'm not sure
> how to break this dependency -- perhaps a factory function that just returns a
> |WebKitPlatformSupportImpl*|?

yeah, a factory function sgtm. apologies for the confusing state, the content
test situation is still being worked on and so there were stuff like this in
intermediate state. I was waiting to finish content_browsertest before
tightening all these deps rules, but that was perhaps not the right order.

> 
> The render_thread_impl.h situation could be solved the same way, or perhaps by
> just separating that out into a new header under content/renderer/.

This change motivated me to move all the public headers from content/test to
content/public/test, that way checkdeps can enforce that internal headers don't
leak out (previously I was depending on codereview, but this is more robust). So
we can either add this to one of the headers that will remain n content/test, or
create a new one.

> 
> Thoughts?
> 
> On 2012/06/01 22:11:24, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> > File content/test/render_test_utils.h (right):
> > 
> >
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> > content/test/render_test_utils.h:15: #include
> > "content/renderer/renderer_webkitplatformsupport_impl.h"
> > we don't allow headers in content/test to "leak through" internal headers of
> > content. i.e. tests in src/chrome shouldn't need to include internal content
> > files, just like they shouldn't include webcore files

[Brian Ryner, 2012-06-02 18:58:09]

So given that change you made, the factory function for the
WebKitPlatformSupportImpl should go in a header under content/public/test, since
it's used under chrome/ ?

Also, I discovered that having this factory function return a
WebKit::WebKitPlatformSupportImpl is not sufficient; RenderThreadImpl needs to
call methods like SuspendSharedTimer() that are defined on
webkit_gue::WebKitPlatformSupportImpl.  Is that type from webkit/glue/
considered "public"?


On 2012/06/02 17:17:58, John Abd-El-Malek wrote:
> On 2012/06/01 23:06:07, Brian Ryner wrote:
> > Hm, so render_view_test.h was already pulling in
> > renderer_webkitplatformsupport_impl.h.  The tests that I changed under
chrome/
> > all make use of the RendererWebKitPlatformSupportImpl subclass that is being
> > defined here (which I'm just moving to a different header).  So, I'm not
sure
> > how to break this dependency -- perhaps a factory function that just returns
a
> > |WebKitPlatformSupportImpl*|?
> 
> yeah, a factory function sgtm. apologies for the confusing state, the content
> test situation is still being worked on and so there were stuff like this in
> intermediate state. I was waiting to finish content_browsertest before
> tightening all these deps rules, but that was perhaps not the right order.
> 
> > 
> > The render_thread_impl.h situation could be solved the same way, or perhaps
by
> > just separating that out into a new header under content/renderer/.
> 
> This change motivated me to move all the public headers from content/test to
> content/public/test, that way checkdeps can enforce that internal headers
don't
> leak out (previously I was depending on codereview, but this is more robust).
So
> we can either add this to one of the headers that will remain n content/test,
or
> create a new one.
> 
> > 
> > Thoughts?
> > 
> > On 2012/06/01 22:11:24, John Abd-El-Malek wrote:
> > >
> >
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> > > File content/test/render_test_utils.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> > > content/test/render_test_utils.h:15: #include
> > > "content/renderer/renderer_webkitplatformsupport_impl.h"
> > > we don't allow headers in content/test to "leak through" internal headers
of
> > > content. i.e. tests in src/chrome shouldn't need to include internal
content
> > > files, just like they shouldn't include webcore files

[jam, 2012-06-03 23:31:14]

btw here's a change that does this: http://codereview.chromium.org/10497013/

On 2012/06/02 18:58:09, Brian Ryner wrote:
> So given that change you made, the factory function for the
> WebKitPlatformSupportImpl should go in a header under content/public/test,
since
> it's used under chrome/ ?
> 
> Also, I discovered that having this factory function return a
> WebKit::WebKitPlatformSupportImpl is not sufficient; RenderThreadImpl needs to
> call methods like SuspendSharedTimer() that are defined on
> webkit_gue::WebKitPlatformSupportImpl.  Is that type from webkit/glue/
> considered "public"?
> 
> 
> On 2012/06/02 17:17:58, John Abd-El-Malek wrote:
> > On 2012/06/01 23:06:07, Brian Ryner wrote:
> > > Hm, so render_view_test.h was already pulling in
> > > renderer_webkitplatformsupport_impl.h.  The tests that I changed under
> chrome/
> > > all make use of the RendererWebKitPlatformSupportImpl subclass that is
being
> > > defined here (which I'm just moving to a different header).  So, I'm not
> sure
> > > how to break this dependency -- perhaps a factory function that just
returns
> a
> > > |WebKitPlatformSupportImpl*|?
> > 
> > yeah, a factory function sgtm. apologies for the confusing state, the
content
> > test situation is still being worked on and so there were stuff like this in
> > intermediate state. I was waiting to finish content_browsertest before
> > tightening all these deps rules, but that was perhaps not the right order.
> > 
> > > 
> > > The render_thread_impl.h situation could be solved the same way, or
perhaps
> by
> > > just separating that out into a new header under content/renderer/.
> > 
> > This change motivated me to move all the public headers from content/test to
> > content/public/test, that way checkdeps can enforce that internal headers
> don't
> > leak out (previously I was depending on codereview, but this is more
robust).
> So
> > we can either add this to one of the headers that will remain n
content/test,
> or
> > create a new one.
> > 
> > > 
> > > Thoughts?
> > > 
> > > On 2012/06/01 22:11:24, John Abd-El-Malek wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> > > > File content/test/render_test_utils.h (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/10449094/diff/9001/content/test/render_test_ut...
> > > > content/test/render_test_utils.h:15: #include
> > > > "content/renderer/renderer_webkitplatformsupport_impl.h"
> > > > we don't allow headers in content/test to "leak through" internal
headers
> of
> > > > content. i.e. tests in src/chrome shouldn't need to include internal
> content
> > > > files, just like they shouldn't include webcore files

[Brian Ryner, 2012-06-05 00:58:55]

Please take another look.

Thanks,

[jam, 2012-06-05 01:47:53]

this is a _lot_ of plumbing. what about if you had a static function like this:

bool RendererWebKitPlatformSupportImpl::SetSandboxEnabledForTesting(bool
enable);

?

[Brian Ryner, 2012-06-05 02:32:03]

Assuming everyone is good with this testing-specific method on
RendererWebKitPlatformSupportImpl, I think what you're suggesting would
certainly work, in terms of fixing RenderViewFakeResourcesTest and
WebRTCAudioDeviceTest.  The only issue is that we couldn't convert the callers
of RendererWebKitPlatformSupportImplNoSandbox under chrome/ to use this
mechanism if it's on RWPSI, since that's not a public content header.  I think
it would be nice to have just one way to disable the sandbox support since
there's less chance of something getting out of sync.

What do you think about exposing this on some global function under
content/public/test, then converting all of these callers to use it?

On 2012/06/05 01:47:53, John Abd-El-Malek wrote:
> this is a _lot_ of plumbing. what about if you had a static function like
this:
> 
> bool RendererWebKitPlatformSupportImpl::SetSandboxEnabledForTesting(bool
> enable);
> 
> ?

[jam, 2012-06-05 03:42:01]

On 2012/06/05 02:32:03, Brian Ryner wrote:
> Assuming everyone is good with this testing-specific method on
> RendererWebKitPlatformSupportImpl, I think what you're suggesting would
> certainly work, in terms of fixing RenderViewFakeResourcesTest and
> WebRTCAudioDeviceTest.  The only issue is that we couldn't convert the callers
> of RendererWebKitPlatformSupportImplNoSandbox under chrome/ to use this
> mechanism if it's on RWPSI, since that's not a public content header.  I think
> it would be nice to have just one way to disable the sandbox support since
> there's less chance of something getting out of sync.

It seems to me like a pretty minor thing that's not at risk of divergence. The
status quo is that the tests in chrome, and RenderViewFakeResourcesTest, are
already pretty different because of the latter's usage of RenderThreadImpl.

> 
> What do you think about exposing this on some global function under
> content/public/test, then converting all of these callers to use it?
> 
> On 2012/06/05 01:47:53, John Abd-El-Malek wrote:
> > this is a _lot_ of plumbing. what about if you had a static function like
> this:
> > 
> > bool RendererWebKitPlatformSupportImpl::SetSandboxEnabledForTesting(bool
> > enable);
> > 
> > ?

[Brian Ryner, 2012-06-05 05:42:40]

Ok, changed to disable sandbox support with a static method.  Please take
another look.

[jam, 2012-06-05 16:14:19]

this looks good, but I think you want to clean up after the test since the
unittest should leave the globals in the same state before it ran. so maybe
SetSandboxEnabledForTesting returns a bool of the old value and the current
places that call it reset it at the end of the test?

[Brian Ryner, 2012-06-05 17:56:48]

Done, and updated the issue description for the new approach.  Please take
another look.

Thanks,

On 2012/06/05 16:14:19, John Abd-El-Malek wrote:
> this looks good, but I think you want to clean up after the test since the
> unittest should leave the globals in the same state before it ran. so maybe
> SetSandboxEnabledForTesting returns a bool of the old value and the current
> places that call it reset it at the end of the test?

[jam, 2012-06-05 20:05:35]

lgtm

[commit-bot: I haz the power, 2012-06-05 20:15:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryner@chromium.org/10449094/38001

[commit-bot: I haz the power, 2012-06-05 22:01:05]

Change committed as 140631