PlzNavigate: Eliminate test_runner bailing out of layout tests too early

PlzNavigate: Eliminate test_runner bailing out of layout tests too early

//components/test_runner signals that a test is finished if it receives a
notificationDone signal from Javascript at a time when (a) it is waiting for
such a signal, (b) it does not think it is in the midst of a load, and (c) it
has no work in its queue. However, the definition of (b) is flawed for
browser-side navigation. The current definition for being in the midst of a
load is the time between WebFrameClient::didStartProvisionalLoad() being called
and WebFrameClient::didStopLoading() being called. However, for browser-side
navigations there is a period of time when navigation is ongoing but the
renderer has not yet received the didStartProvisionalLoad() callback. If
//components/test_runner gets a notificationDone signal during this period, it
can erroneously think that the test is finished.

This CL fixes the problem by having WebFrameTestProxy implement
WebFrameClient::didStartLoading(), teaching //components/test_runner to be
aware of the time period where a navigation is occurring, and using that as
input to decide whether a test is finished when it receives a notificationDone
signal.

Committed: https://crrev.com/987cb78dcfbb737f1042e2395ffba5a8ca1e590d
Cr-Commit-Position: refs/heads/master@{#412572}

[blundell, 2016-07-19 08:32:59]

blundell@chromium.org changed reviewers:
+ clamy@chromium.org, jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-07-19 12:29:02]

clamy@, deferring to you

[clamy, 2016-07-19 13:50:35]

Thanks! A few questions below.

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:1842: is_navigating_ = false;
Can it be possible for the new frame to be navigating at this point?

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
components/test_runner/test_runner.h:621: // enabled there is an arbitrary gap
between these two events.
Wouldn't that be the case in the current architecture as well, since we need the
browser to fetch the resource in both cases?

[blundell, 2016-07-21 10:02:18]

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
components/test_runner/test_runner.cc:1842: is_navigating_ = false;
On 2016/07/19 13:50:34, clamy wrote:
> Can it be possible for the new frame to be navigating at this point?

Yes. The flow is this:

- FrameLoader::startLoad() is invoked. This results in a browser-side navigation
and causes WebFrameClient::didStartLoading() to be invoked, so TestRunner's
|is_navigating_| is true.
- The browser-side navigation sends back an IPC that causes
RenderFrameImpl::OnCommitNavigation() to be invoked.
- That method calls into WebLocalFrame::load(), which causes
FrameLoader::startLoad() to be invoked again. This time it goes through to the
end instead of short-circuiting out early and causes
WebFrameClient::didStartProvisionalLoad() to be invoked, which in turn invokes
TestRunner::tryToSetTopLoadingFrame().

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2147633003/diff/20001/components/test_runner/...
components/test_runner/test_runner.h:621: // enabled there is an arbitrary gap
between these two events.
On 2016/07/19 13:50:35, clamy wrote:
> Wouldn't that be the case in the current architecture as well, since we need
the
> browser to fetch the resource in both cases?

Yes, good point. The PlzNavigate-specific gap is between navigation being
started and WebFrameClient::didStartProvisionalLoad(frame*) being called. It's
the latter callback that causes TestRunner's tryToSetTopLoadingFrame(frame*) to
be invoked, which TestRunner relies on for detecting that a load is active.

Updated the comment to be precise.

[blundell, 2016-07-21 10:08:36]

Maybe a cleaner fix is to change |is_navigating_| to be named |is_loading_|,
have TestRunner set that bool to true in tryToSetTopLoadingFrame() (or just
DCHECK it if it turns out that didStartLoading is always called before
didStartProvisionalLoad), and then have TestRunner check just that bool rather
than checking both the is_navigating_ bool and whether frame_ is null. WDYT?

[clamy, 2016-07-21 10:49:11]

That sounds better to me.

[blundell, 2016-07-21 15:03:02]

On 2016/07/21 10:49:11, clamy wrote:
> That sounds better to me.

Hi Camille,

On further investigation I think that the current patch is the best approach.
The reason is when WebFrameClient::didNavigateWithinPage() is called the load
isn't necessarily actually finished; the only thing that we know at that point
is that navigation is finished. Without the callback from
WebFrameClient::didNavigateWithinPage(), some tests time out with this patch
because they get a callback to WebFrameClient::didStartLoading() but never get a
matching callback to WebFrameClient::didStopLoading(). Happy to talk about this
more in person.

[clamy, 2016-07-27 16:57:05]

