Allow devtools clients to handle slow cross-process navigations.

Allow devtools clients to handle slow cross-process navigations.

This issue mostly affects chromedriver, I think. If a navigation is
performed in the same renderer, page load timeout is taken into account;
slow navigation gets cancelled no matter whether it is committed. On the
other hand, during a cross-process navigation chromedriver does not
respond until the navigation is committed or discarded, the page load
timeout is not applied. If e.g. selenium receives socket timeout and
then sends some more commands, the driver still does not respond, i.e.
it becomes unusable.


BUG=chromedriver:1292

[Alexander Semashko, 2015-12-11 13:14:14]

ahest@yandex-team.ru changed reviewers:
+ dgozman@chromium.org, kkania@chromium.org

[Alexander Semashko, 2015-12-11 13:14:14]

Of course this CL is not ready to be committed, it just shows what
changes are needed to make the main issue with chromedriver go away.
The current patch has several downsides:
 - The client gets Runtime.executionContextsCleared; FrameTracker in
   chromedriver clears its frame-to-context-id map.
   Probably this should be tied to a top-level frame id?
 - Also after cancelling the navigation, chromedriver gets an unexpected
   second response to the Page.navigate command and complains about it,
   though it seems safe to ignore.

I could not find a better solution. In any way it looks like RFDTAH
needs to be aware of the messages it dispatches and behave differently
with some of them:
 - response to Page.navigate from the pending renderer should be sent
   immediately, and response from the old renderer should be discarded;
 - Page.frameStartedLoading and Page.frameStoppedLoading from the
   pending renderer should also be sent.
It only has to be implemented in a less ugly way, and probably more generic. 

Maybe I'm missing something? Please share your thoughts.

[dgozman, 2015-12-11 18:39:04]

On 2015/12/11 13:14:14, ahest wrote:
> Of course this CL is not ready to be committed, it just shows what
> changes are needed to make the main issue with chromedriver go away.
> The current patch has several downsides:
>  - The client gets Runtime.executionContextsCleared; FrameTracker in
>    chromedriver clears its frame-to-context-id map.
>    Probably this should be tied to a top-level frame id?
>  - Also after cancelling the navigation, chromedriver gets an unexpected
>    second response to the Page.navigate command and complains about it,
>    though it seems safe to ignore.
> 
> I could not find a better solution. In any way it looks like RFDTAH
> needs to be aware of the messages it dispatches and behave differently
> with some of them:
>  - response to Page.navigate from the pending renderer should be sent
>    immediately, and response from the old renderer should be discarded;
>  - Page.frameStartedLoading and Page.frameStoppedLoading from the
>    pending renderer should also be sent.
> It only has to be implemented in a less ugly way, and probably more generic. 
> 
> Maybe I'm missing something? Please share your thoughts.

I don't really follow you. AFAIK, we only respond to Page.navigate once when the
navigation has committed. Do you propose to send the response earlier? That's
not feasible since we don't really know the frameId until navigation has
committed.

Is the problem that we don't ever respond when navigation was discarded? I think
we can fix this if required. Didn't yet strike anyone though.

Could you please dump the protocol commands/responses/events (with timestamps if
possible) for the case you have troubles with? Then we can figure out what the
problem is.

[Alexander Semashko, 2015-12-11 23:14:20]

On 2015/12/11 18:39:04, dgozman wrote:
> On 2015/12/11 13:14:14, ahest wrote:
> > Of course this CL is not ready to be committed, it just shows what
> > changes are needed to make the main issue with chromedriver go away.
> > The current patch has several downsides:
> >  - The client gets Runtime.executionContextsCleared; FrameTracker in
> >    chromedriver clears its frame-to-context-id map.
> >    Probably this should be tied to a top-level frame id?
> >  - Also after cancelling the navigation, chromedriver gets an unexpected
> >    second response to the Page.navigate command and complains about it,
> >    though it seems safe to ignore.
> > 
> > I could not find a better solution. In any way it looks like RFDTAH
> > needs to be aware of the messages it dispatches and behave differently
> > with some of them:
> >  - response to Page.navigate from the pending renderer should be sent
> >    immediately, and response from the old renderer should be discarded;
> >  - Page.frameStartedLoading and Page.frameStoppedLoading from the
> >    pending renderer should also be sent.
> > It only has to be implemented in a less ugly way, and probably more generic.

> > 
> > Maybe I'm missing something? Please share your thoughts.

> I don't really follow you. AFAIK, we only respond to Page.navigate once when
the
> navigation has committed. Do you propose to send the response earlier? That's
> not feasible since we don't really know the frameId until navigation has
> committed.
> 

No, the response is sent immediately, please see the logs below for example.

> Is the problem that we don't ever respond when navigation was discarded? I
think
> we can fix this if required. Didn't yet strike anyone though.
> 

Not exactly. The problem is that with a hung navigation we don't respond
until it is discarded, and it can not be discarded without the response.
Wait... After writing this I thought "Why not to break this cycle on the
other side?"
But that will require a heavy change to the way that executing commands
is implemented in chromedriver. The timeout should be applied to the
wait for command response (currently it waits forever for the response,
and then waits with the timeout until the page stops loading, calling
window.stop() on timeout).
Ken, WDYT - is it feasible? Would there be more problems if we continue
normal operation without having received response to a command (probably
only to a specific subset of commands)?

> Could you please dump the protocol commands/responses/events (with timestamps
if
> possible) for the case you have troubles with? Then we can figure out what the
> problem is.

