Cancel the next 10 updates if there's a timeout in jumplist updater

chrome/browser/win/jumplist.cc

 // Copyright 2012 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/win/jumplist.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 46 matching lines @@
 #include "ui/gfx/image/image_family.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_rep.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
 using JumpListData = JumpList::JumpListData;
 
 namespace {
 
+// The maximum allowed number of updates that can be cancelled intentionally due
+// to one jumplist updater timeout.
+constexpr int kCancelledUpdates = 10;

[gab, 2017/05/11 14:48:24]

Feels like we should delay for a period of time, e.g. a few minutes, instead of
a count? i.e. simply use a larger timeout for batch updates?

[chengx, 2017/05/11 16:41:43]

I did think about delaying for a period of time. But eventually I still prefer
using a count.

As we need to reduce the update frequency of the Jumplist for this small set of
Chrome users, using a count makes this frequency reduction certain and the same
for all the users in that set independent of time.

Btw, we already have a "delay and coalesce" strategy where all tabs closed
within 3.5 seconds are collapsed into 1 jumplist update. Maybe I can land this
CL to see how it performs, and try another CL with your idea later on. 

+
 // The delay before updating the JumpList to prevent update storms.
 constexpr base::TimeDelta kDelayForJumplistUpdate =
     base::TimeDelta::FromMilliseconds(3500);
 
 // The maximum allowed time for JumpListUpdater::BeginUpdate. Updates taking
 // longer than this are discarded to prevent bogging down slow machines.
-constexpr base::TimeDelta kTimeOutForJumplistUpdate =
+constexpr base::TimeDelta kTimeOutForJumplistBeginUpdate =
     base::TimeDelta::FromMilliseconds(500);
 
+// The maximum allowed time for updating both "most visited" and "recently
+// closed" categories via JumpListUpdater::AddCustomCategory.
+constexpr base::TimeDelta kTimeOutForAddCustomCategory =
+    base::TimeDelta::FromMilliseconds(500);
+
+// The maximum allowed time for JumpListUpdater::CommitUpdate.
+constexpr base::TimeDelta kTimeOutForJumplistCommitUpdate =
+    base::TimeDelta::FromMilliseconds(1000);
+
 // Appends the common switches to each shell link.
 void AppendCommonSwitches(ShellLinkItem* shell_link) {
   const char* kSwitchNames[] = { switches::kUserDataDir };
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   shell_link->GetCommandLine()->CopySwitchesFrom(command_line,
                                                  kSwitchNames,
                                                  arraysize(kSwitchNames));
 }
 
@@ skipping 97 matching lines @@
 }  // namespace
 
 JumpList::JumpListData::JumpListData() {}
 
 JumpList::JumpListData::~JumpListData() {}
 
 JumpList::JumpList(Profile* profile)
     : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread(
           content::BrowserThread::UI)),
       profile_(profile),
