Time out jumplist update for very slow or busy OS

Notifying OS about the jumplist update consists of 4 steps in order:
BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of
which shouldn't take long.

However, UMA data shows that each step can take up to tens of seconds
for some Chrome users. Among the 4 steps, AddTasks is in the best
condition, while CommitUpdate is in the worst condition.

Since they are mostly Windows API calls, we can assume this is due to
user machines (which can be very slow or busy) with a pretty high
confidence, and also there's not much we can improve on Chrome's side.
For these situations, we should stop jumplist update to avoid making
things worse. Code-wise, we stop the update if BeginUpdate takes a
crazy amount of time, as the following steps (delete old icons, create
new icons, AddCustomCategory, CommitUpdate, etc) are very likely to
take a long time too.

BUG=40407, 179576

Review-Url: https://codereview.chromium.org/2831433003
Cr-Commit-Position: refs/heads/master@{#467009}

chrome/browser/win/jumplist.cc

@@ -20,6 +20,7 @@
 #include "base/task_scheduler/post_task.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/timer/elapsed_timer.h"
 #include "base/trace_event/trace_event.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
@@ -64,7 +65,12 @@ using JumpListData = JumpList::JumpListData;
 namespace {
 
 // Delay jumplist updates to allow collapsing of redundant update requests.
-const int kDelayForJumplistUpdateInMS = 3500;
+constexpr base::TimeDelta kDelayForJumplistUpdate =
+    base::TimeDelta::FromMilliseconds(3500);
+
+// Timeout jumplist updates for users with extremely slow OS.
+constexpr base::TimeDelta kTimeOutForJumplistUpdate =
+    base::TimeDelta::FromMilliseconds(500);
 
 // Append the common switches to each shell link.
 void AppendCommonSwitches(ShellLinkItem* shell_link) {
@@ -176,21 +182,30 @@ bool UpdateTaskCategory(
   return jumplist_updater->AddTasks(items);
 }
 
-// Updates the application JumpList.
-bool UpdateJumpList(const wchar_t* app_id,
+// Updates the application JumpList, which consists of 1) delete old icon files;
+// 2) create new icon files; 3) notify the OS.
+// Note that any timeout error along the way results in the old jumplist being
+// left as-is, while any non-timeout error results in the old jumplist being
+// left as-is, but without icon files.
+void UpdateJumpList(const wchar_t* app_id,
                     const base::FilePath& icon_dir,
                     const ShellLinkItemList& most_visited_pages,
                     const ShellLinkItemList& recently_closed_pages,
                     IncognitoModePrefs::Availability incognito_availability) {
-  // JumpList is implemented only on Windows 7 or later.
-  // So, we should return now when this function is called on earlier versions
-  // of Windows.
   if (!JumpListUpdater::IsEnabled())
-    return true;
+    return;
+
+  // Records the time cost of starting a JumpListUpdater.
+  base::ElapsedTimer begin_update_timer;
 
   JumpListUpdater jumplist_updater(app_id);
   if (!jumplist_updater.BeginUpdate())
-    return false;
+    return;
+
+  // Stops jumplist update if JumpListUpdater's start times out, as it's very
+  // likely the following update steps will also take a long time.
+  if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate)
+    return;
 
   // We allocate 60% of the given JumpList slots to "most-visited" items
   // and 40% to "recently-closed" items, respectively.
@@ -218,47 +233,39 @@ bool UpdateJumpList(const wchar_t* app_id,
   // links should be updated anyway, as it doesn't involve disk IO. In this
   // case, Chrome's icon will be used for the new links.
 
-  if (base::DirectoryExists(icon_dir) && base::IsDirectoryEmpty(icon_dir)) {
-    // TODO(chengx): Remove this UMA metric after fixing http://crbug.com/40407.
-    UMA_HISTOGRAM_COUNTS_100(
-        "WinJumplist.CreateIconFilesCount",
-        most_visited_pages.size() + recently_closed_pages.size());
+  bool should_create_icons =
+      base::DirectoryExists(icon_dir) && base::IsDirectoryEmpty(icon_dir);
 
-    // Create icon files for shortcuts in the "Most Visited" category.
+  // Create icon files for shortcuts in the "Most Visited" category.
+  if (should_create_icons)
     CreateIconFiles(icon_dir, most_visited_pages, most_visited_items);
 
-    // Create icon files for shortcuts in the "Recently Closed" category.
-    CreateIconFiles(icon_dir, recently_closed_pages, recently_closed_items);
-  }
-
-  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
-  SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
-
   // 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;
+    return;
   }
 
+  // Create icon files for shortcuts in the "Recently Closed" category.
+  if (should_create_icons)
+    CreateIconFiles(icon_dir, recently_closed_pages, recently_closed_items);
+
   // 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;
+    return;
   }
 
   // Update the "Tasks" category of the JumpList.
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
-    return false;
+    return;
 
   // Commit this transaction and send the updated JumpList to Windows.
-  if (!jumplist_updater.CommitUpdate())
-    return false;
-
-  return true;
+  jumplist_updater.CommitUpdate();
 }
 
 // Updates the jumplist, once all the data has been fetched.
@@ -282,9 +289,7 @@ void RunUpdateJumpList(IncognitoModePrefs::Availability incognito_availability,
     local_recently_closed_pages = data->recently_closed_pages_;
   }
 
-  // Create a new JumpList and replace the current JumpList with it. The
-  // jumplist links are updated anyway, while the jumplist icons may not as
-  // mentioned above.
+  // Updates the application JumpList.
   UpdateJumpList(app_id.c_str(), icon_dir, local_most_visited_pages,
                  local_recently_closed_pages, incognito_availability);
 }
@@ -595,9 +600,7 @@ void JumpList::PostRunUpdate() {
   if (timer_.IsRunning()) {
     timer_.Reset();
   } else {
-    timer_.Start(FROM_HERE,
-                 base::TimeDelta::FromMilliseconds(kDelayForJumplistUpdateInMS),
-                 this,
+    timer_.Start(FROM_HERE, kDelayForJumplistUpdate, this,
                  &JumpList::DeferredRunUpdate);
   }
 }

chrome/browser/win/jumplist_updater.cc

@@ -14,7 +14,6 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/win/win_util.h"
-#include "base/win/windows_version.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/common/content_switches.h"
 
@@ -111,11 +110,9 @@ JumpListUpdater::~JumpListUpdater() {
 
 // static
 bool JumpListUpdater::IsEnabled() {
-  // JumpList is implemented only on Windows 7 or later.
   // Do not create custom JumpLists in tests. See http://crbug.com/389375.
-  return base::win::GetVersion() >= base::win::VERSION_WIN7 &&
-         !base::CommandLine::ForCurrentProcess()->HasSwitch(
-             switches::kTestType);
+  return !base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kTestType);
 }
 
 bool JumpListUpdater::BeginUpdate() {

tools/metrics/histograms/histograms.xml

@@ -82192,6 +82192,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="WinJumplist.CreateIconFilesCount">
+  <obsolete>
+    Obsolete 04/20/2017 as we are no long recording this metric.
+  </obsolete>
   <owner>chengx@chromium.org</owner>
   <summary>
     Count of jumplist icons that will be created per jumplist update. It is
@@ -82342,6 +82345,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="WinJumplist.UpdateJumpListDuration" units="ms">
+  <obsolete>
+    Obsolete 04/20/2017 as it's no longer accurate due to the code change.
+  </obsolete>
   <owner>chengx@chromium.org</owner>
   <summary>
     Time spent in UpdateJumpList(). This method is called whenever there is a