Of course. Here is the log of what happens when navigating to a not
responding page in the same renderer: http://pastebin.com/BekfpJpJ
And here is the log of cross-process navigation with this patch:
http://pastebin.com/efWDcJi5
Without the patch there is nothing to see, because messages from both
pending and current devtools agents are suspended and remain so forever.

[dgozman, 2015-12-14 22:04:39]

Thanks for clarifications.
As I understand this now, the problem is that navigation hangs (for whatever
reason), and protocol does not respond anymore. Am I right?

If so, what happens in the page? Is there any reason to talk to it, if there is
nothing committed inside? Perhaps, you should not communicate with it unless it
has committed. WDYT?
In this case I think that the best solution is to set up a timeout on your side
(since you users expect the timeout to happen, right?) and just navigate the
page to something new after timeout.

[Alexander Semashko, 2015-12-14 23:09:44]

On 2015/12/14 22:04:39, dgozman wrote:
> Thanks for clarifications.
> As I understand this now, the problem is that navigation hangs (for whatever
> reason), and protocol does not respond anymore. Am I right?
> 

Yes.

> If so, what happens in the page? Is there any reason to talk to it, if there
is
> nothing committed inside? Perhaps, you should not communicate with it unless
it
> has committed. WDYT?

At a high level, such a difference between same-process and
cross-process navigations just looks strange (remember that in the
same-process case the client gets all the messages it needs before
commit). I suppose that such differences should be hidden from devtools
clients, because the client can neither know if a navigation will be
cross-process nor control this aspect. WDYT?

> In this case I think that the best solution is to set up a timeout on your
side
> (since you users expect the timeout to happen, right?) and just navigate the
> page to something new after timeout.

Well, I posted a similar question in my previous comment. But I'm not
sure whether
a) is it acceptable w.r.t. the current chromedriver
architecture? (hence the question is addressed to kkania)
b) is it ok from the point of view of devtools protocol design? E.g.
there will be unexpected responses from the browser that should be
silenced, and probably some other things to deal with.

[dgozman, 2015-12-14 23:30:45]

On 2015/12/14 23:09:44, ahest wrote:
> On 2015/12/14 22:04:39, dgozman wrote:
> > Thanks for clarifications.
> > As I understand this now, the problem is that navigation hangs (for whatever
> > reason), and protocol does not respond anymore. Am I right?
> > 
> 
> Yes.
> 
> > If so, what happens in the page? Is there any reason to talk to it, if there
> is
> > nothing committed inside? Perhaps, you should not communicate with it unless
> it
> > has committed. WDYT?
> 
> At a high level, such a difference between same-process and
> cross-process navigations just looks strange (remember that in the
> same-process case the client gets all the messages it needs before
> commit). I suppose that such differences should be hidden from devtools
> clients, because the client can neither know if a navigation will be
> cross-process nor control this aspect. WDYT?

That's a good point. Although we do not usually guarantee anything about
relative order of commands,
we can improve some particular usecases. Do you have specific order issues?

I don't think we can really handle commands and respond until navigation
commits, since results could be completely different depending on the
commit/discard status. It just happens to work synchronously in renderer-only
navigation, but we can't rely on that. I presume the correct log would be:

Page.navigate
....waiting....
events like Runtime.executionContextCreated or Page.frameStartedLoading, in no
particular order
Page.navigate response, which means we are sure the page has committed or
discarded the navigation request
other events like Runtime.executionContextCreated or Page.frameStartedLoading,
in no particular order



> 
> > In this case I think that the best solution is to set up a timeout on your
> side
> > (since you users expect the timeout to happen, right?) and just navigate the
> > page to something new after timeout.
> 
> Well, I posted a similar question in my previous comment. But I'm not
> sure whether
> a) is it acceptable w.r.t. the current chromedriver
> architecture? (hence the question is addressed to kkania)
> b) is it ok from the point of view of devtools protocol design? E.g.
> there will be unexpected responses from the browser that should be
> silenced, and probably some other things to deal with.
See above for my point of view about devtools protocol.
Not sure what do you mean by unexpected responses from browser. Are those
protocol responses? We try to maintain perfect consistency, so please report
when you get extra responses from any use of the protocol.

[Alexander Semashko, 2016-02-03 16:12:29]

Sorry for a long delay. I finally made a patch that handles timeouts on the
chromedriver side, and it's working fine. Sent it for review, you can see it in
https://codereview.chromium.org/1669453002/ if interested.

[Alexander Semashko, 2016-06-05 20:54:14]

Description was changed from

==========
Allow devtools clients to handle slow cross-process navigations.

This issue mostly affects chromedriver, I think. If a navigation is
performed in the same renderer, page load timeout is taken into account;
slow navigation gets cancelled no matter whether it is committed. On the
other hand, during a cross-process navigation chromedriver does not
respond until the navigation is committed or discarded, the page load
timeout is not applied. If e.g. selenium receives socket timeout and
then sends some more commands, the driver still does not respond, i.e.
it becomes unusable.


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

to

==========
Allow devtools clients to handle slow cross-process navigations.

This issue mostly affects chromedriver, I think. If a navigation is
performed in the same renderer, page load timeout is taken into account;
slow navigation gets cancelled no matter whether it is committed. On the
other hand, during a cross-process navigation chromedriver does not
respond until the navigation is committed or discarded, the page load
timeout is not applied. If e.g. selenium receives socket timeout and
then sends some more commands, the driver still does not respond, i.e.
it becomes unusable.


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

[Alexander Semashko, 2016-06-05 20:54:14]

ahest@yandex-team.ru changed reviewers:
- dgozman@chromium.org, kkania@chromium.org