Set navigationStart correctly for all load types.

Set navigationStart correctly for all load types.

Currently for browser-initiated navigations the navigationStart recorded in the
browser process is discarded for "non-standard" load types, e.g. history
navigations and navigations with should_replace_current_entry flag set. As per
the comments in code, this is done to prevent setting navigationStart before
prompting to unload the previous document. Sadly it is quite wrong and only
causes inconsistency in the meaning of navigationStart (what it means depends on
load type, though the spec makes no distinctions between them). And still in
same-process navigations with WebFrameLoadType::Standard we take the
browser-provided navigationStart, then fire beforeunload (probably waiting for
user action on the dialog) and keep the old timestamp as is.

In this CL navigationStart is reset to Now() after calling a beforeunload
handler. On the other hand, it is not reset for load types other than standard,
if beforeunload have been called earlier or the frame being navigated contains
just an untouched initial document (guaranteed to be empty).

Committed: https://crrev.com/eacad60524fdf286eccc22ebc160385cda2ad99c
Cr-Commit-Position: refs/heads/master@{#415431}

[Alexander Semashko, 2016-06-28 12:29:27]

ahest@yandex-team.ru changed reviewers:
+ clamy@chromium.org, csharrison@chromium.org, jochen@chromium.org

[Alexander Semashko, 2016-06-28 12:29:28]

PTAL. More tests are likely needed, but I'd like to see WDYT in general first.
And probably there could be a more elegant condition for resetting
navigation_start in RenderFrameImpl::didHandleOnBeforeUnloadEvent?

[jochen (gone - plz use gerrit), 2016-06-28 13:17:25]

which part do you want me to review?

[Alexander Semashko, 2016-06-28 13:43:15]

On 2016/06/28 13:17:25, jochen wrote:
> which part do you want me to review?

third_party/WebKit.
Though I've added you because you were reviewing highly related
https://codereview.chromium.org/1425823002/ and a subsequent CL, and thought
that you're familiar with the issue. Sorry if I'm wrong.

[Charlie Harrison, 2016-06-28 14:01:43]

Thanks a lot for working on this and the general approach looks good to me. I
haven't done a full pass but left a few comments on the tests.

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2057: DocumentState* document_state
= DocumentState::FromDataSource(ds);
Getting the navigation start from the document_state is a bit of an
implementation detail. Do you mind writing this test so we get it from the more
public APIs like IPC or WebPerformance?

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2147: // not fired yet.
This comment is unclear. If we're getting the DidStartProvisionalLoad IPC then
we probably should have fired beforeunload at some point. Either the comment
should change or the test should change.

[Alexander Semashko, 2016-06-28 14:47:45]

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2057: DocumentState* document_state
= DocumentState::FromDataSource(ds);
On 2016/06/28 14:01:43, csharrison wrote:
> Getting the navigation start from the document_state is a bit of an
> implementation detail. Do you mind writing this test so we get it from the
more
> public APIs like IPC or WebPerformance?

It's ok to take it from the IPC (to avoid comparing after lossy conversions),
but I think it would be better to move the
RendererNavigationStartTransmittedToBrowser test above it then (to keep the
tests in a natural order), WDYT?

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2147: // not fired yet.
On 2016/06/28 14:01:43, csharrison wrote:
> This comment is unclear. If we're getting the DidStartProvisionalLoad IPC then
> we probably should have fired beforeunload at some point. Either the comment
> should change or the test should change.

I meant that it is not fired at the moment where the comment is (before the call
to Navigate). I'll rephrase the comment.

[Charlie Harrison, 2016-06-28 15:54:15]

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2057: DocumentState* document_state
= DocumentState::FromDataSource(ds);
On 2016/06/28 at 14:47:45, Alexander Semashko wrote:
> On 2016/06/28 14:01:43, csharrison wrote:
> > Getting the navigation start from the document_state is a bit of an
> > implementation detail. Do you mind writing this test so we get it from the
more
> > public APIs like IPC or WebPerformance?
> 
> It's ok to take it from the IPC (to avoid comparing after lossy conversions),
but I think it would be better to move the
RendererNavigationStartTransmittedToBrowser test above it then (to keep the
tests in a natural order), WDYT?

Sounds good to me.

[Alexander Semashko, 2016-06-29 10:11:56]

I added more tests and updated the comment in WebFrameClient.h. My todo list for
this CL is now empty, so PTAL!

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (left):

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2069: base::TimeTicks
lower_bound_navigation_start;
Looks like it had to be initialized to Now(), right?

[clamy, 2016-06-29 11:15:27]

Thanks! As explained in my comment below, I think the patch can be made a lot
shorter.

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3670: void
RenderFrameImpl::didHandleOnBeforeUnloadEvent(bool eventListenerCalled) {
In the case of a same-process navigation, the BeforeUnload event is actually
dispatched from RenderFrameImpl itself in decidePolicyForNavigation. If you get
the time just after it's done, this is when it finishes being handled. This
would be simpler than going through Blink.

It is also more correct. Under the current version of the patch, we could have
issues where we have an ongoing same-process navigation. A cross-process
navigation starts, which lead to the BeforeUnload event being executed in the
renderer. The patch would update the earlier same-process navigation start
time-stamp, which is wrong in that case.

[Alexander Semashko, 2016-06-29 15:08:10]

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3670: void
RenderFrameImpl::didHandleOnBeforeUnloadEvent(bool eventListenerCalled) {
On 2016/06/29 11:15:26, clamy wrote:
> In the case of a same-process navigation, the BeforeUnload event is actually
> dispatched from RenderFrameImpl itself in decidePolicyForNavigation. If you
get
> the time just after it's done, this is when it finishes being handled. This
> would be simpler than going through Blink.

I thought somewhy that some other call path are possible. Ok then, we can avoid
adding new method to all those clients. But we still need to get the
eventListenerCalled flag (e.g. via an out parameter). I'll upload a new patch
shortly, let's see if it looks better, though it won't be really much shorter.

> It is also more correct. Under the current version of the patch, we could have
> issues where we have an ongoing same-process navigation. A cross-process
> navigation starts, which lead to the BeforeUnload event being executed in the
> renderer. The patch would update the earlier same-process navigation start
> time-stamp, which is wrong in that case.

I don't get how it could happen. First of all, won't the second navigation
cancel the first one?
All this stuff is executed during RFI::NavigateInternal, and by the time it
finishes, either the new navigation is dropped or navigation_start is put into
the DataSource when it is created. And this DataSource, which is also a
DocumentLoader, replaces the old provisional document loader. Am I wrong?

[clamy, 2016-06-29 15:23:13]

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3670: void
RenderFrameImpl::didHandleOnBeforeUnloadEvent(bool eventListenerCalled) {
On 2016/06/29 15:08:09, Alexander Semashko wrote:
> On 2016/06/29 11:15:26, clamy wrote:
> > In the case of a same-process navigation, the BeforeUnload event is actually
> > dispatched from RenderFrameImpl itself in decidePolicyForNavigation. If you
> get
> > the time just after it's done, this is when it finishes being handled. This
> > would be simpler than going through Blink.
> 
> I thought somewhy that some other call path are possible. Ok then, we can
avoid
> adding new method to all those clients. But we still need to get the
> eventListenerCalled flag (e.g. via an out parameter). I'll upload a new patch
> shortly, let's see if it looks better, though it won't be really much shorter.

This changed around ~a month ago. The only other possible codepath is as a
result of a BeforeUnload IPC from the browser process.

Why do you need to check if the eventListener was called? We currently don't
check for that in cross-process navigations, and still update the timestamp.

> 
> > It is also more correct. Under the current version of the patch, we could
have
> > issues where we have an ongoing same-process navigation. A cross-process
> > navigation starts, which lead to the BeforeUnload event being executed in
the
> > renderer. The patch would update the earlier same-process navigation start
> > time-stamp, which is wrong in that case.
> 
> I don't get how it could happen. First of all, won't the second navigation
> cancel the first one?
> All this stuff is executed during RFI::NavigateInternal, and by the time it
> finishes, either the new navigation is dropped or navigation_start is put into
> the DataSource when it is created. And this DataSource, which is also a
> DocumentLoader, replaces the old provisional document loader. Am I wrong?
> 

Well the second navigation won't cancel the first one if the first navigation is
same-process and the second navigation is cross-process. That said, I did read a
bit too quickly and since you're modifying the pending_nav_params it should be
ok in that case.

Thinking about it some more though, I see two potentially problematic use cases:
- transfer navigations: we don't want to update the start time when we start the
navigation in the transfer renderer (I'm not sure we execute BeforeUnload in
that case)
- it is possible for a history navigation to be suspended in Blink and only
scheduled later. In that case, I'm not quite sure what happens with the
navigation start. Probably it starts when we create the DataSource.

[Alexander Semashko, 2016-06-29 17:33:30]

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3670: void
RenderFrameImpl::didHandleOnBeforeUnloadEvent(bool eventListenerCalled) {
On 2016/06/29 15:23:13, clamy wrote:
> On 2016/06/29 15:08:09, Alexander Semashko wrote:
> > On 2016/06/29 11:15:26, clamy wrote:
> > > In the case of a same-process navigation, the BeforeUnload event is
actually
> > > dispatched from RenderFrameImpl itself in decidePolicyForNavigation. If
you
> > get
> > > the time just after it's done, this is when it finishes being handled.
This
> > > would be simpler than going through Blink.
> > 
> > I thought somewhy that some other call path are possible. Ok then, we can
> avoid
> > adding new method to all those clients. But we still need to get the
> > eventListenerCalled flag (e.g. via an out parameter). I'll upload a new
patch
> > shortly, let's see if it looks better, though it won't be really much
shorter.
> 
> This changed around ~a month ago. The only other possible codepath is as a
> result of a BeforeUnload IPC from the browser process.
> 
> Why do you need to check if the eventListener was called? We currently don't
> check for that in cross-process navigations, and still update the timestamp.

Because we use the browser-side timestamp only if this is the first navigation
in the frame and there is no beforeunload handler in the initial document.
The second condition probably can be changed to the check that the initial
document is totally untouched (i.e. didAccessInitialDocument did not happen).
I'm not sure which is better.

> > 
> > > It is also more correct. Under the current version of the patch, we could
> have
> > > issues where we have an ongoing same-process navigation. A cross-process
> > > navigation starts, which lead to the BeforeUnload event being executed in
> the
> > > renderer. The patch would update the earlier same-process navigation start
> > > time-stamp, which is wrong in that case.
> > 
> > I don't get how it could happen. First of all, won't the second navigation
> > cancel the first one?
> > All this stuff is executed during RFI::NavigateInternal, and by the time it
> > finishes, either the new navigation is dropped or navigation_start is put
into
> > the DataSource when it is created. And this DataSource, which is also a
> > DocumentLoader, replaces the old provisional document loader. Am I wrong?
> > 
> 
> Well the second navigation won't cancel the first one if the first navigation
is
> same-process and the second navigation is cross-process.

Are we both discussing the case when a frame begins a navigation and then
receives a FrameMsg_Navigate? I read your comments as saying that a single
RenderFrame could have 2 pending navigations in it simultaneously, and I still
think it's impossible.

> That said, I did read a
> bit too quickly and since you're modifying the pending_nav_params it should be
> ok in that case.

> Thinking about it some more though, I see two potentially problematic use
cases:
> - transfer navigations: we don't want to update the start time when we start
the
> navigation in the transfer renderer (I'm not sure we execute BeforeUnload in
> that case)
> - it is possible for a history navigation to be suspended in Blink and only
> scheduled later. In that case, I'm not quite sure what happens with the
> navigation start. Probably it starts when we create the DataSource.

I'll check transfers. Can you provide some pointers where to look for these
suspended history navs?

[clamy, 2016-06-30 09:59:49]

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3670: void
RenderFrameImpl::didHandleOnBeforeUnloadEvent(bool eventListenerCalled) {
On 2016/06/29 17:33:30, Alexander Semashko wrote:
> On 2016/06/29 15:23:13, clamy wrote:
> > On 2016/06/29 15:08:09, Alexander Semashko wrote:
> > > On 2016/06/29 11:15:26, clamy wrote:
> > > > In the case of a same-process navigation, the BeforeUnload event is
> actually
> > > > dispatched from RenderFrameImpl itself in decidePolicyForNavigation. If
> you
> > > get
> > > > the time just after it's done, this is when it finishes being handled.
> This
> > > > would be simpler than going through Blink.
> > > 
> > > I thought somewhy that some other call path are possible. Ok then, we can
> > avoid
> > > adding new method to all those clients. But we still need to get the
> > > eventListenerCalled flag (e.g. via an out parameter). I'll upload a new
> patch
> > > shortly, let's see if it looks better, though it won't be really much
> shorter.
> > 
> > This changed around ~a month ago. The only other possible codepath is as a
> > result of a BeforeUnload IPC from the browser process.
> > 
> > Why do you need to check if the eventListener was called? We currently don't
> > check for that in cross-process navigations, and still update the timestamp.
> 
> Because we use the browser-side timestamp only if this is the first navigation
> in the frame and there is no beforeunload handler in the initial document.
> The second condition probably can be changed to the check that the initial
> document is totally untouched (i.e. didAccessInitialDocument did not happen).
> I'm not sure which is better.

I don't agree fully with this. Conceptually we have two cases:
1) This is the first navigation in a tab/window. There is no prior document, so
we should compute the navigation start as when we actually start the navigation.
2) We already have a document in the frame in which we're navigating. In this
case, navigation start should be the moment we finished the BeforeUnload event
in the previous document of the frame.

The important thing here is that we care about the frame as a concept, which due
to the multi-process architecture of navigation is _not_ fully equivalent to the
RenderFrame object. Instead, we should think about the FrameTreeNode that exists
browser-side. Taking into account that we have a multi-process architecture,
this gives the following:
A) we're navigating same-process. The document whose BeforeUnload event we care
about is the one that is inside the RenderFrame we're navigating. So here we
should update the navigation start. We can just check that we have a
current_history_item_.
B) we're navigating cross-process. The document whose BeforeUnload event we care
about is _not_ the one that is inside the RenderFrame we're navigating, it is
located in a different process. When we navigate cross-process we create a brand
new RenderFrame that is not even shown to the user. There is absolutely no
reason for any BeforeUnload event to execute in that newly created RenderFrame,
and we should not update the navigation start, since we care about BeforeUnload
in a completely different RenderFrame.
C) Transfer navigations. In that case, once the proper navigation start has been
computed in the initial renderer (whether the navigation was same-process or
cross-process), it should be kept when we switch to a new renderer. Note: it's
quite possible that this broken right now.

Going back to the implementation (apart from transfer mnavigations), this boils
down to two cases:
1) You've already navigated in this RenderFrame. Then we should update the
navigation start time after prompting the current document for BeforeUnload
(this is the same-process case).
2) You've never navigated in this RenderFrame. This is either the navigation in
a new tab or the cross-process case. In that case we should not update the
navigation start time.
Therefore, I don't think the knowledge of whether there was any event listener
executing is not needed.

