Avoid too many title changes in history

Pages can use Javascript to update the title, which is an event that,
from history's perspective, we added support for quite recently in
https://chromium-review.googlesource.com/847697

We're mostly interested during page load, and the first few seconds
after loading has completed (just in case). However, some pages stay for
very long (even indefinitely) in loading state, so we should guard
against too many updates hammering history, which as side effect can
trigger sync.

We cap the max number of title updates per navigation, i.e. with a
shared counter for the full timespan during which title updates are
propagated to history (loading state and the few seconds after loaded).

Bug: 843057
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I3eb8c4157b951ea2aae941510719256bd26491d7
Reviewed-on: https://chromium-review.googlesource.com/1060055
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561001}

chrome/browser/history/history_tab_helper.cc

@@ -113,6 +113,7 @@ void HistoryTabHelper::DidFinishNavigation(
 
   if (navigation_handle->IsInMainFrame()) {
     is_loading_ = true;
+    num_title_changes_ = 0;
   } else if (!navigation_handle->HasSubframeNavigationEntryCommitted()) {
     // Filter out unwanted URLs. We don't add auto-subframe URLs that don't
     // change which NavigationEntry is current. They are a large part of history
@@ -182,6 +183,10 @@ void HistoryTabHelper::TitleWasSet(NavigationEntry* entry) {
   if (!entry)
     return;
 
+  // Protect against pages changing their title too often.
+  if (num_title_changes_ >= history::kMaxTitleChanges)
+    return;
+
   // Only store page titles into history if they were set while the page was
   // loading or during a brief span after load is complete. This fixes the case
   // where a page uses a title change to alert a user of a situation but that
@@ -189,8 +194,10 @@ void HistoryTabHelper::TitleWasSet(NavigationEntry* entry) {
   if (is_loading_ || (base::TimeTicks::Now() - last_load_completion_ <
                       history::GetTitleSettingWindow())) {
     history::HistoryService* hs = GetHistoryService();
-    if (hs)
+    if (hs) {
       hs->SetPageTitle(entry->GetVirtualURL(), entry->GetTitleForDisplay());
+      ++num_title_changes_;
+    }
   }
 }
 

chrome/browser/history/history_tab_helper.h

@@ -53,6 +53,9 @@ class HistoryTabHelper : public content::WebContentsObserver,
   // loading. Only applies to the main frame of the page.
   bool is_loading_ = false;
 
+  // Number of title changes since the loading of the navigation started.
+  int num_title_changes_ = 0;
+
   // The time that the current page finished loading. Only title changes within
   // a certain time period after the page load is complete will be saved to the
   // history system. Only applies to the main frame of the page.

chrome/browser/history/history_tab_helper_unittest.cc

+// Copyright 2018 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 "chrome/browser/history/history_tab_helper.h"
+
+#include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/task/cancelable_task_tracker.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/history/core/browser/history_constants.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/history/core/browser/url_row.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/web_contents_tester.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class HistoryTabHelperTest : public ChromeRenderViewHostTestHarness {
+ protected:
+  HistoryTabHelperTest() {}
+
+  // ChromeRenderViewHostTestHarness:
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+    ASSERT_TRUE(profile()->CreateHistoryService(/*delete_file=*/false,
+                                                /*no_db=*/false));
+    history_service_ = HistoryServiceFactory::GetForProfile(
+        profile(), ServiceAccessType::IMPLICIT_ACCESS);
+    ASSERT_TRUE(history_service_);
+    history_service_->AddPage(
+        page_url_, base::Time::Now(), /*context_id=*/nullptr,
+        /*nav_entry_id=*/0,
+        /*referrer=*/GURL(), history::RedirectList(), ui::PAGE_TRANSITION_TYPED,
+        history::SOURCE_BROWSED, /*did_replace_entry=*/false);
+    HistoryTabHelper::CreateForWebContents(web_contents());
+  }
+
+  HistoryTabHelper* history_tab_helper() {
+    return HistoryTabHelper::FromWebContents(web_contents());
+  }
+
+  content::WebContentsTester* web_contents_tester() {
+    return content::WebContentsTester::For(web_contents());
+  }
+
+  std::string QueryPageTitleFromHistory(const GURL& url) {
+    base::string16 title;
+    base::RunLoop loop;
+    history_service_->QueryURL(
+        url, /*want_visits=*/false,
+        base::BindLambdaForTesting([&](bool success, const history::URLRow& row,
+                                       const history::VisitVector&) {
+          EXPECT_TRUE(success);
+          title = row.title();
+          loop.Quit();
+        }),
+        &tracker_);
+    loop.Run();
+    return base::UTF16ToUTF8(title);
+  }
+
+  const GURL page_url_ = GURL("http://foo.com");
+
+ private:
+  base::CancelableTaskTracker tracker_;
+  history::HistoryService* history_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryTabHelperTest);
+};
+
+TEST_F(HistoryTabHelperTest, ShouldUpdateTitleInHistory) {
+  web_contents_tester()->NavigateAndCommit(page_url_);
+
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetActiveEntry();
+  ASSERT_NE(nullptr, entry);
+  ASSERT_TRUE(web_contents()->IsLoading());
+
+  web_contents()->UpdateTitleForEntry(entry, base::UTF8ToUTF16("title1"));
+  EXPECT_EQ("title1", QueryPageTitleFromHistory(page_url_));
+}
+
+TEST_F(HistoryTabHelperTest, ShouldLimitTitleUpdatesPerPage) {
+  web_contents_tester()->NavigateAndCommit(page_url_);
+
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetActiveEntry();
+  ASSERT_NE(nullptr, entry);
+  ASSERT_TRUE(web_contents()->IsLoading());
+
+  // The first 10 title updates are accepted and update history, as per
+  // history::kMaxTitleChanges.
+  for (int i = 1; i <= history::kMaxTitleChanges; ++i) {
+    const std::string title = base::StringPrintf("title%d", i);
+    web_contents()->UpdateTitleForEntry(entry, base::UTF8ToUTF16(title));
+  }
+
+  ASSERT_EQ("title10", QueryPageTitleFromHistory(page_url_));
+
+  // Furhter updates should be ignored.
+  web_contents()->UpdateTitleForEntry(entry, base::UTF8ToUTF16("title11"));
+  EXPECT_EQ("title10", QueryPageTitleFromHistory(page_url_));
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -2368,6 +2368,7 @@ test("unit_tests") {
     "../browser/history/android/sqlite_cursor_unittest.cc",
     "../browser/history/android/urls_sql_handler_unittest.cc",
     "../browser/history/android/visit_sql_handler_unittest.cc",
+    "../browser/history/history_tab_helper_unittest.cc",
     "../browser/infobars/mock_infobar_service.cc",
     "../browser/infobars/mock_infobar_service.h",
     "../browser/install_verification/win/loaded_module_verification_unittest.cc",

components/history/core/browser/history_constants.cc

@@ -18,6 +18,8 @@ const base::FilePath::CharType kTopSitesFilename[] =
 
 const int kMaxTopHosts = 50;
 
+const int kMaxTitleChanges = 10;
+
 base::TimeDelta GetTitleSettingWindow() {
   const auto value = base::TimeDelta::FromSeconds(5);
   return value;

components/history/core/browser/history_constants.h

@@ -18,6 +18,11 @@ extern const base::FilePath::CharType kTopSitesFilename[];
 // The maximum size of the list returned by history::HistoryService::TopHosts().
 extern const int kMaxTopHosts;
 
+// The maximum number of times a page can change it's title during the relevant
+// timestamp (page is either loading is has recently loaded as per
+// GetTitleSettingWindow() below).
+extern const int kMaxTitleChanges;
+
 // The span of time after load is complete during which a page may set its title
 // and have the title change be saved in history.
 base::TimeDelta GetTitleSettingWindow();

ios/chrome/browser/history/history_tab_helper.h

@@ -75,6 +75,9 @@ class HistoryTabHelper : public history::Context,
   // is set to false.
   bool delay_notification_ = false;
 
+  // Number of title changes since the loading of the navigation started.
+  int num_title_changes_;
+
   // The time that the current page finished loading. Only title changes within
   // a certain time period after the page load is complete will be saved to the
   // history system. Only applies to the main frame of the page.

ios/chrome/browser/history/history_tab_helper.mm

@@ -125,6 +125,8 @@ void HistoryTabHelper::DidFinishNavigation(
   }
 #endif
 
+  num_title_changes_ = 0;
+
   history::RedirectList redirects;
   const GURL& original_url = visible_item->GetOriginalRequestURL();
   const GURL& referrer_url = visible_item->GetReferrer().url;
@@ -189,6 +191,10 @@ void HistoryTabHelper::TitleWasSet(web::WebState* web_state) {
     return;
   }
 
+  // Protect against pages changing their title too often during page load.
+  if (num_title_changes_ >= history::kMaxTitleChanges)
+    return;
+
   // Only store page titles into history if they were set while the page was
   // loading or during a brief span after load is complete. This fixes the case
   // where a page uses a title change to alert a user of a situation but that
@@ -198,8 +204,10 @@ void HistoryTabHelper::TitleWasSet(web::WebState* web_state) {
        history::GetTitleSettingWindow())) {
     web::NavigationItem* last_committed_item =
         web_state_->GetNavigationManager()->GetLastCommittedItem();
-    if (last_committed_item)
+    if (last_committed_item) {
       UpdateHistoryPageTitle(*last_committed_item);
+      ++num_title_changes_;
+    }
   }
 }