Thanks!

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
components/test_runner/test_runner.h:623: bool is_navigating_;
I find the name is_navigating_ a bit misleading, considering that it covers only
the moment between we say we are loading and the moment we get the start of the
provisional load. How about will_navigate_? Also the comment should highlight
that this is for PlzNavigate, where that gap can be quite big.

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
File components/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
components/test_runner/web_frame_test_client.cc:519:
test_runner_->OnNavigationEnd();
Based on what we discussed offline, do you still need to call OnNavigationEnd if
we navigate in-page?

[blundell, 2016-07-29 13:14:24]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 13:14:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 14:17:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 14:17:16]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-07-29 15:02:46]

Thanks!

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
components/test_runner/test_runner.h:623: bool is_navigating_;
On 2016/07/27 16:57:05, clamy (ooo until Aug 16) wrote:
> I find the name is_navigating_ a bit misleading, considering that it covers
only
> the moment between we say we are loading and the moment we get the start of
the
> provisional load. How about will_navigate_? Also the comment should highlight
> that this is for PlzNavigate, where that gap can be quite big.

Name changed. Re: commenting, is the second sentence of the existing comment not
sufficient for those purposes?

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
File components/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2147633003/diff/40001/components/test_runner/...
components/test_runner/web_frame_test_client.cc:519:
test_runner_->OnNavigationEnd();
On 2016/07/27 16:57:05, clamy (ooo until Aug 16) wrote:
> Based on what we discussed offline, do you still need to call OnNavigationEnd
if
> we navigate in-page?

Yes, it's needed: there is at least one test where didStartLoading() is called
but neither didStartProvisionalLoadForFrame() nor didStopLoading() is ever
called. This patch needs the callback from didNavigateWithPage() to avoid that
test hanging.

[blundell, 2016-08-16 07:29:43]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 07:29:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 08:37:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 08:37:01]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-08-17 13:39:48]

ping :)

[clamy, 2016-08-17 14:32:21]

Thanks! Lgtm.

[blundell, 2016-08-17 14:42:09]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2016-08-17 14:42:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 17:25:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-17 17:30:08]

Description was changed from

==========
PlzNavigate: Eliminate test_runner bailing out of layout tests too early

//components/test_runner signals that a test is finished if it receives a
notificationDone signal from Javascript at a time when (a) it is waiting for
such a signal, (b) it does not think it is in the midst of a load, and (c) it
has no work in its queue. However, the definition of (b) is flawed for
browser-side navigation. The current definition for being in the midst of a
load is the time between WebFrameClient::didStartProvisionalLoad() being called
and WebFrameClient::didStopLoading() being called. However, for browser-side
navigations there is a period of time when navigation is ongoing but the
renderer has not yet received the didStartProvisionalLoad() callback. If
//components/test_runner gets a notificationDone signal during this period, it
can erroneously think that the test is finished.

This CL fixes the problem by having WebFrameTestProxy implement
WebFrameClient::didStartLoading(), teaching //components/test_runner to be
aware of the time period where a navigation is occurring, and using that as
input to decide whether a test is finished when it receives a notificationDone
signal.
==========

to

==========
PlzNavigate: Eliminate test_runner bailing out of layout tests too early

//components/test_runner signals that a test is finished if it receives a
notificationDone signal from Javascript at a time when (a) it is waiting for
such a signal, (b) it does not think it is in the midst of a load, and (c) it
has no work in its queue. However, the definition of (b) is flawed for
browser-side navigation. The current definition for being in the midst of a
load is the time between WebFrameClient::didStartProvisionalLoad() being called
and WebFrameClient::didStopLoading() being called. However, for browser-side
navigations there is a period of time when navigation is ongoing but the
renderer has not yet received the didStartProvisionalLoad() callback. If
//components/test_runner gets a notificationDone signal during this period, it
can erroneously think that the test is finished.

This CL fixes the problem by having WebFrameTestProxy implement
WebFrameClient::didStartLoading(), teaching //components/test_runner to be
aware of the time period where a navigation is occurring, and using that as
input to decide whether a test is finished when it receives a notificationDone
signal.

Committed: https://crrev.com/987cb78dcfbb737f1042e2395ffba5a8ca1e590d
Cr-Commit-Position: refs/heads/master@{#412572}
==========

[commit-bot: I haz the power, 2016-08-17 17:30:09]

Patchset 6 (id:??) landed as
https://crrev.com/987cb78dcfbb737f1042e2395ffba5a8ca1e590d
Cr-Commit-Position: refs/heads/master@{#412572}