> 
> > > 
> > > > It is also more correct. Under the current version of the patch, we
could
> > have
> > > > issues where we have an ongoing same-process navigation. A cross-process
> > > > navigation starts, which lead to the BeforeUnload event being executed
in
> > the
> > > > renderer. The patch would update the earlier same-process navigation
start
> > > > time-stamp, which is wrong in that case.
> > > 
> > > I don't get how it could happen. First of all, won't the second navigation
> > > cancel the first one?
> > > All this stuff is executed during RFI::NavigateInternal, and by the time
it
> > > finishes, either the new navigation is dropped or navigation_start is put
> into
> > > the DataSource when it is created. And this DataSource, which is also a
> > > DocumentLoader, replaces the old provisional document loader. Am I wrong?
> > > 
> > 
> > Well the second navigation won't cancel the first one if the first
navigation
> is
> > same-process and the second navigation is cross-process.
> 
> Are we both discussing the case when a frame begins a navigation and then
> receives a FrameMsg_Navigate? I read your comments as saying that a single
> RenderFrame could have 2 pending navigations in it simultaneously, and I still
> think it's impossible.

No I'm discussing the case where we have 2 pending navigations in two different
processes that correspond to the same frame in the browser process. Ie a
FrameTreeNode (that is a frame browser side) can have 2 RenderFrameHost
navigating (current & pending)_ that each has a communication channel to a
different RenderFrame (in a different process). The start of the cross-site
navigation requires the BeforeUnload event to execute in the _current_
RenderFrame, which may be navigating at that time.

