[chromedriver] Apply page load timeout to slow cross-process navigations

[chromedriver] Apply page load timeout to slow cross-process navigations

This is a second attempt to fight chromedriver hangs on slow-to-commit
cross-process navigations. In the discussion of the first, incorrect
solution (see https://codereview.chromium.org/1520883002/), it became
clear that it'd be better to handle this in chromedriver. Here I
introduce a timeout tracking mechanism which is easy to use in the
chromedriver's architecture. In general, it allows to abort waiting for
a devtools command response, do some cleanup and return an error to the
client.

Currently this timeout tracking is used only when executing the 'get'
command, history navigations and reloads are left as is. I wanted to get
feedback about the approach as a whole first. Please take a look.

BUG=chromedriver:1292
Committed: https://crrev.com/091637f2223b51356cfeb3f89bb03394ed1f2b6a
Cr-Commit-Position: refs/heads/master@{#387658}

[Alexander Semashko, 2016-02-03 15:49:22]

ahest@yandex-team.ru changed reviewers:
+ samuong@chromium.org, stgao@chromium.org

[Alexander Semashko, 2016-02-10 17:05:23]

On 2016/02/03 15:49:22, Alexander Semashko wrote:

PTAL

[samuong, 2016-02-12 06:04:22]

samuong@chromium.org changed reviewers:
- stgao@chromium.org

[samuong, 2016-02-12 06:04:22]

Thanks for the patch! I'm still looking at this, will get back to you about it
tomorrow.

[samuong, 2016-02-15 06:37:38]

On 2016/02/12 06:04:22, samuong wrote:
> Thanks for the patch! I'm still looking at this, will get back to you about it
> tomorrow.

I think the general approach looks good. Even if we could fix this on the
DevTools side, we should handle errors better in ChromeDriver. Will send some
more detailed comments on Tuesday, thanks for looking into this!

[samuong, 2016-02-19 20:18:58]

Overall I think this looks quite good. Just a few comments and questions below.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/DEPS (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/DEPS:10: "+chrome/test/chromedriver/util.h",
how about we move the Timeout class into
chrome/test/chromedriver/net/timeout.{cc,h}?

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/devtools_client.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/devtools_client.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
it's 2016 :)

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
File chrome/test/chromedriver/util.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
chrome/test/chromedriver/util.cc:47: DCHECK(deadline_.is_null());
ChromeDriver is a bit different from Chrome in that 1) we don't get crash
reports from users and 2) few (if any) people use debug builds, so these checks
will never get run. This makes DCHECK() less useful here, and anyway this will
only get called once per instance, so I think we should change this to an if
condition, and log the error.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:282:
timeout->SetDuration(session->page_load_timeout);
Is there a need to have a series of subtask timeouts if the "outer" timeout has
the same duration?

[Alexander Semashko, 2016-02-19 23:38:59]

Thanks, Sam! As I mentioned, this patch does not cover back/forward navigations
and reloads, but it's natural to apply page load timeout to them as well. Do you
want me to add these bits to this CL, or is it better to proceed with this
if/after the patch lands and proves to be okay?

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/DEPS (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/DEPS:10: "+chrome/test/chromedriver/util.h",
On 2016/02/19 20:18:57, samuong wrote:
> how about we move the Timeout class into
> chrome/test/chromedriver/net/timeout.{cc,h}?

Initially I thought about it, but it does not seem a truly right way to go since
this Timeout is not really a /net level concept. If there was chromedriver/base,
Timeout would fit there well, but I'm not sure it's worth creating a /base dir
for it. I'd accept whatever you decide, just wanted to clarify the issue.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/devtools_client.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/devtools_client.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2016/02/19 20:18:57, samuong wrote:
> it's 2016 :)

The patch was baking for a long time :) Ok, I'll fix it.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
File chrome/test/chromedriver/util.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
chrome/test/chromedriver/util.cc:47: DCHECK(deadline_.is_null());
On 2016/02/19 20:18:57, samuong wrote:
> ChromeDriver is a bit different from Chrome in that 1) we don't get crash
> reports from users and 2) few (if any) people use debug builds, so these
checks
> will never get run. This makes DCHECK() less useful here, and anyway this will
> only get called once per instance, so I think we should change this to an if
> condition, and log the error.

