Deflake PrerenderExcessiveMemory, fix race

Deflake PrerenderExcessiveMemory, fixing a race in setting 
the visibility state of prerendered RenderViews in the 
process.

Also add some more visibility API tests that would have
reliably failed when losing the fixed race.


BUG=93081
TEST=PrerenderBrowserTest.PrerenderExcessiveMemory. PrerenderBrowserTest.PrerenderInfiniteLoop

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

[mmenke, 2011-08-25 21:12:37]

The NOTIFICATION_TAB_CONTENTS_CONNECTED notification has a race with loading the
page.  The flake we were seeing in the excessive memory test could either have
been due to losing the race, or to the renderer process taking 30MB before
performing the navigation.  Neither scenario strikes me as all that likely, but
I can't come up with any other cause.  This change should fix it, either way.

[dominich, 2011-08-25 21:18:04]

LGTM

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_visibility_hidden_quick.html (right):

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_visibility_hidden_quick.html:13: (lastState
== 'prerender' || lastState == 'hidden');
it might be more future-proof to check that lastState !== 'visible'

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_visibility_quick.html (right):

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_visibility_quick.html:6: TODO(mmenke): 
Currently, we get get extra events when a page is shown on
Is this due to the code that forcibly unhides the renderview twice on Windows?
Is that still there and needed after switching to Tabs?

[mmenke, 2011-08-25 21:36:53]

Thanks for the review.

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_visibility_hidden_quick.html (right):

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_visibility_hidden_quick.html:13: (lastState
== 'prerender' || lastState == 'hidden');
On 2011/08/25 21:18:04, dominich wrote:
> it might be more future-proof to check that lastState !== 'visible'

Done.  Still only allow at most one transition from non-hidden to hidden.

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_visibility_quick.html (right):

http://codereview.chromium.org/7693029/diff/9001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_visibility_quick.html:6: TODO(mmenke): 
Currently, we get get extra events when a page is shown on
On 2011/08/25 21:18:04, dominich wrote:
> Is this due to the code that forcibly unhides the renderview twice on Windows?
> Is that still there and needed after switching to Tabs?

That code no longer seems to exist, or at least I can't find it.  We still do
have the extra transition, however.  Looking into it is on my TODO list, but not
very high on it.

[Paweł Hajdan Jr., 2011-08-25 23:32:51]

Drive-by with a testing comment.

http://codereview.chromium.org/7693029/diff/7008/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/7693029/diff/7008/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:675: if
(tab_contents->IsLoading())
I think this may be prone to flakiness. Have you considered using a windowed
notification observer?

[mmenke, 2011-08-26 15:47:19]

http://codereview.chromium.org/7693029/diff/7008/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/7693029/diff/7008/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:675: if
(tab_contents->IsLoading())
On 2011/08/25 23:32:52, Paweł Hajdan Jr. wrote:
> I think this may be prone to flakiness. Have you considered using a windowed
> notification observer?

I don't think there was any flakiness in this particular instance, but agree
that in general, windowed notification observers can fix flakiness in a number
of cases.  Done.

[tburkard, 2011-08-26 17:54:47]

LGTM

On Thu, Aug 25, 2011 at 2:12 PM, <mmenke@chromium.org> wrote:

> Reviewers: dominich, tburkard,
>
> Message:
> The NOTIFICATION_TAB_CONTENTS_**CONNECTED notification has a race with
> loading the
> page.  The flake we were seeing in the excessive memory test could either
> have
> been due to losing the race, or to the renderer process taking 30MB before
> performing the navigation.  Neither scenario strikes me as all that likely,
> but
> I can't come up with any other cause.  This change should fix it, either
> way.
>
> Description:
> Deflake PrerenderExcessiveMemory, fixing a race in setting
> the visibility state of prerendered RenderViews in the
> process.
>
> BUG=93081
> TEST=PrerenderBrowserTest.**PrerenderExcessiveMemory.
> PrerenderBrowserTest.**PrerenderInfiniteLoop
>
> Please review this at
http://codereview.chromium.**org/7693029/<http://codereview.chromium.org/7693...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     chrome/browser/prerender/**prerender_browsertest.cc
>  M     chrome/browser/prerender/**prerender_contents.h
>  M     chrome/browser/prerender/**prerender_contents.cc
>  A     chrome/test/data/prerender/**prerender_visibility_hidden_**
> quick.html
>  A     chrome/test/data/prerender/**prerender_visibility_quick.**html
>
>
>

[Paweł Hajdan Jr., 2011-08-26 22:10:06]

Code I commented in the drive-by LGTM, thanks!

[commit-bot: I haz the power, 2011-08-27 02:40:29]

Change committed as 98539

[cbentzel, 2011-08-29 12:26:18]

LGTM

http://codereview.chromium.org/7693029/diff/4020/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/7693029/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:251: :
number_of_loads_(number_of_loads),
Nit: should this change to expected_number_of_loads_

http://codereview.chromium.org/7693029/diff/4020/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7693029/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:377: // informated promptly of
the size change.
Nit: informed

http://codereview.chromium.org/7693029/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:378:
prerender_contents_->view()->SizeContents(tab_bounds_.size());
Is the RenderWidgetHostView hidden at this point? It looks like this turns into
a no-op if done then, at least on Windows. Or does the Hide right below take
care of that?

[mmenke, 2011-08-29 12:30:35]

http://codereview.chromium.org/7693029/diff/4020/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7693029/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:378:
prerender_contents_->view()->SizeContents(tab_bounds_.size());
On 2011/08/29 12:26:18, cbentzel wrote:
> Is the RenderWidgetHostView hidden at this point? It looks like this turns
into
> a no-op if done then, at least on Windows. Or does the Hide right below take
> care of that?

RenderWidgetHostViews start out as visible.  This results in the TabContents
window being resized, which also results in resizing the RenderViewHost and
sending the size message (Synchronously, at least on Windows, OSX, and non-Views
Linux).

Then it's hidden in the next line.  Still need to figure out why one of the
tests was failing on each of linux_views and linux_chromeos after I landed this.