> 
> > That said, I did read a
> > bit too quickly and since you're modifying the pending_nav_params it should
be
> > ok in that case.
> 
> > Thinking about it some more though, I see two potentially problematic use
> cases:
> > - transfer navigations: we don't want to update the start time when we start
> the
> > navigation in the transfer renderer (I'm not sure we execute BeforeUnload in
> > that case)
> > - it is possible for a history navigation to be suspended in Blink and only
> > scheduled later. In that case, I'm not quite sure what happens with the
> > navigation start. Probably it starts when we create the DataSource.
> 
> I'll check transfers. Can you provide some pointers where to look for these
> suspended history navs?

Look at m_deferredHistoryLoad in the FrameLoader:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr....

[Alexander Semashko, 2016-06-30 19:35:08]

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3670: void
RenderFrameImpl::didHandleOnBeforeUnloadEvent(bool eventListenerCalled) {
On 2016/06/30 09:59:49, clamy wrote:
> On 2016/06/29 17:33:30, Alexander Semashko wrote:
> > On 2016/06/29 15:23:13, clamy wrote:
> > > On 2016/06/29 15:08:09, Alexander Semashko wrote:
> > > > On 2016/06/29 11:15:26, clamy wrote:
> > > > > In the case of a same-process navigation, the BeforeUnload event is
> > actually
> > > > > dispatched from RenderFrameImpl itself in decidePolicyForNavigation.
If
> > you
> > > > get
> > > > > the time just after it's done, this is when it finishes being handled.
> > This
> > > > > would be simpler than going through Blink.
> > > >
> > > > I thought somewhy that some other call path are possible. Ok then, we
can
> > > avoid
> > > > adding new method to all those clients. But we still need to get the
> > > > eventListenerCalled flag (e.g. via an out parameter). I'll upload a new
> > patch
> > > > shortly, let's see if it looks better, though it won't be really much
> > shorter.
> > >
> > > This changed around ~a month ago. The only other possible codepath is as a
> > > result of a BeforeUnload IPC from the browser process.
> > >
> > > Why do you need to check if the eventListener was called? We currently
don't
> > > check for that in cross-process navigations, and still update the
timestamp.
> >
> > Because we use the browser-side timestamp only if this is the first
navigation
> > in the frame and there is no beforeunload handler in the initial document.
> > The second condition probably can be changed to the check that the initial
> > document is totally untouched (i.e. didAccessInitialDocument did not
happen).
> > I'm not sure which is better.
>
> I don't agree fully with this. Conceptually we have two cases:
> 1) This is the first navigation in a tab/window. There is no prior document,
so
> we should compute the navigation start as when we actually start the
navigation.

There is always an initial blank document created when the frame itself is
created.

> 2) We already have a document in the frame in which we're navigating. In this
> case, navigation start should be the moment we finished the BeforeUnload event
> in the previous document of the frame.
>
> The important thing here is that we care about the frame as a concept, which
due
> to the multi-process architecture of navigation is _not_ fully equivalent to
the
> RenderFrame object. Instead, we should think about the FrameTreeNode that
exists
> browser-side. Taking into account that we have a multi-process architecture,
> this gives the following:
> A) we're navigating same-process. The document whose BeforeUnload event we
care
> about is the one that is inside the RenderFrame we're navigating. So here we
> should update the navigation start. We can just check that we have a
> current_history_item_.
> B) we're navigating cross-process. The document whose BeforeUnload event we
care
> about is _not_ the one that is inside the RenderFrame we're navigating, it is
> located in a different process. When we navigate cross-process we create a
brand
> new RenderFrame that is not even shown to the user. There is absolutely no
> reason for any BeforeUnload event to execute in that newly created
RenderFrame,
> and we should not update the navigation start, since we care about
BeforeUnload
> in a completely different RenderFrame.

Yet we do call dispatchBeforeUnloadEvent in this case (it just does nothing).
But currently we can't tell if the frame really was created just a moment ago.
(see continuation below)

> C) Transfer navigations. In that case, once the proper navigation start has
been
> computed in the initial renderer (whether the navigation was same-process or
> cross-process), it should be kept when we switch to a new renderer. Note: it's
> quite possible that this broken right now.
>
> Going back to the implementation (apart from transfer mnavigations), this
boils
> down to two cases:
> 1) You've already navigated in this RenderFrame. Then we should update the
> navigation start time after prompting the current document for BeforeUnload
> (this is the same-process case).
> 2) You've never navigated in this RenderFrame. This is either the navigation
in
> a new tab or the cross-process case. In that case we should not update the
> navigation start time.
> Therefore, I don't think the knowledge of whether there was any event listener
> executing is not needed.

There is one more possible case:
3) We've never navigated in this RenderFrame, but its opener has modified the
initial document. The frame did not commit a load, its current_history_item_ is
null, but it can have arbitrary content, including a beforeunload handler. This
frame can be the main frame of a tab (created via window.open()), and when the
user navigates this tab, chrome shows a beforeunload dialog if needed. Certainly
we should not use a timestamp recorded before showing the dialog.

Though the more I think about it, the more I like the idea of remembering the
fact that the initial document was accessed and discarding browser timestamp in
this case, no matter if a beforeunload handler was present.

> >
> > > >
> > > > > It is also more correct. Under the current version of the patch, we
> could
> > > have
> > > > > issues where we have an ongoing same-process navigation. A
cross-process
> > > > > navigation starts, which lead to the BeforeUnload event being executed
> in
> > > the
> > > > > renderer. The patch would update the earlier same-process navigation
> start
> > > > > time-stamp, which is wrong in that case.
> > > >
> > > > I don't get how it could happen. First of all, won't the second
navigation
> > > > cancel the first one?
> > > > All this stuff is executed during RFI::NavigateInternal, and by the time
> it
> > > > finishes, either the new navigation is dropped or navigation_start is
put
> > into
> > > > the DataSource when it is created. And this DataSource, which is also a
> > > > DocumentLoader, replaces the old provisional document loader. Am I
wrong?
> > > >
> > >
> > > Well the second navigation won't cancel the first one if the first
> navigation
> > is
> > > same-process and the second navigation is cross-process.
> >
> > Are we both discussing the case when a frame begins a navigation and then
> > receives a FrameMsg_Navigate? I read your comments as saying that a single
> > RenderFrame could have 2 pending navigations in it simultaneously, and I
still
> > think it's impossible.
>
> No I'm discussing the case where we have 2 pending navigations in two
different
> processes that correspond to the same frame in the browser process. Ie a
> FrameTreeNode (that is a frame browser side) can have 2 RenderFrameHost
> navigating (current & pending)_ that each has a communication channel to a
> different RenderFrame (in a different process). The start of the cross-site
> navigation requires the BeforeUnload event to execute in the _current_
> RenderFrame, which may be navigating at that time.

