Quit immediately in TestNavigationObserver.

Quit immediately in TestNavigationObserver.

TestNavigationObserver is currently the most often used test helper that
employs a MessageLoopRunner with deferred quit. Combined with the calls
to the two NavigateToURL* functions, which use it internally (one in
ui_test_utils in chrome/ and the other in content_browser_test_utils in
content/), there are around 3200 instantiations in the codebase. This CL
switches around 40% of them to quit immediately.

The remaining part is the calls to ui_test_utils::NavigateToURL. It is
left out because there are several problematic cases which I think are
better to investigate at once instead of patching with calls to
RunAllPendingInMessageLoop, and looks like this is going to be a
completely different story.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2635203002
Cr-Commit-Position: refs/heads/master@{#446045}
Committed: https://chromium.googlesource.com/chromium/src/+/c2f9b2a000040b13963a67b6eac0075ec11fa12c

[Alexander Semashko, 2017-01-17 12:54:23]

Description was changed from

==========
Quit immediately in TestNavigationObserver.

BUG=
==========

to

==========
Quit immediately in TestNavigationObserver.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Alexander Semashko, 2017-01-17 12:55:46]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[Alexander Semashko, 2017-01-17 12:55:53]

Description was changed from

==========
Quit immediately in TestNavigationObserver.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Quit immediately in TestNavigationObserver.

BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-17 12:55:57]

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, 2017-01-17 13:53:30]

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

[commit-bot: I haz the power, 2017-01-17 13:53:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Semashko, 2017-01-18 11:11:37]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 11:11:52]

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, 2017-01-18 12:30:51]

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

[commit-bot: I haz the power, 2017-01-18 12:30:52]

Dry run: This issue passed the CQ dry run.

[Alexander Semashko, 2017-01-18 13:06:44]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 13:07:01]

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, 2017-01-18 13:42:35]

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

[commit-bot: I haz the power, 2017-01-18 13:42:36]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alexander Semashko, 2017-01-19 16:00:46]

Description was changed from

==========
Quit immediately in TestNavigationObserver.

BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Quit immediately in TestNavigationObserver.

TestNavigationObserver is currently the most often used test helper that
employs a MessageLoopRunner with deferred quit. Combined with the calls
to the two NavigateToURL* functions, which use it internally (one in
ui_test_utils in chrome/ and the other in content_browser_test_utils in
content/), there are around 3200 instantiations in the codebase. This CL
switches around 40% of them to quit immediately.

The remaining part is the calls to ui_test_utils::NavigateToURL. It is
left out because there are several problematic cases which I think are
better to investigate at once instead of patching with calls to
RunAllPendingInMessageLoop, and looks like this is going to be a
completely different story.


R=jialiul@chromium.org, nasko@chromium.org
BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Alexander Semashko, 2017-01-19 16:01:31]

Description was changed from

==========
Quit immediately in TestNavigationObserver.

TestNavigationObserver is currently the most often used test helper that
employs a MessageLoopRunner with deferred quit. Combined with the calls
to the two NavigateToURL* functions, which use it internally (one in
ui_test_utils in chrome/ and the other in content_browser_test_utils in
content/), there are around 3200 instantiations in the codebase. This CL
switches around 40% of them to quit immediately.

The remaining part is the calls to ui_test_utils::NavigateToURL. It is
left out because there are several problematic cases which I think are
better to investigate at once instead of patching with calls to
RunAllPendingInMessageLoop, and looks like this is going to be a
completely different story.


R=jialiul@chromium.org, nasko@chromium.org
BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Quit immediately in TestNavigationObserver.

TestNavigationObserver is currently the most often used test helper that
employs a MessageLoopRunner with deferred quit. Combined with the calls
to the two NavigateToURL* functions, which use it internally (one in
ui_test_utils in chrome/ and the other in content_browser_test_utils in
content/), there are around 3200 instantiations in the codebase. This CL
switches around 40% of them to quit immediately.

The remaining part is the calls to ui_test_utils::NavigateToURL. It is
left out because there are several problematic cases which I think are
better to investigate at once instead of patching with calls to
RunAllPendingInMessageLoop, and looks like this is going to be a
completely different story.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Alexander Semashko, 2017-01-19 16:01:31]

