Set navigationStart correctly for all load types.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 4167222969def20c760b86eb6e739add6fd9986a..09c5e0d42a29debcf7b7e7064c6d52c68fe1660b 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -565,11 +565,8 @@ WebURLRequest CreateURLRequestForNavigation(
 // clamping it to renderer_navigation_start, initialized earlier in the call
 // stack.
 base::TimeTicks SanitizeNavigationTiming(
-    blink::WebFrameLoadType load_type,
     const base::TimeTicks& browser_navigation_start,
     const base::TimeTicks& renderer_navigation_start) {
-  if (load_type != blink::WebFrameLoadType::Standard)
-    return base::TimeTicks();
   DCHECK(!browser_navigation_start.is_null());
   base::TimeTicks navigation_start =
       std::min(browser_navigation_start, renderer_navigation_start);
@@ -3670,6 +3667,21 @@ void RenderFrameImpl::didHandleOnloadEvents(blink::WebLocalFrame* frame) {
   }
 }
 
+void RenderFrameImpl::didHandleOnBeforeUnloadEvent(bool eventListenerCalled) {

[clamy, 2016/06/29 11:15:26]

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:09]

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.
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]

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.
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]

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.
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.
I'll check transfers. Can you provide some pointers where to look for these
suspended history navs?

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

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.
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.
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]

There is always an initial blank document created when the frame itself is
created.
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)
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.
Yeah, in this case the first navigation's start timestamp won't be modified.
Thanks!

+  // If there are pending navigation params, then the beforeunload event was
+  // caused by a same-process browser-initiated navigation, and the
+  // navigation_start was recorded before dispatching beforeunload, which means
+  // it should not be used, and the right timestamp is |Now|.
+  // If this is the first navigation in the frame, and the initial document did
+  // not have a beforeunload handler, then it is likely a cross-process
+  // navigation, and the browser timestamp is the right one.
+  if (pending_navigation_params_ &&
+      (eventListenerCalled || !current_history_item_.isNull())) {
+    pending_navigation_params_->common_params.navigation_start =
+        base::TimeTicks::Now();
+  }
+}
+
 void RenderFrameImpl::didFailLoad(blink::WebLocalFrame* frame,
                                   const blink::WebURLError& error,
                                   blink::WebHistoryCommitType commit_type) {
@@ -5468,13 +5480,11 @@ void RenderFrameImpl::NavigateInternal(
   pending_navigation_params_.reset(
       new NavigationParams(common_params, start_params, request_params));
 
-  // Unless the load is a WebFrameLoadType::Standard, this should remain
-  // uninitialized. It will be updated when the load type is determined to be
-  // Standard, or after the previous document's unload handler has been
-  // triggered. This occurs in UpdateNavigationState.
-  // TODO(csharrison) See if we can always use the browser timestamp.
+  // Sanitize navigation start and store in |pending_navigation_params_|.
+  // It will be picked up in UpdateNavigationState.
   pending_navigation_params_->common_params.navigation_start =
-      base::TimeTicks();
+      SanitizeNavigationTiming(common_params.navigation_start,
+                               renderer_navigation_start);
 
   // Create parameters for a standard navigation, indicating whether it should
   // replace the current NavigationEntry.
@@ -5598,11 +5608,6 @@ void RenderFrameImpl::NavigateInternal(
   }
 
   if (should_load_request) {
-    // Sanitize navigation start now that we know the load_type.
-    pending_navigation_params_->common_params.navigation_start =
-        SanitizeNavigationTiming(load_type, common_params.navigation_start,
-                                 renderer_navigation_start);
-
     // PlzNavigate: check if the navigation being committed originated as a
     // client redirect.
     bool is_client_redirect =
@@ -6041,12 +6046,7 @@ void RenderFrameImpl::UpdateNavigationState(DocumentState* document_state,
     return;
   }
 
-  // If this is a browser-initiated load that doesn't override
-  // navigation_start, set it here.
-  if (pending_navigation_params_->common_params.navigation_start.is_null()) {
-    pending_navigation_params_->common_params.navigation_start =
-        base::TimeTicks::Now();
-  }
+  DCHECK(!pending_navigation_params_->common_params.navigation_start.is_null());
   document_state->set_navigation_state(CreateNavigationStateFromPending());
 
   // The |set_was_load_data_with_base_url_request| state should not change for