Yeah, in this case the first navigation's start timestamp won't be modified.

> >
> > > That said, I did read a
> > > bit too quickly and since you're modifying the pending_nav_params it
should
> be
> > > ok in that case.
> >
> > > Thinking about it some more though, I see two potentially problematic use
> > cases:
> > > - transfer navigations: we don't want to update the start time when we
start
> > the
> > > navigation in the transfer renderer (I'm not sure we execute BeforeUnload
in
> > > that case)
> > > - it is possible for a history navigation to be suspended in Blink and
only
> > > scheduled later. In that case, I'm not quite sure what happens with the
> > > navigation start. Probably it starts when we create the DataSource.
> >
> > I'll check transfers. Can you provide some pointers where to look for these
> > suspended history navs?
>
> Look at m_deferredHistoryLoad in the FrameLoader:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr....

Thanks!

[Alexander Semashko, 2016-07-08 18:16:55]

Returning back to this CL.

I checked those problematic cases and it seems there are no problems.
 1. Beforeunload on transferred navigation. It must be a transfer to
    the previously active renderer, which implies that it was
    redirected, right? decidePolicyForNavigation does not call
    beforeunload on redirects, so the timestamp is not modified.
 2. Navigation start for a deferred history load is recorded when the
    actual load starts and its DataSource is created. This seems right.

So, looks like the only remaining issue is querying for the presence of
beforeunload event listeners.
Since nobody objected to replacing it with the check for
didAccessInitialDocument, I'm currently preparing a new patch.
There are some problems with testing it using RenderViewTest fixture,
so it is not ready yet.

[Alexander Semashko, 2016-07-08 23:05:32]

Finally got the tests working. PTAL once more.

[Charlie Harrison, 2016-07-12 13:56:17]

I will defer to clamy@ on the implementation but if you could, please update the
CL description to account for how the code changed (i.e. your approach), and
wrap the text manually to 80 chars per line.

Messing with navigation start can be very tricky, so the more documentation we
have for changes like this, the better.

[Alexander Semashko, 2016-07-12 16:12:42]

Description was changed from

==========
Set navigationStart correctly for all load types.

Currently for browser-initiated navigations the navigationStart recorded in the
browser process is discarded for "non-standard" load types, e.g. history
navigations and navigations with should_replace_current_entry flag set.
As per the comments in code, this is done to prevent setting navigationStart
before prompting to unload the previous document.
Sadly it is quite wrong and only causes inconsistency in the meaning of
navigationStart (what it means depends on load type, though the spec makes no
distinctions between them). And still in same-process navigations with
WebFrameLoadType::Standard we take the browser-provided navigationStart, then
fire beforeunload (probably waiting for user action on the dialog) and keep the
old timestamp as is.

This CL aims to eliminate both deviations from the spec.

BUG=552395
==========

to

==========
Set navigationStart correctly for all load types.

Currently for browser-initiated navigations the navigationStart recorded in the
browser process is discarded for "non-standard" load types, e.g. history
navigations and navigations with should_replace_current_entry flag set. As per
the comments in code, this is done to prevent setting navigationStart before
prompting to unload the previous document. Sadly it is quite wrong and only
causes inconsistency in the meaning of navigationStart (what it means depends on
load type, though the spec makes no distinctions between them). And still in
same-process navigations with WebFrameLoadType::Standard we take the
browser-provided navigationStart, then fire beforeunload (probably waiting for
user action on the dialog) and keep the old timestamp as is.

In this CL navigationStart is reset to Now() after calling a beforeunload
handler. On the other hand, it is not reset for load types other than standard,
if beforeunload have been called earlier or the frame being navigated contains
just an untouched initial document (guaranteed to be empty).
==========

[Alexander Semashko, 2016-07-12 16:14:33]

On 2016/07/12 13:56:17, csharrison wrote:
> I will defer to clamy@ on the implementation but if you could, please update
the
> CL description to account for how the code changed (i.e. your approach), and
> wrap the text manually to 80 chars per line.
> 
> Messing with navigation start can be very tricky, so the more documentation we
> have for changes like this, the better.

Sure, I updated the description.

[clamy, 2016-07-12 16:16:41]

clamy@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[clamy, 2016-07-12 16:19:09]

