Fix flakiness and enable RenderViewHostManagerTest on Linux/Mac.

Fix flakiness and enable RenderViewHostManagerTest on Linux/Mac.

BUG=67532
TEST=browser_tests --gtest_filter=RenderViewHostManagerTest.*

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

[Charlie Reis, 2011-03-24 00:56:36]

The WaitForLoadStop call was flaky because it was possible for the navigation to
finish before we got there.  This CL fixes that.

This also removes the line in chrome_tests.gypi that made this test
Windows-only.  (Rafael confirmed that it was introduced as Windows-only in 2009,
but it doesn't need to be now.)

[Charlie Reis, 2011-03-24 16:29:14]

I've updated the patch to compare URL paths instead of titles, since the title
is updated asynchronously and can cause the test to fail.

[Paweł Hajdan Jr., 2011-03-24 17:22:57]

Work on flakiness is always awesome, thanks! Just a comment to make it even
better.

http://codereview.chromium.org/6730016/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

http://codereview.chromium.org/6730016/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:76:
ui_test_utils::WaitForLoadStop(
Is it possibly a more general problem with WaitForLoadStop? In other words, does
every client of it has to check is_loading to avoid intermittent problems?

I'm just wondering whether we can put the is_loading() check inside
WaitForLoadStop to make it easy to use and hard to misuse.

[Charlie Reis, 2011-03-24 17:33:55]

http://codereview.chromium.org/6730016/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

http://codereview.chromium.org/6730016/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:76:
ui_test_utils::WaitForLoadStop(
On 2011/03/24 17:22:57, Paweł Hajdan Jr. wrote:
> Is it possibly a more general problem with WaitForLoadStop? In other words,
does
> every client of it has to check is_loading to avoid intermittent problems?
> 
> I'm just wondering whether we can put the is_loading() check inside
> WaitForLoadStop to make it easy to use and hard to misuse.

Yes, that sounds like a good idea-- I'll give it a try.

The try bots are also being kind today and showing all the various forms of
flakiness here.  :)  The linux try bot just failed on the last line of this
test.  It looks like we need to wait for both the current and newly opened tabs
to finish their loads.  I'll make sure it clears the try servers before I send
you the updated patch.

Thanks!

[Charlie Reis, 2011-03-24 23:15:39]

http://codereview.chromium.org/6730016/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

http://codereview.chromium.org/6730016/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:76:
ui_test_utils::WaitForLoadStop(
On 2011/03/24 17:33:55, creis wrote:
> On 2011/03/24 17:22:57, Paweł Hajdan Jr. wrote:
> > Is it possibly a more general problem with WaitForLoadStop? In other words,
> does
> > every client of it has to check is_loading to avoid intermittent problems?
> > 
> > I'm just wondering whether we can put the is_loading() check inside
> > WaitForLoadStop to make it easy to use and hard to misuse.
> 
> Yes, that sounds like a good idea-- I'll give it a try.
> 
> The try bots are also being kind today and showing all the various forms of
> flakiness here.  :)  The linux try bot just failed on the last line of this
> test.  It looks like we need to wait for both the current and newly opened
tabs
> to finish their loads.  I'll make sure it clears the try servers before I send
> you the updated patch.
> 
> Thanks!

Fixed.

I had to also check whether the tab has a pending RenderViewHost, since it's
possible that we get here even before the DidStartLoading message is sent from
the renderer (in addition to getting here after the DidStopLoading message is
sent).  That's even more of an argument to put this check in WaitForLoadStop, so
that everyone else doesn't have to worry about it.

http://codereview.chromium.org/6730016/diff/6002/chrome/test/plugin/pdf_brows...
File chrome/test/plugin/pdf_browsertest.cc (left):

http://codereview.chromium.org/6730016/diff/6002/chrome/test/plugin/pdf_brows...
chrome/test/plugin/pdf_browsertest.cc:255:
IN_PROC_BROWSER_TEST_F(PDFBrowserTest, FLAKY_SLOW_Loading) {
I'm going to keep this marked flaky, since it already had a check before calling
WaitForLoadStop.  The flakiness is likely due to something else.

[Paweł Hajdan Jr., 2011-03-25 09:39:42]

LGTM with a comment, thanks!

http://codereview.chromium.org/6730016/diff/6002/chrome/test/ui_test_utils.h
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/6730016/diff/6002/chrome/test/ui_test_utils.h#...
chrome/test/ui_test_utils.h:113: // currently loading.
nit: Please either remove the "if the tab is currently loading" part, or add
"otherwise returns immediately".

[Charlie Reis, 2011-03-25 19:46:03]

http://codereview.chromium.org/6730016/diff/6002/chrome/test/ui_test_utils.h
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/6730016/diff/6002/chrome/test/ui_test_utils.h#...
chrome/test/ui_test_utils.h:113: // currently loading.
On 2011/03/25 09:39:42, Paweł Hajdan Jr. wrote:
> nit: Please either remove the "if the tab is currently loading" part, or add
> "otherwise returns immediately".

Done (and landed).