ahest@yandex-team.ru changed reviewers:
+ jialiul@chromium.org, nasko@chromium.org

[Alexander Semashko, 2017-01-19 16:03:30]

Nasko, please look at changes in content/, or tell me if you're tired of
reviewing all this stuff about MessageLoopRunners :)
I keep adding you because you already know the context, but I don't know, maybe
it'd be better if other content/ onwers took part in this more.
And thank you for reviews!

Jialiu, WDYT about the change in SBNavigationObserverBrowserTest? Turns out that
these tests depend strongly on that some tasks are processed on UI thread after
the load stops. Do you know what exactly these task are? It is important because
if these tasks originate from other thread/process, then this must be fixed in
an other way.

[Jialiu Lin, 2017-01-19 19:41:24]

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

[commit-bot: I haz the power, 2017-01-19 19:42:00]

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

[Jialiu Lin, 2017-01-19 19:51:17]

On 2017/01/19 16:03:30, Alexander Semashko wrote:
> Nasko, please look at changes in content/, or tell me if you're tired of
> reviewing all this stuff about MessageLoopRunners :)
> I keep adding you because you already know the context, but I don't know,
maybe
> it'd be better if other content/ onwers took part in this more.
> And thank you for reviews!
> 
> Jialiu, WDYT about the change in SBNavigationObserverBrowserTest? Turns out
that
> these tests depend strongly on that some tasks are processed on UI thread
after
> the load stops. Do you know what exactly these task are? It is important
because
> if these tasks originate from other thread/process, then this must be fixed in
> an other way.

Hi Alexander,
My tests basically checks if SB navigation observer can correctly record
navigation
details in case of various redirect/retargeting scenarios. I believe the code
after
the load stops are mainly for verify results. I think your change looks fine as
long
as all the try bots are happy.

[commit-bot: I haz the power, 2017-01-19 21:16:22]

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

[commit-bot: I haz the power, 2017-01-19 21:16:24]

Dry run: This issue passed the CQ dry run.

[Alexander Semashko, 2017-01-19 22:57:47]

On 2017/01/19 19:51:17, Jialiu Lin wrote:
> On 2017/01/19 16:03:30, Alexander Semashko wrote:
> > Nasko, please look at changes in content/, or tell me if you're tired of
> > reviewing all this stuff about MessageLoopRunners :)
> > I keep adding you because you already know the context, but I don't know,
> maybe
> > it'd be better if other content/ onwers took part in this more.
> > And thank you for reviews!
> > 
> > Jialiu, WDYT about the change in SBNavigationObserverBrowserTest? Turns out
> that
> > these tests depend strongly on that some tasks are processed on UI thread
> after
> > the load stops. Do you know what exactly these task are? It is important
> because
> > if these tasks originate from other thread/process, then this must be fixed
in
> > an other way.
> 
> Hi Alexander,
> My tests basically checks if SB navigation observer can correctly record
> navigation
> details in case of various redirect/retargeting scenarios. I believe the code
> after
> the load stops are mainly for verify results. I think your change looks fine
as
> long
> as all the try bots are happy.

I think I was not clear enough, let me try to reiterate.
Currently the TestNavigationObserver in these tests executes
several UI thread tasks after load stops, because otherwise these
checks often fail. This means that the tests depend on something
happening after load stops, and the question is: what exactly do
they depend on?

If it is, for example, a subsequent message from
the renderer, then it is a hidden source of flakiness. Even if
all tests pass now, it might show up if somebody later adds some
work in the renderer that delays that message - I'd like to
ensure that it is not the case, i.e. the tests are correctly
waiting for all the necessary events.

[Jialiu Lin, 2017-01-19 23:23:37]