+      cancelled_update_count_(0),
+      jumplist_updater_timeout_(false),
       jumplist_data_(new base::RefCountedData<JumpListData>),
       task_id_(base::CancelableTaskTracker::kBadTaskId),
       update_jumplist_task_runner_(base::CreateCOMSTATaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
       delete_jumplisticons_task_runner_(
           base::CreateSequencedTaskRunnerWithTraits(
               {base::MayBlock(), base::TaskPriority::BACKGROUND,
                base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
       weak_ptr_factory_(this) {
@@ skipping 312 matching lines @@
   // avoiding update storms.
   if (timer_most_visited_.IsRunning()) {
     timer_most_visited_.Reset();
   } else {
     timer_most_visited_.Start(
         FROM_HERE, kDelayForJumplistUpdate,
         base::Bind(&JumpList::DeferredTopSitesChanged, base::Unretained(this)));
   }
 }
 
+bool JumpList::CheckAndUpdateTimeoutInfo() {
+  if (jumplist_updater_timeout_ &&
+      ++cancelled_update_count_ > kCancelledUpdates) {
+    jumplist_updater_timeout_ = false;
+    cancelled_update_count_ = 0;
+  }
+  return jumplist_updater_timeout_;
+}
+
 void JumpList::DeferredTopSitesChanged() {
+  if (CheckAndUpdateTimeoutInfo())
+    return;
+
   scoped_refptr<history::TopSites> top_sites =
       TopSitesFactory::GetForProfile(profile_);
   if (top_sites) {
     top_sites->GetMostVisitedURLs(
         base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                    weak_ptr_factory_.GetWeakPtr()),
         false);
   }
 }
 
 void JumpList::DeferredTabRestoreServiceChanged() {
+  if (CheckAndUpdateTimeoutInfo())
+    return;
+
   // Create a list of ShellLinkItems from the "Recently Closed" pages.
   // As noted above, we create a ShellLinkItem objects with the following
   // parameters.
   // * arguments
   //   The last URL of the tab object.
   // * title
   //   The title of the last URL.
   // * icon
   //   An empty string. This value is to be updated in OnFaviconDataAvailable().
   const int kRecentlyClosedCount = 3;
@@ skipping 66 matching lines @@
   JumpListUpdater jumplist_updater(app_id);
 
   base::ElapsedTimer begin_update_timer;
 
   if (!jumplist_updater.BeginUpdate())
     return false;
 
   // Discard this JumpList update if JumpListUpdater::BeginUpdate takes longer
   // than the maximum allowed time, as it's very likely the following update
   // steps will also take a long time.
-  if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate)
+  if (begin_update_timer.Elapsed() >= kTimeOutForJumplistBeginUpdate) {
+    jumplist_updater_timeout_ = true;
     return false;
+  }
 
   // The default maximum number of items to display in JumpList is 10.
   // https://msdn.microsoft.com/library/windows/desktop/dd378398.aspx
   // The "Most visited" category title always takes 1 of the JumpList slots if
   // |most_visited_pages| isn't empty.
   // The "Recently closed" category title will also take 1 if
   // |recently_closed_pages| isn't empty.
   // For the remaining slots, we allocate 5/8 (i.e., 5 slots if both categories
   // present) to "most-visited" items and 3/8 (i.e., 3 slots if both categories
   // present) to "recently-closed" items, respectively.
@@ skipping 44 matching lines @@
     icons_to_create +=
         std::min(recently_closed_pages.size(), recently_closed_items);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   UMA_HISTOGRAM_COUNTS_100("WinJumplist.CreateIconFilesCount", icons_to_create);
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
 
+  base::ElapsedTimer add_custom_category_timer;
+
   // Update the "Most Visited" category of the JumpList if it exists.
   // This update request is applied into the JumpList when we commit this
   // transaction.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_NEW_TAB_MOST_VISITED),
           most_visited_pages, most_visited_items)) {
     return false;
   }
 
   // Update the "Recently Closed" category of the JumpList.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED), recently_closed_pages,
           recently_closed_items)) {
     return false;
   }
 
+  // If JumpListUpdater::AddCustomCategory takes longer than the maximum allowed
+  // time, mark the flag but continue with this update.
+  if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory)
+    jumplist_updater_timeout_ = true;
+
   // Update the "Tasks" category of the JumpList.
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
     return false;
 
+  base::ElapsedTimer commit_update_timer;
+
   // Commit this transaction and send the updated JumpList to Windows.
-  return jumplist_updater.CommitUpdate();
+  bool commit_result = jumplist_updater.CommitUpdate();
+
+  // If JumpListUpdater::CommitUpdate takes longer than the maximum allowed
+  // time, mark the flag but continue with this update.
+  if (commit_update_timer.Elapsed() >= kTimeOutForJumplistCommitUpdate)
+    jumplist_updater_timeout_ = true;
+
+  return commit_result;
 }
 
 void JumpList::RunUpdateJumpList(
     IncognitoModePrefs::Availability incognito_availability,
     const base::string16& app_id,
     const base::FilePath& profile_dir,
     base::RefCountedData<JumpListData>* ref_counted_data) {
   JumpListData* data = &ref_counted_data->data;
   ShellLinkItemList local_most_visited_pages;
   ShellLinkItemList local_recently_closed_pages;
@@ skipping 30 matching lines @@
           app_id, profile_dir, local_most_visited_pages,
           local_recently_closed_pages, most_visited_pages_have_updates,
           recently_closed_pages_have_updates, incognito_availability)) {
     base::AutoLock auto_lock(data->list_lock_);
     if (most_visited_pages_have_updates)
       data->most_visited_pages_have_updates_ = true;
     if (recently_closed_pages_have_updates)
       data->recently_closed_pages_have_updates_ = true;
   }
 }