Yeah, I put DCHECKs here hoping they'll be hit in tests if something violates
the conditions. But I don't know if Chromedriver integration tests are run with
debug builds or DCHECK_ALWAYS_ON. Do they? Probably it would be best to keep
DCHECKs and add logs?

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:282:
timeout->SetDuration(session->page_load_timeout);
On 2016/02/19 20:18:58, samuong wrote:
> Is there a need to have a series of subtask timeouts if the "outer" timeout
has
> the same duration?

Why not? Anyway, I could'n conceive a better (yet simple) way for
ExecuteWindowCommand to set a timeout specific for a particular command, if
there is one. And it's easy to imagine having a long timeout for e.g. executing
scripts and a small timeout for page loads (for example, testing a web service
in local network with heavy server-side processing), in such a case a hung
navigation is a reason to abort as soon as possible.

[samuong, 2016-02-20 00:11:45]

I have a slight preference to break this up over two CLs, but I'm happy to
review the one big one if that makes it easier for you.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/DEPS (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/DEPS:10: "+chrome/test/chromedriver/util.h",
On 2016/02/19 23:38:58, Alexander Semashko wrote:
> On 2016/02/19 20:18:57, samuong wrote:
> > how about we move the Timeout class into
> > chrome/test/chromedriver/net/timeout.{cc,h}?
> 
> Initially I thought about it, but it does not seem a truly right way to go
since
> this Timeout is not really a /net level concept. If there was
chromedriver/base,
> Timeout would fit there well, but I'm not sure it's worth creating a /base dir
> for it. I'd accept whatever you decide, just wanted to clarify the issue. 

I agree that it'd be better in a base directory, but also agree that it's not
really yet worth creating a new base directory just for this.  Right now we're
only using it in the context of DevTools/network requests, but if that changes
we could move it out of net/ into some other higher-level directory. I just
don't think we should be putting too many different things into this util file.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
File chrome/test/chromedriver/util.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
chrome/test/chromedriver/util.cc:47: DCHECK(deadline_.is_null());
On 2016/02/19 23:38:58, Alexander Semashko wrote:
> On 2016/02/19 20:18:57, samuong wrote:
> > ChromeDriver is a bit different from Chrome in that 1) we don't get crash
> > reports from users and 2) few (if any) people use debug builds, so these
> checks
> > will never get run. This makes DCHECK() less useful here, and anyway this
will
> > only get called once per instance, so I think we should change this to an if
> > condition, and log the error.
> 
> Yeah, I put DCHECKs here hoping they'll be hit in tests if something violates
> the conditions. But I don't know if Chromedriver integration tests are run
with
> debug builds or DCHECK_ALWAYS_ON. Do they? Probably it would be best to keep
> DCHECKs and add logs?

For the C++ unit tests, they'll get built differently depending on which bot is
doing the building. Some will get built in Debug mode, with DCHECKs enabled. But
ChromeDriver gets built with target=Release in the end-to-end tests, here's an
example:

https://build.chromium.org/p/chromium.chromedriver/builders/Linux/builds/1685...

In these tests, if something goes wrong, it'll crash chromedriver and I suspect
it'd be pretty hard to figure out what happened, so adding logs would definitely
help there.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:206: Timeout timeout;
I might have misread how this works, but does timeout.deadline_ == 0 here?

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:238:
Timeout(session->page_load_timeout, &timeout),
...and if so, does that mean that the timeout duration here will also be 0,
since min(session->page_load_timeout, timeout.duration_) == timeout.duration_ ==
0?

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:282:
timeout->SetDuration(session->page_load_timeout);
On 2016/02/19 23:38:59, Alexander Semashko wrote:
> On 2016/02/19 20:18:58, samuong wrote:
> > Is there a need to have a series of subtask timeouts if the "outer" timeout
> has
> > the same duration?
> 
> Why not? Anyway, I could'n conceive a better (yet simple) way for
> ExecuteWindowCommand to set a timeout specific for a particular command, if
> there is one. And it's easy to imagine having a long timeout for e.g.
executing
> scripts and a small timeout for page loads (for example, testing a web service
> in local network with heavy server-side processing), in such a case a hung
> navigation is a reason to abort as soon as possible.

If the outer and inner timeouts have the same duration, but the outer one has an
earlier start time, doesn't that make the inner sub-task timeout redundant?

