NonValidatingReload: Monitor reload operations in NavigationControllerImpl

content/browser/frame_host/navigation_controller_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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 208 matching lines @@
       pending_entry_index_(-1),
       transient_entry_index_(-1),
       delegate_(delegate),
       max_restored_page_id_(-1),
       ssl_manager_(this),
       needs_reload_(false),
       is_initial_navigation_(true),
       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_);
 }
 
 NavigationControllerImpl::~NavigationControllerImpl() {
   DiscardNonCommittedEntriesInternal();
 }
 
 WebContents* NavigationControllerImpl::GetWebContents() const {
   return delegate_->GetWebContents();
 }
@@ skipping 82 matching lines @@
     if (current_index != -1) {
       entry = GetEntryAtIndex(current_index);
     }
   }
 
   // If we are no where, then we can't reload.  TODO(darin): We should add a
   // CanReload method.
   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
     // they really want to do this. If they do, the dialog will call us back
     // with check_for_repost = false.
     delegate_->NotifyBeforeFormRepostWarningShow();
 
     pending_reload_ = reload_type;
     delegate_->ActivateAndShowRepostFormWarningDialog();
   } else {
@@ skipping 453 matching lines @@
   // The renderer tells us whether the navigation replaces the current entry.
   details->did_replace_entry = params.should_replace_current_entry;
 
   // Do navigation-type specific actions. These will make and commit an entry.
   details->type = ClassifyNavigation(rfh, params);
 
   // is_in_page must be computed before the entry gets committed.
   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,
                                    details->did_replace_entry);
       break;
     case NAVIGATION_TYPE_EXISTING_PAGE:
       details->did_replace_entry = details->is_in_page;
       RendererDidNavigateToExistingPage(rfh, params, details->is_in_page);
       break;
     case NAVIGATION_TYPE_SAME_PAGE:
@@ skipping 1286 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content