On 2017/01/19 22:57:47, Alexander Semashko wrote:
> On 2017/01/19 19:51:17, Jialiu Lin wrote:
> > On 2017/01/19 16:03:30, Alexander Semashko wrote:
> > > Nasko, please look at changes in content/, or tell me if you're tired of
> > > reviewing all this stuff about MessageLoopRunners :)
> > > I keep adding you because you already know the context, but I don't know,
> > maybe
> > > it'd be better if other content/ onwers took part in this more.
> > > And thank you for reviews!
> > > 
> > > Jialiu, WDYT about the change in SBNavigationObserverBrowserTest? Turns
out
> > that
> > > these tests depend strongly on that some tasks are processed on UI thread
> > after
> > > the load stops. Do you know what exactly these task are? It is important
> > because
> > > if these tasks originate from other thread/process, then this must be
fixed
> in
> > > an other way.
> > 
> > Hi Alexander,
> > My tests basically checks if SB navigation observer can correctly record
> > navigation
> > details in case of various redirect/retargeting scenarios. I believe the
code
> > after
> > the load stops are mainly for verify results. I think your change looks fine
> as
> > long
> > as all the try bots are happy.
> 
> I think I was not clear enough, let me try to reiterate.
> Currently the TestNavigationObserver in these tests executes
> several UI thread tasks after load stops, because otherwise these
> checks often fail. This means that the tests depend on something
> happening after load stops, and the question is: what exactly do
> they depend on?
> 
> If it is, for example, a subsequent message from
> the renderer, then it is a hidden source of flakiness. Even if
> all tests pass now, it might show up if somebody later adds some
> work in the renderer that delays that message - I'd like to
> ensure that it is not the case, i.e. the tests are correctly
> waiting for all the necessary events.

I see. I guess I'll let nasko answer this question. I'm not very familiar
with the detailed logic inside TestNavigationObserver.

[nasko, 2017-01-20 22:54:45]

I'm not tired of these, so definitely keep them coming :). Just a couple of
comments.

https://codereview.chromium.org/2635203002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2635203002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:295:
base::WeakPtrFactory<TestNavigationThrottleInstaller> weak_factory_;
Can you add a comment as to why this weak factory is needed. It isn't
immediately obvious to someone who hasn't investigated the code in-depth.

https://codereview.chromium.org/2635203002/diff/80001/content/public/test/tes...
File content/public/test/test_navigation_observer.h (right):

https://codereview.chromium.org/2635203002/diff/80001/content/public/test/tes...
content/public/test/test_navigation_observer.h:34: explicit
TestNavigationObserver(WebContents* web_contents,
nit: Do we need for explicit anymore, as there are more than one parameters.

[Alexander Semashko, 2017-01-23 19:45:39]

Re: Jialiu
My question was not about TestNavigationObserver, it was about conditions
specific for the exact test. I checked and it seems that these tests just need
several DownloadManagerImpl tasks to be processed on the UI thread, which means
that it is acceptable to just call RunAllPending.

https://codereview.chromium.org/2635203002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):

https://codereview.chromium.org/2635203002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:295:
base::WeakPtrFactory<TestNavigationThrottleInstaller> weak_factory_;
On 2017/01/20 22:54:45, nasko wrote:
> Can you add a comment as to why this weak factory is needed. It isn't
> immediately obvious to someone who hasn't investigated the code in-depth.

Done.

https://codereview.chromium.org/2635203002/diff/80001/content/public/test/tes...
File content/public/test/test_navigation_observer.h (right):

https://codereview.chromium.org/2635203002/diff/80001/content/public/test/tes...
content/public/test/test_navigation_observer.h:34: explicit
TestNavigationObserver(WebContents* web_contents,
On 2017/01/20 22:54:45, nasko wrote:
> nit: Do we need for explicit anymore, as there are more than one parameters.

Constructors should be explicit as long as it is possible to call them with a
single argument. In this case this is possible thanks to the second argument
having a default value. A note w.r.t. default arguments: they are allowed now in
non-virtual functions, and (I hope) this one is temporary and will be removed
when we sort out issues around NavigateToURL in browser_tests.

[Jialiu Lin, 2017-01-23 19:59:36]

On 2017/01/23 19:45:39, Alexander Semashko wrote:
> Re: Jialiu
> My question was not about TestNavigationObserver, it was about conditions
> specific for the exact test. I checked and it seems that these tests just need
> several DownloadManagerImpl tasks to be processed on the UI thread, which
means
> that it is acceptable to just call RunAllPending.
Yes, that's about right. My tests just wait for all the navigations to be done
and a resulting download to be started. If you think there are better way to do
it, please feel free to make changes, or let me know how should I improve them.
Thank you! (As I said, I'm not expert in navigation. I was mimicking other's
code
when I wrote these tests.). 

> 
>
https://codereview.chromium.org/2635203002/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2635203002/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl_browsertest.cc:295:
> base::WeakPtrFactory<TestNavigationThrottleInstaller> weak_factory_;
> On 2017/01/20 22:54:45, nasko wrote:
> > Can you add a comment as to why this weak factory is needed. It isn't
> > immediately obvious to someone who hasn't investigated the code in-depth.
> 
> Done.
> 
>
https://codereview.chromium.org/2635203002/diff/80001/content/public/test/tes...
> File content/public/test/test_navigation_observer.h (right):
> 
>
https://codereview.chromium.org/2635203002/diff/80001/content/public/test/tes...
> content/public/test/test_navigation_observer.h:34: explicit
> TestNavigationObserver(WebContents* web_contents,
> On 2017/01/20 22:54:45, nasko wrote:
> > nit: Do we need for explicit anymore, as there are more than one parameters.
> 
> Constructors should be explicit as long as it is possible to call them with a
> single argument. In this case this is possible thanks to the second argument
> having a default value. A note w.r.t. default arguments: they are allowed now
in
> non-virtual functions, and (I hope) this one is temporary and will be removed
> when we sort out issues around NavigateToURL in browser_tests.

[nasko, 2017-01-23 22:35:14]

content/ LGTM

[Jialiu Lin, 2017-01-23 22:47:06]

lgtm

[Alexander Semashko, 2017-01-24 00:34:03]

ahest@yandex-team.ru changed reviewers:
+ phajdan.jr@chromium.org

[Alexander Semashko, 2017-01-24 00:34:04]

phajdan.jr@chromium.org: Can you please look at this and stamp for  changes in
chrome/test/base?

[Paweł Hajdan Jr., 2017-01-24 11:07:34]

LGTM

[Alexander Semashko, 2017-01-24 11:08:40]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2017-01-24 11:08:55]

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, 2017-01-24 11:41:08]

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

[commit-bot: I haz the power, 2017-01-24 11:41:09]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Alexander Semashko, 2017-01-25 12:14:35]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 12:14:47]

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, 2017-01-25 13:07:18]

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

