Set navigationStart correctly for all load types.

content/renderer/render_frame_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 547 matching lines @@
 // Sanitizes the navigation_start timestamp for browser-initiated navigations,
 // where the browser possibly has a better notion of start time than the
 // renderer. In the case of cross-process navigations, this carries over the
 // time of finishing the onbeforeunload handler of the previous page.
 // TimeTicks is sometimes not monotonic across processes, and because
 // |browser_navigation_start| is likely before this process existed,
 // InterProcessTimeTicksConverter won't help. The timestamp is sanitized by
 // 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);
   base::TimeDelta difference =
       renderer_navigation_start - browser_navigation_start;
   if (difference > base::TimeDelta()) {
     UMA_HISTOGRAM_TIMES("Navigation.Start.RendererBrowserDifference.Positive",
                         difference);
   } else {
     UMA_HISTOGRAM_TIMES("Navigation.Start.RendererBrowserDifference.Negative",
@@ skipping 3080 matching lines @@
             frame->dataSource()->request().inputPerfMetricReportPolicy());
     base::TimeTicks ui_timestamp = base::TimeTicks() +
         base::TimeDelta::FromSecondsD(
             frame->dataSource()->request().uiStartTime());
 
     Send(new FrameHostMsg_DocumentOnLoadCompleted(
         routing_id_, report_type, ui_timestamp));
   }
 }
 
+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) {
   TRACE_EVENT1("navigation", "RenderFrameImpl::didFailLoad",
                "id", routing_id_);
   DCHECK_EQ(frame_, frame);
   // TODO(nasko): Move implementation here. No state needed.
   WebDataSource* ds = frame->dataSource();
   DCHECK(ds);
 
@@ skipping 1778 matching lines @@
   }
 
   // If the navigation is for "view source", the WebLocalFrame needs to be put
   // in a special mode.
   if (request_params.is_view_source)
     frame_->enableViewSourceMode(true);
 
   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.
   blink::WebFrameLoadType load_type =
       common_params.should_replace_current_entry
           ? blink::WebFrameLoadType::ReplaceCurrentItem
           : blink::WebFrameLoadType::Standard;
   blink::WebHistoryLoadType history_load_type =
       blink::WebHistoryDifferentDocumentLoad;
   bool should_load_request = false;
@@ skipping 103 matching lines @@
           GetWebHTTPBodyForRequestBody(common_params.post_data));
     }
 
     // A session history navigation should have been accompanied by state.
     CHECK_EQ(request_params.page_id, -1);
 
     should_load_request = true;
   }
 
   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 =
         browser_side_navigation
             ? !!(common_params.transition & ui::PAGE_TRANSITION_CLIENT_REDIRECT)
             : false;
 
     // Perform a navigation to a data url if needed.
     // Note: the base URL might be invalid, so also check the data URL string.
     bool should_load_data_url = !common_params.base_url_for_data_url.is_empty();
@@ skipping 418 matching lines @@
   // If this was a browser-initiated navigation, then there could be pending
   // navigation params, so use them. Otherwise, just reset the document state
   // here, since if pending navigation params exist they are for some other
   // navigation <https://crbug.com/597239>.
   if (!pending_navigation_params_ || content_initiated) {
     document_state->set_navigation_state(
         NavigationStateImpl::CreateContentInitiated());
     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
   // an in-page navigation, so skip updating it from the in-page navigation
   // params in this case.
   if (!was_within_same_page) {
     const CommonNavigationParams& common_params =
         pending_navigation_params_->common_params;
     bool load_data = !common_params.base_url_for_data_url.is_empty() &&
                      !common_params.history_url_for_data_url.is_empty() &&
@@ skipping 250 matching lines @@
   // event target. Potentially a Pepper plugin will receive the event.
   // In order to tell whether a plugin gets the last mouse event and which it
   // is, we set |pepper_last_mouse_event_target_| to null here. If a plugin gets
   // the event, it will notify us via DidReceiveMouseEvent() and set itself as
   // |pepper_last_mouse_event_target_|.
   pepper_last_mouse_event_target_ = nullptr;
 #endif
 }
 
 }  // namespace content