[Alexander Semashko, 2016-02-20 00:38:40]

I'm fine with not adding more behavior changes to this CL, lets address other
commands afterwards.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/DEPS (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/DEPS:10: "+chrome/test/chromedriver/util.h",
On 2016/02/20 00:11:44, samuong wrote:
> On 2016/02/19 23:38:58, Alexander Semashko wrote:
> > On 2016/02/19 20:18:57, samuong wrote:
> > > how about we move the Timeout class into
> > > chrome/test/chromedriver/net/timeout.{cc,h}?
> > 
> > Initially I thought about it, but it does not seem a truly right way to go
> since
> > this Timeout is not really a /net level concept. If there was
> chromedriver/base,
> > Timeout would fit there well, but I'm not sure it's worth creating a /base
dir
> > for it. I'd accept whatever you decide, just wanted to clarify the issue. 
> 
> I agree that it'd be better in a base directory, but also agree that it's not
> really yet worth creating a new base directory just for this.  Right now we're
> only using it in the context of DevTools/network requests, but if that changes
> we could move it out of net/ into some other higher-level directory. I just
> don't think we should be putting too many different things into this util
file.

Ok, let's move it to /net.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:238:
Timeout(session->page_load_timeout, &timeout),
On 2016/02/20 00:11:44, samuong wrote:
> ...and if so, does that mean that the timeout duration here will also be 0,
> since min(session->page_load_timeout, timeout.duration_) == timeout.duration_
==
> 0?

No, the outer timout's duration has effect only if it is set. The first call to
WaitForPendingNavigations will always be allowed to wait for
session->page_load_timeout.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:282:
timeout->SetDuration(session->page_load_timeout);
On 2016/02/20 00:11:44, samuong wrote:
> On 2016/02/19 23:38:59, Alexander Semashko wrote:
> > On 2016/02/19 20:18:58, samuong wrote:
> > > Is there a need to have a series of subtask timeouts if the "outer"
timeout
> > has
> > > the same duration?
> > 
> > Why not? Anyway, I could'n conceive a better (yet simple) way for
> > ExecuteWindowCommand to set a timeout specific for a particular command, if
> > there is one. And it's easy to imagine having a long timeout for e.g.
> executing
> > scripts and a small timeout for page loads (for example, testing a web
service
> > in local network with heavy server-side processing), in such a case a hung
> > navigation is a reason to abort as soon as possible.
> 
> If the outer and inner timeouts have the same duration, but the outer one has
an
> earlier start time, doesn't that make the inner sub-task timeout redundant?

Yeah, it is, only if they have the same duration. But it's different if the
outer one has a long duration. Right now the outer timeout is set only in
ExecuteGet and happen to be the same that ExecuteWindowCommand uses for
preparation. After a while this may change.

[samuong, 2016-02-22 23:55:35]

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:238:
Timeout(session->page_load_timeout, &timeout),
On 2016/02/20 00:38:40, Alexander Semashko wrote:
> On 2016/02/20 00:11:44, samuong wrote:
> > ...and if so, does that mean that the timeout duration here will also be 0,
> > since min(session->page_load_timeout, timeout.duration_) ==
timeout.duration_
> ==
> > 0?
> 
> No, the outer timout's duration has effect only if it is set. The first call
to
> WaitForPendingNavigations will always be allowed to wait for
> session->page_load_timeout.

Ah right, I misread the way TimeTicks::is_null() works. So the first call to
WaitForPendingNavigations will use session->page_load_timeout, since
timeout->deadline_ is null. This shouldn't cause any problem.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:243: status = command.Run(session,
web_view, params, value, &timeout);
...however the call to command.Run here may or may not set the timeout duration,
depending on which command we're dealing with. In the case of ExecuteGet, it
will set the duration, and if ExecuteGet fails, it'll be retried. So on the
second attempt, we call ExecuteGet again, and this will in turn call
timeout.SetDuration for a second time?