[commit-bot: I haz the power, 2017-01-25 13:07:19]

Dry run: This issue passed the CQ dry run.

[Alexander Semashko, 2017-01-25 13:08:18]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2017-01-25 13:08:37]

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, 2017-01-25 17:13:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485349698335070,
"parent_rev": "6fe839799893f3d82b5ab6ccfe0e759f8044c091", "commit_rev":
"c2f9b2a000040b13963a67b6eac0075ec11fa12c"}

[commit-bot: I haz the power, 2017-01-25 17:14:27]

Description was changed from

==========
Quit immediately in TestNavigationObserver.

TestNavigationObserver is currently the most often used test helper that
employs a MessageLoopRunner with deferred quit. Combined with the calls
to the two NavigateToURL* functions, which use it internally (one in
ui_test_utils in chrome/ and the other in content_browser_test_utils in
content/), there are around 3200 instantiations in the codebase. This CL
switches around 40% of them to quit immediately.

The remaining part is the calls to ui_test_utils::NavigateToURL. It is
left out because there are several problematic cases which I think are
better to investigate at once instead of patching with calls to
RunAllPendingInMessageLoop, and looks like this is going to be a
completely different story.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Quit immediately in TestNavigationObserver.

TestNavigationObserver is currently the most often used test helper that
employs a MessageLoopRunner with deferred quit. Combined with the calls
to the two NavigateToURL* functions, which use it internally (one in
ui_test_utils in chrome/ and the other in content_browser_test_utils in
content/), there are around 3200 instantiations in the codebase. This CL
switches around 40% of them to quit immediately.

The remaining part is the calls to ui_test_utils::NavigateToURL. It is
left out because there are several problematic cases which I think are
better to investigate at once instead of patching with calls to
RunAllPendingInMessageLoop, and looks like this is going to be a
completely different story.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2635203002
Cr-Commit-Position: refs/heads/master@{#446045}
Committed:
https://chromium.googlesource.com/chromium/src/+/c2f9b2a000040b13963a67b6eac0...
==========

[commit-bot: I haz the power, 2017-01-25 17:14:28]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c2f9b2a000040b13963a67b6eac0...