NonValidatingReload: Monitor reload operations in NavigationControllerImpl

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 4b1ba60e089679a1a873c195644e4dc35bdfdf7c..8ed3ece31beadd32b3809b05a7ca8b1b0de8a149 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -226,7 +226,8 @@ NavigationControllerImpl::NavigationControllerImpl(
       in_navigate_to_pending_entry_(false),
       pending_reload_(ReloadType::NONE),
       get_timestamp_callback_(base::Bind(&base::Time::Now)),
-      screenshot_manager_(new NavigationEntryScreenshotManager(this)) {
+      screenshot_manager_(new NavigationEntryScreenshotManager(this)),
+      last_committed_reload_type_(ReloadType::NONE) {
   DCHECK(browser_context_);
 }
 
@@ -329,6 +330,22 @@ void NavigationControllerImpl::ReloadInternal(bool check_for_repost,
   if (!entry)
     return;
 
+  // Check if previous navigation was reload to track reload operations.

[Charlie Reis, 2016/09/19 22:23:51]

nit: was a reload, to track consecutive reload operations.

[Takashi Toyoshima, 2016/09/26 12:23:34]

Done.

+  if (last_committed_reload_type_ != ReloadType::NONE) {
+    base::Time now =
+      time_smoother_.GetSmoothedTime(get_timestamp_callback_.Run());

[Charlie Reis, 2016/09/19 22:23:51]

nit: Wrong indent.  git cl format can take care of that.  :)

[Takashi Toyoshima, 2016/09/26 12:23:34]

Oops. Sorry, I forgot to run it for this patch set.
Done.

+    base::TimeDelta delta = now - last_committed_reload_time_;

[Charlie Reis, 2016/09/19 22:23:51]

Maybe include a sanity check that last_committed_reload_time_ is valid and
earlier than now, and skip sending the UMA if not?

[Takashi Toyoshima, 2016/09/26 12:23:34]

Done.

+    UMA_HISTOGRAM_MEDIUM_TIMES("Navigation.Reload.ReloadToReloadDuration",
+                               delta);
+    if (last_committed_reload_type_ == ReloadType::MAIN_RESOURCE) {
+      UMA_HISTOGRAM_MEDIUM_TIMES(
+          "Navigation.Reload.ReloadMainResourceToReloadDuration", delta);
+    }
+  }
+
+  // Set ReloadType for |entry| in order to check it at commit time.
+  entry->set_reload_type(reload_type);

[Charlie Reis, 2016/09/19 22:23:51]

Side note: While debugging I discovered (much to my surprise) that we don't get
here for reloads if DevTools is open.  It appears ReloadInternal in
browser_commands.cc is to blame, where there's code to intercept reloads and
send them to DevTools's ReloadInspectedWebContents.  Hopefully it's ok to miss
out on data in that case.

[Takashi Toyoshima, 2016/09/26 12:23:34]

Yeah, I noticed this problem too. I think it's fine to ignore such cases since
it means users are doing something special that beyond end general users
use-cases, and that would be noise for my purpose. I imagine a case there
developers take many reload operations to debug their sites.

+
   if (g_check_for_repost && check_for_repost &&
       entry->GetHasPostData()) {
     // The user is asking to reload a page with POST data. Prompt to make sure
@@ -802,6 +819,27 @@ bool NavigationControllerImpl::RendererDidNavigate(
   details->is_in_page = IsURLInPageNavigation(params.url, params.origin,
                                               params.was_within_same_page, rfh);
 
+  // Update reload information to track user initiated reload operations for the
+  // same content.
+  if (pending_entry_ && pending_entry_->GetUniqueID() == params.nav_entry_id) {

[Charlie Reis, 2016/09/19 22:23:51]

I would suggest using PendingEntryMatchesHandle instead.  (Sadly, that alone
won't fix https://crbug.com/646730.)

This also needs to check whether it's a reload (either using
pending_entry_->reload_type(), params.transition, or both).  Otherwise we'll set
the reload type/time fields for other browser-initiated navigations with pending
entries, like omnibox navigations and back/forward.

[Takashi Toyoshima, 2016/09/26 12:23:34]

Done. Is my fix correct? I use rfh->navigation_handle() to obtain the Handle for
PendingEntryMatchesHandle.

For reload type check, I prefer to check only reload_type() but do you have any
concern about this? If we want to check ui::PageTransition, the check could be a
little complicated since it could have both PAGE_TRANSITION_FORWARD_BACK and
PAGE_TRANSITION_RELOAD when history navigation happens against a reloaded page.

Also, I assumed reload_type() always returns NONE for other browser-initiated
navigations. But now it sounds better to check it here and reset
last_committed_reload_time_ inside 'else if (){ ... }'.

[Charlie Reis, 2016/09/26 23:34:39]

Yes, that looks right to me, thanks.
ReloadType should be fine.  (For reference, you should be able to use
PageTransitionCoreTypeIs to separate PAGE_TRANSITION_RELOAD from
PAGE_TRANSITION_FORWARD_BACK, but I don't see a need for it here.)

+    // If navigation was initiated by |pending_entry_|, save reload type and
+    // timestamp to check when the next reload is requested.
+    last_committed_reload_type_ = pending_entry_->reload_type();
+    last_committed_reload_time_ = pending_entry_->GetTimestamp();

[Charlie Reis, 2016/09/19 22:23:51]

This won't be the right timestamp, because it doesn't get assigned until later
in this method, on line 905.

I can see that you might not want to move this whole block down there, since
that's after the pending entry has been cleared/committed.  Maybe we can
generate our own timestamp here (despite being a bit redundant)?

[Takashi Toyoshima, 2016/09/26 12:23:34]

Done.

+  } else if ((details->type == NAVIGATION_TYPE_EXISTING_PAGE &&
+              params.gesture == NavigationGestureUser) ||
+             (details->type != NAVIGATION_TYPE_EXISTING_PAGE &&
+              details->type != NAVIGATION_TYPE_AUTO_SUBFRAME)) {

[Charlie Reis, 2016/09/19 22:23:51]

This doesn't look like the right condition.

We want to reset the state if the navigation is browser-initiated
(!pending_entry_->is_renderer_initiated()) or if it has a user gesture,
regardless of what details->type is.  Again, we'll want to be sure to use
PendingEntryMatchesHandle.

[Takashi Toyoshima, 2016/09/26 12:23:34]

Done.

+    // Same page navigation initiated by user action, e.g. clicking an anchor to
+    // the same URL, should reset the reload information as we do for other
+    // user initiated navigation to a different page.
+    // But we do not reset if the same page navigation was initiated without any
+    // user action. For instance, location.reload() should not reset it.
+    // We also ignore subframe navigations automatically performed inside the
+    // page.
+    last_committed_reload_type_ = ReloadType::NONE;

[Charlie Reis, 2016/09/19 22:23:51]

Also clear the timestamp, to avoid having old data there.

[Takashi Toyoshima, 2016/09/26 12:23:33]

Done.

+  }
+
   switch (details->type) {
     case NAVIGATION_TYPE_NEW_PAGE:
       RendererDidNavigateToNewPage(rfh, params, details->is_in_page,