I guess this won't cause an issue since it gets set to the same duration (unless
we're in a debug build and the DCHECKs are enabled...)

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:282:
timeout->SetDuration(session->page_load_timeout);
On 2016/02/20 00:38:40, Alexander Semashko wrote:
> On 2016/02/20 00:11:44, samuong wrote:
> > On 2016/02/19 23:38:59, Alexander Semashko wrote:
> > > On 2016/02/19 20:18:58, samuong wrote:
> > > > Is there a need to have a series of subtask timeouts if the "outer"
> timeout
> > > has
> > > > the same duration?
> > > 
> > > Why not? Anyway, I could'n conceive a better (yet simple) way for
> > > ExecuteWindowCommand to set a timeout specific for a particular command,
if
> > > there is one. And it's easy to imagine having a long timeout for e.g.
> > executing
> > > scripts and a small timeout for page loads (for example, testing a web
> service
> > > in local network with heavy server-side processing), in such a case a hung
> > > navigation is a reason to abort as soon as possible.
> > 
> > If the outer and inner timeouts have the same duration, but the outer one
has
> an
> > earlier start time, doesn't that make the inner sub-task timeout redundant?
> 
> Yeah, it is, only if they have the same duration. But it's different if the
> outer one has a long duration. Right now the outer timeout is set only in
> ExecuteGet and happen to be the same that ExecuteWindowCommand uses for
> preparation. After a while this may change.

OK, it's fine if this is the same for now but you want to make it easy to make
them different later.

[Alexander Semashko, 2016-02-24 12:51:20]

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/DEPS (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/DEPS:10: "+chrome/test/chromedriver/util.h",
On 2016/02/20 00:38:40, Alexander Semashko wrote:
> On 2016/02/20 00:11:44, samuong wrote:
> > On 2016/02/19 23:38:58, Alexander Semashko wrote:
> > > On 2016/02/19 20:18:57, samuong wrote:
> > > > how about we move the Timeout class into
> > > > chrome/test/chromedriver/net/timeout.{cc,h}?
> > > 
> > > Initially I thought about it, but it does not seem a truly right way to go
> > since
> > > this Timeout is not really a /net level concept. If there was
> > chromedriver/base,
> > > Timeout would fit there well, but I'm not sure it's worth creating a /base
> dir
> > > for it. I'd accept whatever you decide, just wanted to clarify the issue. 
> > 
> > I agree that it'd be better in a base directory, but also agree that it's
not
> > really yet worth creating a new base directory just for this.  Right now
we're
> > only using it in the context of DevTools/network requests, but if that
changes
> > we could move it out of net/ into some other higher-level directory. I just
> > don't think we should be putting too many different things into this util
> file.
> 
> Ok, let's move it to /net.

Done.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/devtools_client.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/devtools_client.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2016/02/19 20:18:57, samuong wrote:
> it's 2016 :)

Done.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
File chrome/test/chromedriver/util.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/ut...
chrome/test/chromedriver/util.cc:47: DCHECK(deadline_.is_null());
On 2016/02/20 00:11:44, samuong wrote:
> On 2016/02/19 23:38:58, Alexander Semashko wrote:
> > On 2016/02/19 20:18:57, samuong wrote:
> > > ChromeDriver is a bit different from Chrome in that 1) we don't get crash
> > > reports from users and 2) few (if any) people use debug builds, so these
> > checks
> > > will never get run. This makes DCHECK() less useful here, and anyway this
> will
> > > only get called once per instance, so I think we should change this to an
if
> > > condition, and log the error.
> > 
> > Yeah, I put DCHECKs here hoping they'll be hit in tests if something
violates
> > the conditions. But I don't know if Chromedriver integration tests are run
> with
> > debug builds or DCHECK_ALWAYS_ON. Do they? Probably it would be best to keep
> > DCHECKs and add logs?
> 
> For the C++ unit tests, they'll get built differently depending on which bot
is
> doing the building. Some will get built in Debug mode, with DCHECKs enabled.
But
> ChromeDriver gets built with target=Release in the end-to-end tests, here's an
> example:
> 
>
https://build.chromium.org/p/chromium.chromedriver/builders/Linux/builds/1685...
> 
> In these tests, if something goes wrong, it'll crash chromedriver and I
suspect
> it'd be pretty hard to figure out what happened, so adding logs would
definitely
> help there.