@creis, nasko: PTAL. I'm adding you to the review, since this is turning into
determining when we need to execute BeforeUnload following an OnNavigate
message. Right now we execute it all the time, but we shouldn't do that in a
cross-process navigation and a transfer navigation. When we determine that
correctly, we can update the navigation start time received for the browser
correctly (ie if we execute BeforeUnload then we update, otherwise we don't).

[Alexander Semashko, 2016-07-12 16:36:36]

On 2016/07/12 16:19:09, clamy wrote:
> @creis, nasko: PTAL. I'm adding you to the review, since this is turning into
> determining when we need to execute BeforeUnload following an OnNavigate
> message. Right now we execute it all the time, but we shouldn't do that in a
> cross-process navigation and a transfer navigation. When we determine that
> correctly, we can update the navigation start time received for the browser
> correctly (ie if we execute BeforeUnload then we update, otherwise we don't).

Let me correct a bit: we don't call beforeunload for navigations that are
transferred after a redirect.

[Charlie Reis, 2016-07-12 21:27:55]

On 2016/07/12 16:36:36, Alexander Semashko wrote:
> On 2016/07/12 16:19:09, clamy wrote:
> > @creis, nasko: PTAL. I'm adding you to the review, since this is turning
into
> > determining when we need to execute BeforeUnload following an OnNavigate
> > message. Right now we execute it all the time, but we shouldn't do that in a
> > cross-process navigation and a transfer navigation. When we determine that
> > correctly, we can update the navigation start time received for the browser
> > correctly (ie if we execute BeforeUnload then we update, otherwise we
don't).
> 
> Let me correct a bit: we don't call beforeunload for navigations that are
> transferred after a redirect.

I'm not sure I follow most of this (and I'm not familiar with the
navigationStart timing API), but I agree that we don't need to run beforeUnload
in the pending RenderFrame during a cross-process navigation, nor during a
transfer.  I think we never made an attempt to avoid it in the past, since there
won't be any beforeUnload handler to run, making it more or less a no-op.  If it
matters for other reasons, I'm not opposed to skipping it in those cases.

[Alexander Semashko, 2016-07-14 19:39:21]

On 2016/07/12 21:27:55, Charlie Reis wrote:
> On 2016/07/12 16:36:36, Alexander Semashko wrote:
> > On 2016/07/12 16:19:09, clamy wrote:
> > > @creis, nasko: PTAL. I'm adding you to the review, since this is turning
> into
> > > determining when we need to execute BeforeUnload following an OnNavigate
> > > message. Right now we execute it all the time, but we shouldn't do that in
a
> > > cross-process navigation and a transfer navigation. When we determine that
> > > correctly, we can update the navigation start time received for the
browser
> > > correctly (ie if we execute BeforeUnload then we update, otherwise we
> don't).
> > 
> > Let me correct a bit: we don't call beforeunload for navigations that are
> > transferred after a redirect.
> 
> I'm not sure I follow most of this (and I'm not familiar with the
> navigationStart timing API), but I agree that we don't need to run
beforeUnload
> in the pending RenderFrame during a cross-process navigation, nor during a
> transfer.  I think we never made an attempt to avoid it in the past, since
there
> won't be any beforeUnload handler to run, making it more or less a no-op.  If
it
> matters for other reasons, I'm not opposed to skipping it in those cases.

Ok, I changed conditions for calling beforeunload as clamy suggested.
I don't think it really makes an observable difference, but this way the logic
looks cleaner.

P.S. I accidentally uploaded this change along with a revase in one patchset,
sorry.

[nasko, 2016-07-18 22:20:00]

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:1114: bool did_access_initial_document_;
Why can't we just expose the bit that is already kept in blink and check that
instead of introducing another piece of state to keep track of? Especially since
it will be false for the majority of the lifetime of this object.

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
content/renderer/render_view_browsertest.cc:195: // navigation_start set to
Now() plus the given offet.
nit: "offset"

[Alexander Semashko, 2016-07-19 10:59:07]

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:1114: bool did_access_initial_document_;
On 2016/07/18 22:20:00, nasko wrote:
> Why can't we just expose the bit that is already kept in blink and check that
> instead of introducing another piece of state to keep track of? Especially
since
> it will be false for the majority of the lifetime of this object.

Indeed. Done.

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/80001/content/renderer/render...
content/renderer/render_view_browsertest.cc:195: // navigation_start set to
Now() plus the given offet.
On 2016/07/18 22:20:00, nasko wrote:
> nit: "offset"

Done.

[Alexander Semashko, 2016-07-19 22:46:05]

Since this CL will quite likely affect navigation metrics (histograms) from
users, as well as, probably, perf-tests, what is the right procedure for landing
it? I guess I have to notify perf sheriffs. What else needs to be done?

[Charlie Harrison, 2016-07-19 22:50:08]

On 2016/07/19 22:46:05, Alexander Semashko wrote:
> Since this CL will quite likely affect navigation metrics (histograms) from
> users, as well as, probably, perf-tests, what is the right procedure for
landing
> it? I guess I have to notify perf sheriffs. What else needs to be done?

You might want to also notify loading-dev@chromium.org.

[nasko, 2016-07-19 23:58:28]

On 2016/07/19 22:50:08, csharrison wrote:
> On 2016/07/19 22:46:05, Alexander Semashko wrote:
> > Since this CL will quite likely affect navigation metrics (histograms) from
> > users, as well as, probably, perf-tests, what is the right procedure for
> landing
> > it? I guess I have to notify perf sheriffs. What else needs to be done?
> 
> You might want to also notify mailto:loading-dev@chromium.org.

Ensure clamy@ is happy with the content/ side and jochen@ with the Blink side.
Once you get the approval, send it to CQ. Notifying loading-dev@ is a good idea
and keeping an eye on the perf dashboard (https://chromeperf.appspot.com/) is
also probably good.

[Alexander Semashko, 2016-07-20 13:12:46]

On 2016/07/19 23:58:28, nasko wrote:
> On 2016/07/19 22:50:08, csharrison wrote:
> > On 2016/07/19 22:46:05, Alexander Semashko wrote:
> > > Since this CL will quite likely affect navigation metrics (histograms)
from
> > > users, as well as, probably, perf-tests, what is the right procedure for
> > landing
> > > it? I guess I have to notify perf sheriffs. What else needs to be done?
> > 
> > You might want to also notify mailto:loading-dev@chromium.org.
> 
> Ensure clamy@ is happy with the content/ side and jochen@ with the Blink side.
> Once you get the approval, send it to CQ. Notifying loading-dev@ is a good
idea
> and keeping an eye on the perf dashboard (https://chromeperf.appspot.com/) is
> also probably good.

Ok, thanks.
clamy, jochen, can you take a final look, please?

> keeping an eye on the perf dashboard (https://chromeperf.appspot.com/) is
> also probably good

Unfortunately, I won't be able to watch it for too long, I'm going to 2 week
vacation starting from Saturday. Does it sound problematic? If so, probably it
would be better to defer landing.

[Bryan McQuade, 2016-07-20 19:59:49]

On 2016/07/20 at 13:12:46, ahest wrote:
> On 2016/07/19 23:58:28, nasko wrote:
> > On 2016/07/19 22:50:08, csharrison wrote:
> > > On 2016/07/19 22:46:05, Alexander Semashko wrote:
> > > > Since this CL will quite likely affect navigation metrics (histograms)
from
> > > > users, as well as, probably, perf-tests, what is the right procedure for
> > > landing
> > > > it? I guess I have to notify perf sheriffs. What else needs to be done?
> > > 
> > > You might want to also notify mailto:loading-dev@chromium.org.
> > 
> > Ensure clamy@ is happy with the content/ side and jochen@ with the Blink
side.
> > Once you get the approval, send it to CQ. Notifying loading-dev@ is a good
idea
> > and keeping an eye on the perf dashboard (https://chromeperf.appspot.com/)
is
> > also probably good.
> 
> Ok, thanks.
> clamy, jochen, can you take a final look, please?
> 
> > keeping an eye on the perf dashboard (https://chromeperf.appspot.com/) is
> > also probably good
> 
> Unfortunately, I won't be able to watch it for too long, I'm going to 2 week
vacation starting from Saturday. Does it sound problematic? If so, probably it
would be better to defer landing.

Tracing the history of this code, it looks like the logic to not override nav
start for non-Standard load types was added as part of
https://codereview.chromium.org/1157863005

clamy, can you give context on why we only overrode nav start for standard load
types? Are there any issues you see with changing that logic now? I do think we
should try to be spec compliant, but I just want to make sure we're not missing
anything that you were addressing when adding this conditional in your original
change.

[clamy, 2016-07-21 11:52:44]

Thanks! I'm happy with the code in render_frame_impl, a few questions on the
tests.

https://codereview.chromium.org/2103733004/diff/120001/content/public/test/mo...
File content/public/test/mock_render_thread.h (right):

https://codereview.chromium.org/2103733004/diff/120001/content/public/test/mo...
content/public/test/mock_render_thread.h:105: void
set_new_window_main_frame_routing_id(int32_t id) {
What is this used for?

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5065: // navigation_start must be recorded
immediately after dispatching the
nit: blank line above.
s/navigation_start/|navigation_start|

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:573: CloseRenderView(new_view);
Why were those calls needed?

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.h:109: bool
getDidAccessInitialDocument() const { return m_didAccessInitialDocument; }
Normally, for a boolean getter, we would call the method
didAccessInitialDocument, but considering we already have another one called
didAccessInitialDocument this is a bit tricky. How about calling it
hasAccessedIntialDocument (with member variable name change), and changing the
names accordingly in WebFrame?

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:138: virtual bool
getDidAccessInitialDocument() const = 0;
See my comment about naming in FrameLoader.h

[Alexander Semashko, 2016-07-21 14:00:22]

https://codereview.chromium.org/2103733004/diff/120001/content/public/test/mo...
File content/public/test/mock_render_thread.h (right):

https://codereview.chromium.org/2103733004/diff/120001/content/public/test/mo...
content/public/test/mock_render_thread.h:105: void
set_new_window_main_frame_routing_id(int32_t id) {
On 2016/07/21 11:52:44, clamy wrote:
> What is this used for?

This id is put into the response to ViewHostMsg_CreateWindow, which makes the
new RenderView create a RenderFrame with given id. The field was there, it was
just not hooked up. Note that it is initialized to 0, which is still a valid
route id, and the frame is created even if the test does not call
set_new_window_main_frame_routing_id. Probably we should initialize them to
MSG_ROUTING_NONE and fix tests, but that's a separate issue and I did not have
time for it yet.

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5065: // navigation_start must be recorded
immediately after dispatching the
On 2016/07/21 11:52:44, clamy wrote:
> nit: blank line above.
> s/navigation_start/|navigation_start|

Done.

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:573: CloseRenderView(new_view);
On 2016/07/21 11:52:44, clamy wrote:
> Why were those calls needed?

Not sure I understood your question correctly, but...
Nothing is limiting the new view's lifetime. The test itself must be its owner,
hence it deletes the view in the end. Note that I just moved those calls to this
new method CloseRenderView.

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.h:109: bool
getDidAccessInitialDocument() const { return m_didAccessInitialDocument; }
On 2016/07/21 11:52:44, clamy wrote:
> Normally, for a boolean getter, we would call the method
> didAccessInitialDocument, but considering we already have another one called
> didAccessInitialDocument this is a bit tricky. How about calling it
> hasAccessedIntialDocument (with member variable name change), and changing the
> names accordingly in WebFrame?

Fine. Done.

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2103733004/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:138: virtual bool
getDidAccessInitialDocument() const = 0;
On 2016/07/21 11:52:44, clamy wrote:
> See my comment about naming in FrameLoader.h

Done.

[jochen (gone - plz use gerrit), 2016-07-29 09:50:59]

jochen@chromium.org changed reviewers:
+ dcheng@chromium.org

[jochen (gone - plz use gerrit), 2016-07-29 09:51:00]

lgtm

+dcheng fyi

webkit lgtm

[dcheng, 2016-07-29 12:50:54]

https://codereview.chromium.org/2103733004/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2103733004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:138: virtual bool
hasAccessedInitialDocument() const = 0;
Can't we just track this bit in RenderFrameImpl rather than plumbing it through
all these layers again?

[Alexander Semashko, 2016-08-12 09:26:35]

https://codereview.chromium.org/2103733004/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2103733004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:138: virtual bool
hasAccessedInitialDocument() const = 0;
On 2016/07/29 12:50:53, dcheng (OOO Aug 1 - Aug 11) wrote:
> Can't we just track this bit in RenderFrameImpl rather than plumbing it
through
> all these layers again?

Done.

[Alexander Semashko, 2016-08-16 21:21:09]

dcheng, clamy, do you want anything else to be changed here?

[dcheng, 2016-08-17 03:19:20]

blink lgtm

[clamy, 2016-08-17 13:03:55]

Thanks! I'm happy with the code in RenderFrameImpl, some comments on the testing
code.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5076: bool should_dispatch_before_unload =
I think we may be missing the case where the navigation is transferred back to
an existing renderer (we will end up executing BeforeUnload twice). Could you
add a TODO to check for this case?

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:405: // Clean up so we don't leak
it.
nit: blank line above.
nit: avoid using we in comments.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2049: TEST_F(RenderViewImplTest,
RendererNavigationStartTransmittedToBrowser) {
Could you add comments explaining what the test does? It's not clear to me what
it is doing there.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
Isn't this test testing that the browser navigation start is used if the initial
document was not accessed? If so, can we remove
BrowserNavigationStartUsedIfInitialDocumentWasNotAccessed, which looks a lot
more complicated?

If this the case, can we also make a similar test for initial document access
where we just access the document of the current frame?

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2153:
FiringBeforeUnloadInSubFrameDiscardsBrowserNavigationStart) {
This doesn't seem correct to me: based on how the code is written, if we haven't
yet navigated anywhere we won't fire the BeforeUnload event unless the document
was accessed. So this similar to testing whether the initial document was
accessed or not.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2172: TEST_F(RenderViewImplTest,
BrowserNavigationStartNotUsedForReload) {
Can you rename it NavigationStartForReload?

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2193:
common_params.navigation_start);
Can you do the following:
if (!IsBrowserSideNavigationEnabled()) {
  // The browser navigation_start should not be used because beforeunload   will
be fired during Navigate.
  EXPECT_PRED2(...
} else {
  // PlzNavigate: the browser navigation_start is always used.
  EXPECT_EQ(common_params.navigation_start, std::get<1>(nav_params));
}

Same in the history test.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2197:
BrowserNavigationStartNotUsedForSameProcessHistoryNavigation) {
Please rename it NavigationStartForSameProcessHistoryNavigation.

[Alexander Semashko, 2016-08-17 17:10:25]

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5076: bool should_dispatch_before_unload =
On 2016/08/17 13:03:55, clamy wrote:
> I think we may be missing the case where the navigation is transferred back to
> an existing renderer (we will end up executing BeforeUnload twice). Could you
> add a TODO to check for this case?

How can this occur without a redirect? If |is_redirect| is true we don't fire
the event.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:405: // Clean up so we don't leak
it.
On 2016/08/17 13:03:55, clamy wrote:
> nit: blank line above.
> nit: avoid using we in comments.

Ok. I just removed the comment since it duplicates the comment above the
function.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2049: TEST_F(RenderViewImplTest,
RendererNavigationStartTransmittedToBrowser) {
On 2016/08/17 13:03:55, clamy wrote:
> Could you add comments explaining what the test does? It's not clear to me
what
> it is doing there.

Done.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
On 2016/08/17 13:03:55, clamy wrote:
> Isn't this test testing that the browser navigation start is used if the
initial
> document was not accessed? If so, can we remove
> BrowserNavigationStartUsedIfInitialDocumentWasNotAccessed, which looks a lot
> more complicated?

Assuming that these tests will always start with an untouched initial doc, it's
fine to remove. Done.

> If this the case, can we also make a similar test for initial document access
> where we just access the document of the current frame?

This means relying on that test-only methods for executing script will always
trigger didAccessInitialDocument, and I'm somewhat wary about this. Currently
this test mimics the real-world cases closer. Are you sure it's ok to simplify
the test?

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2153:
FiringBeforeUnloadInSubFrameDiscardsBrowserNavigationStart) {
On 2016/08/17 13:03:55, clamy wrote:
> This doesn't seem correct to me: based on how the code is written, if we
haven't
> yet navigated anywhere we won't fire the BeforeUnload event unless the
document
> was accessed. So this similar to testing whether the initial document was
> accessed or not.

Well, now the test name does not reflect what is happening, I just forgot to
remove it.
It is passing because ExecuteJavaScriptForTests triggers
didAccessInitialDocument in the main frame.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2172: TEST_F(RenderViewImplTest,
BrowserNavigationStartNotUsedForReload) {
On 2016/08/17 13:03:55, clamy wrote:
> Can you rename it NavigationStartForReload?

Done.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2193:
common_params.navigation_start);
On 2016/08/17 13:03:55, clamy wrote:
> Can you do the following:
> if (!IsBrowserSideNavigationEnabled()) {
>   // The browser navigation_start should not be used because beforeunload  
will
> be fired during Navigate.
>   EXPECT_PRED2(...
> } else {
>   // PlzNavigate: the browser navigation_start is always used.
>   EXPECT_EQ(common_params.navigation_start, std::get<1>(nav_params));
> }
> 
> Same in the history test.

Done.

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2197:
BrowserNavigationStartNotUsedForSameProcessHistoryNavigation) {
On 2016/08/17 13:03:55, clamy wrote:
> Please rename it NavigationStartForSameProcessHistoryNavigation.

Done. Also renamed the cross-process counterpart and
BrowserNavigationStartDiscardedIfInitialDocumentWasAccessed.

[Alexander Semashko, 2016-08-19 07:47:40]

clamy, PTAL

[Alexander Semashko, 2016-08-24 09:33:35]

clamy, friendly ping

[clamy, 2016-08-24 23:23:10]

Thanks!

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
On 2016/08/17 17:10:25, Alexander Semashko wrote:
> On 2016/08/17 13:03:55, clamy wrote:
> > Isn't this test testing that the browser navigation start is used if the
> initial
> > document was not accessed? If so, can we remove
> > BrowserNavigationStartUsedIfInitialDocumentWasNotAccessed, which looks a lot
> > more complicated?
> 
> Assuming that these tests will always start with an untouched initial doc,
it's
> fine to remove. Done.
> 
> > If this the case, can we also make a similar test for initial document
access
> > where we just access the document of the current frame?
> 
> This means relying on that test-only methods for executing script will always
> trigger didAccessInitialDocument, and I'm somewhat wary about this. Currently
> this test mimics the real-world cases closer. Are you sure it's ok to simplify
> the test?

Well it was working when you just added a subframe to the testing frame. Also
your current version relies on maintaining testing code (for routing ids) that
is not used outside of that test.

https://codereview.chromium.org/2103733004/diff/200001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/200001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:268: EXPECT_TRUE(message);
Why is this needed?

[Alexander Semashko, 2016-08-25 10:44:33]

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
On 2016/08/24 23:23:09, clamy wrote:
> Well it was working when you just added a subframe to the testing frame.

Yeah, it is working currently. To elaborate more on my concern: we can replace
the current test with a simpler one, but also, I think, less reliable; if the
simplified version breaks in some future moment, there is a high chance that the
test will be disabled/deleted instead of digging and reinventing a complex setup
that makes it working.

> Also your current version relies on maintaining testing code
> (for routing ids) that is not used outside of that test.

Well, I just added setters for values that are implicitly used in many tests.
IMO it is a tiny bit that makes the API more complete and it's better to keep
these setters in any case.

To be clear, these are not strong objections in any way, just considerations;
I'll change the CL whatever way you think suitable.
Just waiting for your decision.

https://codereview.chromium.org/2103733004/diff/200001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/200001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:268: EXPECT_TRUE(message);
On 2016/08/24 23:23:09, clamy wrote:
> Why is this needed?

Simplifies debugging tests.
Earlier, if the message was not sent or was sent multiple times, this function
would just crash. With this change it produces a nice expectation failure and
does not crash :)

[clamy, 2016-08-25 18:00:15]

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
On 2016/08/25 10:44:33, Alexander Semashko wrote:
> On 2016/08/24 23:23:09, clamy wrote:
> > Well it was working when you just added a subframe to the testing frame.
> 
> Yeah, it is working currently. To elaborate more on my concern: we can replace
> the current test with a simpler one, but also, I think, less reliable; if the
> simplified version breaks in some future moment, there is a high chance that
the
> test will be disabled/deleted instead of digging and reinventing a complex
setup
> that makes it working.

Your test is on the commit queue, which means it can only breaks if someone
explicitly disables it in order to land their patch. It is against Chromium
policy to do so: we only disable tests if they are flaky (then we file bugs to
unflake them) or if what they are testing is no longer relevant. So as long as
we want our code to follow the behavior this test is testing, it should remain
unbroken. I do think that the simpler version is easier to understand, an
requires less changes to the testing code. So I would prefer for you to use this
version.

> 
> > Also your current version relies on maintaining testing code
> > (for routing ids) that is not used outside of that test.
> 
> Well, I just added setters for values that are implicitly used in many tests.
> IMO it is a tiny bit that makes the API more complete and it's better to keep
> these setters in any case.
> 
> To be clear, these are not strong objections in any way, just considerations;
> I'll change the CL whatever way you think suitable.
> Just waiting for your decision.

[Alexander Semashko, 2016-08-25 21:32:10]

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
On 2016/08/25 18:00:14, clamy wrote:
> On 2016/08/25 10:44:33, Alexander Semashko wrote:
> > On 2016/08/24 23:23:09, clamy wrote:
> > > Well it was working when you just added a subframe to the testing frame.
> > 
> > Yeah, it is working currently. To elaborate more on my concern: we can
replace
> > the current test with a simpler one, but also, I think, less reliable; if
the
> > simplified version breaks in some future moment, there is a high chance that
> the
> > test will be disabled/deleted instead of digging and reinventing a complex
> setup
> > that makes it working.
> 
> Your test is on the commit queue, which means it can only breaks if someone
> explicitly disables it in order to land their patch. It is against Chromium
> policy to do so: we only disable tests if they are flaky (then we file bugs to
> unflake them) or if what they are testing is no longer relevant. So as long as
> we want our code to follow the behavior this test is testing, it should remain
> unbroken. I do think that the simpler version is easier to understand, an
> requires less changes to the testing code. So I would prefer for you to use
this
> version.

I meant major refactorings, where it sometimes is hard to tell whether a test is
still relevant, and some mistakes just slip through... Nevermind.

Ok, this test is now as simple as it can be.
Note that the part with routing ids is still in the CL (since you did not say
anything about it).

> 
> > 
> > > Also your current version relies on maintaining testing code
> > > (for routing ids) that is not used outside of that test.
> > 
> > Well, I just added setters for values that are implicitly used in many
tests.
> > IMO it is a tiny bit that makes the API more complete and it's better to
keep
> > these setters in any case.
> > 
> > To be clear, these are not strong objections in any way, just
considerations;
> > I'll change the CL whatever way you think suitable.
> > Just waiting for your decision.
>

[clamy, 2016-08-25 22:24:29]

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
On 2016/08/25 21:32:10, Alexander Semashko wrote:
> On 2016/08/25 18:00:14, clamy wrote:
> > On 2016/08/25 10:44:33, Alexander Semashko wrote:
> > > On 2016/08/24 23:23:09, clamy wrote:
> > > > Well it was working when you just added a subframe to the testing frame.
> > > 
> > > Yeah, it is working currently. To elaborate more on my concern: we can
> replace
> > > the current test with a simpler one, but also, I think, less reliable; if
> the
> > > simplified version breaks in some future moment, there is a high chance
that
> > the
> > > test will be disabled/deleted instead of digging and reinventing a complex
> > setup
> > > that makes it working.
> > 
> > Your test is on the commit queue, which means it can only breaks if someone
> > explicitly disables it in order to land their patch. It is against Chromium
> > policy to do so: we only disable tests if they are flaky (then we file bugs
to
> > unflake them) or if what they are testing is no longer relevant. So as long
as
> > we want our code to follow the behavior this test is testing, it should
remain
> > unbroken. I do think that the simpler version is easier to understand, an
> > requires less changes to the testing code. So I would prefer for you to use
> this
> > version.
> 
> I meant major refactorings, where it sometimes is hard to tell whether a test
is
> still relevant, and some mistakes just slip through... Nevermind.
> 
> Ok, this test is now as simple as it can be.
> Note that the part with routing ids is still in the CL (since you did not say
> anything about it).
> 
> > 
> > > 
> > > > Also your current version relies on maintaining testing code
> > > > (for routing ids) that is not used outside of that test.
> > > 
> > > Well, I just added setters for values that are implicitly used in many
> tests.
> > > IMO it is a tiny bit that makes the API more complete and it's better to
> keep
> > > these setters in any case.
> > > 
> > > To be clear, these are not strong objections in any way, just
> considerations;
> > > I'll change the CL whatever way you think suitable.
> > > Just waiting for your decision.
> > 
> 

If the routing id part is not used in the tests, then it should be removed from
the CL.

[Alexander Semashko, 2016-08-25 22:50:20]

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2103733004/diff/180001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2061: TEST_F(RenderViewImplTest,
BrowserNavigationStart) {
On 2016/08/25 22:24:29, clamy wrote:
> On 2016/08/25 21:32:10, Alexander Semashko wrote:
> > On 2016/08/25 18:00:14, clamy wrote:
> > > On 2016/08/25 10:44:33, Alexander Semashko wrote:
> > > > On 2016/08/24 23:23:09, clamy wrote:
> > > > > Well it was working when you just added a subframe to the testing
frame.
> > > > 
> > > > Yeah, it is working currently. To elaborate more on my concern: we can
> > replace
> > > > the current test with a simpler one, but also, I think, less reliable;
if
> > the
> > > > simplified version breaks in some future moment, there is a high chance
> that
> > > the
> > > > test will be disabled/deleted instead of digging and reinventing a
complex
> > > setup
> > > > that makes it working.
> > > 
> > > Your test is on the commit queue, which means it can only breaks if
someone
> > > explicitly disables it in order to land their patch. It is against
Chromium
> > > policy to do so: we only disable tests if they are flaky (then we file
bugs
> to
> > > unflake them) or if what they are testing is no longer relevant. So as
long
> as
> > > we want our code to follow the behavior this test is testing, it should
> remain
> > > unbroken. I do think that the simpler version is easier to understand, an
> > > requires less changes to the testing code. So I would prefer for you to
use
> > this
> > > version.
> > 
> > I meant major refactorings, where it sometimes is hard to tell whether a
test
> is
> > still relevant, and some mistakes just slip through... Nevermind.
> > 
> > Ok, this test is now as simple as it can be.
> > Note that the part with routing ids is still in the CL (since you did not
say
> > anything about it).
> > 
> > > 
> > > > 
> > > > > Also your current version relies on maintaining testing code
> > > > > (for routing ids) that is not used outside of that test.
> > > > 
> > > > Well, I just added setters for values that are implicitly used in many
> > tests.
> > > > IMO it is a tiny bit that makes the API more complete and it's better to
> > keep
> > > > these setters in any case.
> > > > 
> > > > To be clear, these are not strong objections in any way, just
> > considerations;
> > > > I'll change the CL whatever way you think suitable.
> > > > Just waiting for your decision.
> > > 
> > 
> 
> If the routing id part is not used in the tests, then it should be removed
from
> the CL.

Done.

[clamy, 2016-08-25 23:11:01]

Lgtm. Thanks for sticking through the long review process!

[Alexander Semashko, 2016-08-25 23:43:59]

On 2016/08/25 23:11:01, clamy wrote:
> Lgtm. Thanks for sticking through the long review process!

Cool, thanks for the review!

[Alexander Semashko, 2016-08-25 23:47:00]

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

[Alexander Semashko, 2016-08-25 23:47:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2103733004/#ps260001
(title: "Remove routing id related changes.")

[commit-bot: I haz the power, 2016-08-25 23:47:39]

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-26 00:52:34]

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

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

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

[Alexander Semashko, 2016-08-29 16:18:20]

I had to change timeline-dispatchEvent a bit, since it was expecting that adding
frame produces beforeunload and unload events. Other failures seem totally
irrelevant.

[Alexander Semashko, 2016-08-29 16:19:02]

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

[Alexander Semashko, 2016-08-29 16:19:03]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dcheng@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2103733004/#ps280001
(title: "Update test.")

[commit-bot: I haz the power, 2016-08-29 16:22:06]

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-29 17:58:09]

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

[commit-bot: I haz the power, 2016-08-29 17:58:11]

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, 2016-08-29 19:24:40]

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

[commit-bot: I haz the power, 2016-08-29 19:31:30]

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-30 03:17:03]

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

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

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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Semashko, 2016-08-30 09:42:59]

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

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

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-30 11:42:21]

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

[commit-bot: I haz the power, 2016-08-30 11:42:24]

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, 2016-08-30 12:21:57]

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

[commit-bot: I haz the power, 2016-08-30 12:22:33]

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-30 15:18:22]

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

[commit-bot: I haz the power, 2016-08-30 15:18:24]

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, 2016-08-30 16:35:14]

I saw similar failures in a couple of other CLs, so was thinking it's just
flaking.
However, when examining the logs, I saw the following. The bot tries to run a
new test without the patch. Then it says that output.json is missing and quits,
saying that there is an exception.
This is clearly a bug, but I don't know what to do with it. Can somebody please
help me?

[clamy, 2016-08-30 18:40:36]

It seems you had an issue with the PlzNavigate run. Now that it fixed, I'll try
resending your patch to CQ. What happened with the bot is bit weird, but if you
don't have any test failures this step should be skipped.

[clamy, 2016-08-30 18:40:47]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-08-30 18:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dcheng@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2103733004/#ps320001
(title: "Account for PlzNavigate.")

[commit-bot: I haz the power, 2016-08-30 18:41:25]

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

[Alexander Semashko, 2016-08-30 18:51:18]

On 2016/08/30 18:40:36, clamy wrote:
> It seems you had an issue with the PlzNavigate run. Now that it fixed, I'll
try
> resending your patch to CQ. What happened with the bot is bit weird, but if
you
> don't have any test failures this step should be skipped.

Yeah, a test was failing. It was hard to notice because build results do not
mention "normal" test failures in presence of such an exception.

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

Description was changed from

==========
Set navigationStart correctly for all load types.

Currently for browser-initiated navigations the navigationStart recorded in the
browser process is discarded for "non-standard" load types, e.g. history
navigations and navigations with should_replace_current_entry flag set. As per
the comments in code, this is done to prevent setting navigationStart before
prompting to unload the previous document. Sadly it is quite wrong and only
causes inconsistency in the meaning of navigationStart (what it means depends on
load type, though the spec makes no distinctions between them). And still in
same-process navigations with WebFrameLoadType::Standard we take the
browser-provided navigationStart, then fire beforeunload (probably waiting for
user action on the dialog) and keep the old timestamp as is.

In this CL navigationStart is reset to Now() after calling a beforeunload
handler. On the other hand, it is not reset for load types other than standard,
if beforeunload have been called earlier or the frame being navigated contains
just an untouched initial document (guaranteed to be empty).
==========

to

==========
Set navigationStart correctly for all load types.

Currently for browser-initiated navigations the navigationStart recorded in the
browser process is discarded for "non-standard" load types, e.g. history
navigations and navigations with should_replace_current_entry flag set. As per
the comments in code, this is done to prevent setting navigationStart before
prompting to unload the previous document. Sadly it is quite wrong and only
causes inconsistency in the meaning of navigationStart (what it means depends on
load type, though the spec makes no distinctions between them). And still in
same-process navigations with WebFrameLoadType::Standard we take the
browser-provided navigationStart, then fire beforeunload (probably waiting for
user action on the dialog) and keep the old timestamp as is.

In this CL navigationStart is reset to Now() after calling a beforeunload
handler. On the other hand, it is not reset for load types other than standard,
if beforeunload have been called earlier or the frame being navigated contains
just an untouched initial document (guaranteed to be empty).
==========

[commit-bot: I haz the power, 2016-08-30 21:26:12]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-08-30 21:28:10]

Description was changed from

==========
Set navigationStart correctly for all load types.

Currently for browser-initiated navigations the navigationStart recorded in the
browser process is discarded for "non-standard" load types, e.g. history
navigations and navigations with should_replace_current_entry flag set. As per
the comments in code, this is done to prevent setting navigationStart before
prompting to unload the previous document. Sadly it is quite wrong and only
causes inconsistency in the meaning of navigationStart (what it means depends on
load type, though the spec makes no distinctions between them). And still in
same-process navigations with WebFrameLoadType::Standard we take the
browser-provided navigationStart, then fire beforeunload (probably waiting for
user action on the dialog) and keep the old timestamp as is.

In this CL navigationStart is reset to Now() after calling a beforeunload
handler. On the other hand, it is not reset for load types other than standard,
if beforeunload have been called earlier or the frame being navigated contains
just an untouched initial document (guaranteed to be empty).
==========

to

==========
Set navigationStart correctly for all load types.

Currently for browser-initiated navigations the navigationStart recorded in the
browser process is discarded for "non-standard" load types, e.g. history
navigations and navigations with should_replace_current_entry flag set. As per
the comments in code, this is done to prevent setting navigationStart before
prompting to unload the previous document. Sadly it is quite wrong and only
causes inconsistency in the meaning of navigationStart (what it means depends on
load type, though the spec makes no distinctions between them). And still in
same-process navigations with WebFrameLoadType::Standard we take the
browser-provided navigationStart, then fire beforeunload (probably waiting for
user action on the dialog) and keep the old timestamp as is.

In this CL navigationStart is reset to Now() after calling a beforeunload
handler. On the other hand, it is not reset for load types other than standard,
if beforeunload have been called earlier or the frame being navigated contains
just an untouched initial document (guaranteed to be empty).

Committed: https://crrev.com/eacad60524fdf286eccc22ebc160385cda2ad99c
Cr-Commit-Position: refs/heads/master@{#415431}
==========

[commit-bot: I haz the power, 2016-08-30 21:28:12]

Patchset 17 (id:??) landed as
https://crrev.com/eacad60524fdf286eccc22ebc160385cda2ad99c
Cr-Commit-Position: refs/heads/master@{#415431}