When simulating a navigation via TestRenderViewHost, first start a provisional load before committin

When simulating a navigation via TestRenderViewHost, first start a provisional load before committing it

BUG=none
TEST=none

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

[jochen (gone - plz use gerrit), 2012-07-18 14:33:46]

plz review

I had to update TestRenderViewHost, so it would first start a provisional load
before committing it

[jam, 2012-07-18 16:31:59]

content lgtm

[Charlie Reis, 2012-07-18 17:21:18]

http://codereview.chromium.org/10801011/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10801011/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:295: if
(render_view_host_ == NULL) {
nit: if (!render_view_host_) {

http://codereview.chromium.org/10801011/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:299:
DCHECK(!pending_render_view_host_);
Are you sure DidCommit or DidFail is always going to be called before DidStart,
when rapid back/forward cross-process navigations are in progress?  I'd love for
this to be true, but I'm skeptical because it relies on messages from the
renderers, allowing for races.

For example, I wouldn't be surprised to see this check fail if you tried the
following:
1) Navigate to several 5 or so different sites in the same tab via the omnibox,
causing process swaps.
2) Keep at least one of the renderer processes alive by opening another tab in
the same process.  (Might need a targeted link or window.open.)
3) Hold the Back keyboard shortcut down to race past entries, then the Forward
keyboard shortcut.  Or generally try to go back/forward fast in a random manner.

I've found this tends to expose lots of races we might not have expected.

[jochen (gone - plz use gerrit), 2012-07-18 18:03:40]

http://codereview.chromium.org/10801011/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10801011/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:299:
DCHECK(!pending_render_view_host_);
On 2012/07/18 17:21:18, creis wrote:
> Are you sure DidCommit or DidFail is always going to be called before
DidStart,
> when rapid back/forward cross-process navigations are in progress?  I'd love
for
> this to be true, but I'm skeptical because it relies on messages from the
> renderers, allowing for races.

No, I'm not sure.

However, wouldn't that mean that a web contents can have almost arbitrarily many
non-swapped out RVH attached?

[Charlie Reis, 2012-07-18 18:51:34]

http://codereview.chromium.org/10801011/diff/4002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10801011/diff/4002/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:299:
DCHECK(!pending_render_view_host_);
On 2012/07/18 18:03:40, jochen wrote:
> On 2012/07/18 17:21:18, creis wrote:
> > Are you sure DidCommit or DidFail is always going to be called before
> DidStart,
> > when rapid back/forward cross-process navigations are in progress?  I'd love
> for
> > this to be true, but I'm skeptical because it relies on messages from the
> > renderers, allowing for races.
> 
> No, I'm not sure.
> 
> However, wouldn't that mean that a web contents can have almost arbitrarily
many
> non-swapped out RVH attached?

Yes, I don't see why messages couldn't come in from any RVH in the history of
the tab.  Remember, messages from the renderer can be arbitrarily delayed based
on how heavily loaded the client is.  Is there anything that would prevent this?

[jochen (gone - plz use gerrit), 2012-07-19 08:20:25]

only landing the content/ bits

[commit-bot: I haz the power, 2012-07-19 08:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/10801011/6004

[commit-bot: I haz the power, 2012-07-19 09:37:30]

Change committed as 147417