Done.

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1669453002/diff/1/chrome/test/chromedriver/wi...
chrome/test/chromedriver/window_commands.cc:243: status = command.Run(session,
web_view, params, value, &timeout);
On 2016/02/22 23:55:35, samuong wrote:
> ...however the call to command.Run here may or may not set the timeout
duration,
> depending on which command we're dealing with. In the case of ExecuteGet, it
> will set the duration, and if ExecuteGet fails, it'll be retried. So on the
> second attempt, we call ExecuteGet again, and this will in turn call
> timeout.SetDuration for a second time?
> 
> I guess this won't cause an issue since it gets set to the same duration
(unless
> we're in a debug build and the DCHECKs are enabled...)

Good catch! I've changed the code so that it's ok to call SetDuration many times
with the same duration.

[Alexander Semashko, 2016-03-01 19:37:57]

PTAL

[Alexander Semashko, 2016-03-14 15:12:01]

On 2016/03/01 19:37:57, Alexander Semashko wrote:
> PTAL

Sam?

[Alexander Semashko, 2016-04-08 13:25:20]

please take a look

[Alexander Semashko, 2016-04-12 13:42:49]

Description was changed from

==========
[chromedriver] Apply page load timeout to slow cross-process navigations

This is a second attempt to fight chromedriver hangs on slow-to-commit
cross-process navigations. In the discussion of the first, incorrect
solution (see https://codereview.chromium.org/1520883002/), it became
clear that it'd be better to handle this in chromedriver. Here I
introduce a timeout tracking mechanism which is easy to use in the
chromedriver's architecture. In general, it allows to abort waiting for
a devtools command response, do some cleanup and return an error to the
client.

Currently this timeout tracking is used only when executing the 'get'
command, history navigations and reloads are left as is. I wanted to get
feedback about the approach as a whole first. Please take a look.


BUG=chromedriver:1292
==========

to

==========
[chromedriver] Apply page load timeout to slow cross-process navigations

This is a second attempt to fight chromedriver hangs on slow-to-commit
cross-process navigations. In the discussion of the first, incorrect
solution (see https://codereview.chromium.org/1520883002/), it became
clear that it'd be better to handle this in chromedriver. Here I
introduce a timeout tracking mechanism which is easy to use in the
chromedriver's architecture. In general, it allows to abort waiting for
a devtools command response, do some cleanup and return an error to the
client.

Currently this timeout tracking is used only when executing the 'get'
command, history navigations and reloads are left as is. I wanted to get
feedback about the approach as a whole first. Please take a look.


BUG=chromedriver:1292
==========

[Alexander Semashko, 2016-04-12 13:42:49]

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

[Alexander Semashko, 2016-04-12 13:45:31]

stgao can you take a look at this or reroute? Looks like this review stalled :(

[samuong, 2016-04-12 16:36:58]

Sorry Alex, I'll finish looking at this today.

[samuong, 2016-04-13 03:59:31]

Hi Alex,

Thanks for being patient with this.

To be honest I find some of the derived timeout logic to be a bit complicated,
but I don't as of yet have a counter-proposal. So maybe we can revisit this
later, but it's fine for now.

I still have a few questions, below, but this mostly looks good.

Sam.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/devtools_client.h (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/devtools_client.h:43: 
Once we're done with the subsequent CLs, will every call to SendCommand()
replaced with SendCommandWithTimeout()?

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/navigation_tracker.cc (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/navigation_tracker.cc:49: }
I'm not sure we want to return a kTimeout status here. If we do so, test clients
will throw a timeout exception, which could be interpreted as a script timeout
or page load timeout. But these timeouts that you're catching are related to
DevTools command timeouts, which would indicate that something's gone wrong in
the browser/renderer. Shouldn't we continue to return kUnknownError here?

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc:82: &message,
Timeout(base::TimeDelta())));
this conflicts with one of your older cls, so you'll need to rebase and fix this

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1243: # NB: Wuth a too small
timeout chromedriver might not send the
"With" instead of "Wuth"?

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1866:
ChromeDriverTest.GlobalSetUp()
Do the tests in ChromeDriverPageLoadTimeoutTest use the same web server instance
as ChromeDriverTest?

[Alexander Semashko, 2016-04-13 15:51:35]

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/devtools_client.h (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/devtools_client.h:43: 
On 2016/04/13 03:59:30, samuong wrote:
> Once we're done with the subsequent CLs, will every call to SendCommand()
> replaced with SendCommandWithTimeout()?

Most likely no. Of course we can go forward and introduce timeouts everywhere,
but it needs separate discussions/decisions. E.g. what timeout should we use for
Runtime.enable?
It is unclear to me what were the reasons for current design and what would be
desired to have.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/navigation_tracker.cc (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/navigation_tracker.cc:49: }
On 2016/04/13 03:59:30, samuong wrote:
> I'm not sure we want to return a kTimeout status here. If we do so, test
clients
> will throw a timeout exception, which could be interpreted as a script timeout
> or page load timeout. But these timeouts that you're catching are related to
> DevTools command timeouts, which would indicate that something's gone wrong in
> the browser/renderer. Shouldn't we continue to return kUnknownError here?

It may as well indicate a real timeout when browser is functioning properly
(remember that during cross-process navigations all devtools responses are
throttled until the navigation commits/aborts), and the primary goal of this CL
is to handle these timeouts. We must return kTimeout to
WebViewImpl::WaitForPendingNavigations so that it attempts to stop the
navigation. Otherwise, the navigation remains pending, which means we're unable
to command the browser.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc:82: &message,
Timeout(base::TimeDelta())));
On 2016/04/13 03:59:30, samuong wrote:
> this conflicts with one of your older cls, so you'll need to rebase and fix
this

Done.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1243: # NB: Wuth a too small
timeout chromedriver might not send the
On 2016/04/13 03:59:30, samuong wrote:
> "With" instead of "Wuth"?

Done.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1866:
ChromeDriverTest.GlobalSetUp()
On 2016/04/13 03:59:30, samuong wrote:
> Do the tests in ChromeDriverPageLoadTimeoutTest use the same web server
instance
> as ChromeDriverTest?

Yes. Are you concerned that the changes I made to the server might hide some
problems (e.g. in test environment)?

[stgao, 2016-04-13 23:24:24]

stgao@chromium.org changed reviewers:
- stgao@chromium.org

[samuong, 2016-04-15 04:23:55]

Just one more change, and I think we'll be ready to land this.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/devtools_client.h (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/devtools_client.h:43: 
On 2016/04/13 15:51:35, Alexander Semashko wrote:
> On 2016/04/13 03:59:30, samuong wrote:
> > Once we're done with the subsequent CLs, will every call to SendCommand()
> > replaced with SendCommandWithTimeout()?
> 
> Most likely no. Of course we can go forward and introduce timeouts everywhere,
> but it needs separate discussions/decisions. E.g. what timeout should we use
for
> Runtime.enable?
> It is unclear to me what were the reasons for current design and what would be
> desired to have.

Ok, that makes sense. I'm not sure which part of the design you're unclear on
but we can discuss this later outside of the CL if you like.

Can we keep this class pure virtual? Just define the *WithTimeout() functions
like this in StubDevToolsClient and delete devtools_client.cc from this CL.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/navigation_tracker.cc (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/navigation_tracker.cc:49: }
On 2016/04/13 15:51:35, Alexander Semashko wrote:
> On 2016/04/13 03:59:30, samuong wrote:
> > I'm not sure we want to return a kTimeout status here. If we do so, test
> clients
> > will throw a timeout exception, which could be interpreted as a script
timeout
> > or page load timeout. But these timeouts that you're catching are related to
> > DevTools command timeouts, which would indicate that something's gone wrong
in
> > the browser/renderer. Shouldn't we continue to return kUnknownError here?
> 
> It may as well indicate a real timeout when browser is functioning properly
> (remember that during cross-process navigations all devtools responses are
> throttled until the navigation commits/aborts), and the primary goal of this
CL
> is to handle these timeouts. We must return kTimeout to
> WebViewImpl::WaitForPendingNavigations so that it attempts to stop the
> navigation. Otherwise, the navigation remains pending, which means we're
unable
> to command the browser.

OK, sounds good.

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1866:
ChromeDriverTest.GlobalSetUp()
On 2016/04/13 15:51:35, Alexander Semashko wrote:
> On 2016/04/13 03:59:30, samuong wrote:
> > Do the tests in ChromeDriverPageLoadTimeoutTest use the same web server
> instance
> > as ChromeDriverTest?
> 
> Yes. Are you concerned that the changes I made to the server might hide some
> problems (e.g. in test environment)?

We're currently spinning up 2 http servers, which now get used across 3
different test classes. I think it would be nice if we could get all the tests
to share the same server, but I can handle this in a separate CL since it's not
really your problem.

[Alexander Semashko, 2016-04-15 15:11:48]

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/devtools_client.h (right):

https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/devtools_client.h:43: 
On 2016/04/15 04:23:55, samuong wrote:
> On 2016/04/13 15:51:35, Alexander Semashko wrote:
> > On 2016/04/13 03:59:30, samuong wrote:
> > > Once we're done with the subsequent CLs, will every call to SendCommand()
> > > replaced with SendCommandWithTimeout()?
> > 
> > Most likely no. Of course we can go forward and introduce timeouts
everywhere,
> > but it needs separate discussions/decisions. E.g. what timeout should we use
> for
> > Runtime.enable?
> > It is unclear to me what were the reasons for current design and what would
be
> > desired to have.
> 
> Ok, that makes sense. I'm not sure which part of the design you're unclear on
> but we can discuss this later outside of the CL if you like.

I'm thinking of the 10 minute timeout in SendCommandInternal. Each message
coming from the browser restarts the timeout, so if there are periodically
coming events (is it possible? smth like "DOM changed"), we may never return. Is
it intended to be so? Or did I misread something?

> Can we keep this class pure virtual? Just define the *WithTimeout() functions
> like this in StubDevToolsClient and delete devtools_client.cc from this CL.

Done.

[samuong, 2016-04-15 16:59:59]

On 2016/04/15 15:11:48, Alexander Semashko wrote:
>
https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
> File chrome/test/chromedriver/chrome/devtools_client.h (right):
> 
>
https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
> chrome/test/chromedriver/chrome/devtools_client.h:43: 
> On 2016/04/15 04:23:55, samuong wrote:
> > On 2016/04/13 15:51:35, Alexander Semashko wrote:
> > > On 2016/04/13 03:59:30, samuong wrote:
> > > > Once we're done with the subsequent CLs, will every call to
SendCommand()
> > > > replaced with SendCommandWithTimeout()?
> > > 
> > > Most likely no. Of course we can go forward and introduce timeouts
> everywhere,
> > > but it needs separate discussions/decisions. E.g. what timeout should we
use
> > for
> > > Runtime.enable?
> > > It is unclear to me what were the reasons for current design and what
would
> be
> > > desired to have.
> > 
> > Ok, that makes sense. I'm not sure which part of the design you're unclear
on
> > but we can discuss this later outside of the CL if you like.
> 
> I'm thinking of the 10 minute timeout in SendCommandInternal. Each message
> coming from the browser restarts the timeout, so if there are periodically
> coming events (is it possible? smth like "DOM changed"), we may never return.
Is
> it intended to be so? Or did I misread something?
> 
> > Can we keep this class pure virtual? Just define the *WithTimeout()
functions
> > like this in StubDevToolsClient and delete devtools_client.cc from this CL.
> 
> Done.

lgtm, thanks!

When we're doing things like performance tracing, we might get periodic trace
events, but I don't believe we have any periodic events in the normal case -
they're all triggered off of things like page changes and navigation events. 10
minutes is a large timeout, it's meant to handle really rare cases, or clients
that don't bother to set any timeout.

[Alexander Semashko, 2016-04-15 17:07:12]

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

[commit-bot: I haz the power, 2016-04-15 17:07:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1669453002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1669453002/100001

[Alexander Semashko, 2016-04-15 17:12:12]

On 2016/04/15 16:59:59, samuong wrote:
> On 2016/04/15 15:11:48, Alexander Semashko wrote:
> >
>
https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
> > File chrome/test/chromedriver/chrome/devtools_client.h (right):
> > 
> >
>
https://codereview.chromium.org/1669453002/diff/40001/chrome/test/chromedrive...
> > chrome/test/chromedriver/chrome/devtools_client.h:43: 
> > On 2016/04/15 04:23:55, samuong wrote:
> > > On 2016/04/13 15:51:35, Alexander Semashko wrote:
> > > > On 2016/04/13 03:59:30, samuong wrote:
> > > > > Once we're done with the subsequent CLs, will every call to
> SendCommand()
> > > > > replaced with SendCommandWithTimeout()?
> > > > 
> > > > Most likely no. Of course we can go forward and introduce timeouts
> > everywhere,
> > > > but it needs separate discussions/decisions. E.g. what timeout should we
> use
> > > for
> > > > Runtime.enable?
> > > > It is unclear to me what were the reasons for current design and what
> would
> > be
> > > > desired to have.
> > > 
> > > Ok, that makes sense. I'm not sure which part of the design you're unclear
> on
> > > but we can discuss this later outside of the CL if you like.
> > 
> > I'm thinking of the 10 minute timeout in SendCommandInternal. Each message
> > coming from the browser restarts the timeout, so if there are periodically
> > coming events (is it possible? smth like "DOM changed"), we may never
return.
> Is
> > it intended to be so? Or did I misread something?
> > 
> > > Can we keep this class pure virtual? Just define the *WithTimeout()
> functions
> > > like this in StubDevToolsClient and delete devtools_client.cc from this
CL.
> > 
> > Done.
> 
> lgtm, thanks!
> 
> When we're doing things like performance tracing, we might get periodic trace
> events, but I don't believe we have any periodic events in the normal case -
> they're all triggered off of things like page changes and navigation events.
10
> minutes is a large timeout, it's meant to handle really rare cases, or clients
> that don't bother to set any timeout.

Ok, thanks for the explanation.

[commit-bot: I haz the power, 2016-04-15 18:41:21]

Description was changed from

==========
[chromedriver] Apply page load timeout to slow cross-process navigations

This is a second attempt to fight chromedriver hangs on slow-to-commit
cross-process navigations. In the discussion of the first, incorrect
solution (see https://codereview.chromium.org/1520883002/), it became
clear that it'd be better to handle this in chromedriver. Here I
introduce a timeout tracking mechanism which is easy to use in the
chromedriver's architecture. In general, it allows to abort waiting for
a devtools command response, do some cleanup and return an error to the
client.

Currently this timeout tracking is used only when executing the 'get'
command, history navigations and reloads are left as is. I wanted to get
feedback about the approach as a whole first. Please take a look.


BUG=chromedriver:1292
==========

to

==========
[chromedriver] Apply page load timeout to slow cross-process navigations

This is a second attempt to fight chromedriver hangs on slow-to-commit
cross-process navigations. In the discussion of the first, incorrect
solution (see https://codereview.chromium.org/1520883002/), it became
clear that it'd be better to handle this in chromedriver. Here I
introduce a timeout tracking mechanism which is easy to use in the
chromedriver's architecture. In general, it allows to abort waiting for
a devtools command response, do some cleanup and return an error to the
client.

Currently this timeout tracking is used only when executing the 'get'
command, history navigations and reloads are left as is. I wanted to get
feedback about the approach as a whole first. Please take a look.


BUG=chromedriver:1292
==========

[commit-bot: I haz the power, 2016-04-15 18:41:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-15 18:43:08]

Description was changed from

==========
[chromedriver] Apply page load timeout to slow cross-process navigations

This is a second attempt to fight chromedriver hangs on slow-to-commit
cross-process navigations. In the discussion of the first, incorrect
solution (see https://codereview.chromium.org/1520883002/), it became
clear that it'd be better to handle this in chromedriver. Here I
introduce a timeout tracking mechanism which is easy to use in the
chromedriver's architecture. In general, it allows to abort waiting for
a devtools command response, do some cleanup and return an error to the
client.

Currently this timeout tracking is used only when executing the 'get'
command, history navigations and reloads are left as is. I wanted to get
feedback about the approach as a whole first. Please take a look.


BUG=chromedriver:1292
==========

to

==========
[chromedriver] Apply page load timeout to slow cross-process navigations

This is a second attempt to fight chromedriver hangs on slow-to-commit
cross-process navigations. In the discussion of the first, incorrect
solution (see https://codereview.chromium.org/1520883002/), it became
clear that it'd be better to handle this in chromedriver. Here I
introduce a timeout tracking mechanism which is easy to use in the
chromedriver's architecture. In general, it allows to abort waiting for
a devtools command response, do some cleanup and return an error to the
client.

Currently this timeout tracking is used only when executing the 'get'
command, history navigations and reloads are left as is. I wanted to get
feedback about the approach as a whole first. Please take a look.

BUG=chromedriver:1292

Committed: https://crrev.com/091637f2223b51356cfeb3f89bb03394ed1f2b6a
Cr-Commit-Position: refs/heads/master@{#387658}
==========

[commit-bot: I haz the power, 2016-04-15 18:43:09]

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