Time out jumplist update for very slow or busy OS

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"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #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"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/metrics/jumplist_metrics_win.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/shell_integration_win.h"
 #include "chrome/browser/win/jumplist_file_util.h"
 #include "chrome/common/chrome_constants.h"
@@ skipping 26 matching lines @@
 #include "url/gurl.h"
 
 using content::BrowserThread;
 using JumpListData = JumpList::JumpListData;
 
 namespace {
 
 // Delay jumplist updates to allow collapsing of redundant update requests.
 const int kDelayForJumplistUpdateInMS = 3500;
 
+// Timeout jumplist updates for users with extremely slow OS.
+const int kTimeOutForJumplistUpdateInMS = 500;

[grt (UTC plus 2), 2017/04/20 12:48:53]

nit:
constexpr base::TimeDelta kTimeOutForJumplistUpdate =
    base::TimeDelta::FromMilliseconds(500);

[chengx, 2017/04/20 21:01:36]

Done.

+
 // Append 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 102 matching lines @@
                     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;
 
+  // Records the time cost of starting a JumpListUpdater.
+  base::ElapsedTimer begin_update_timer;
+
   JumpListUpdater jumplist_updater(app_id);
   if (!jumplist_updater.BeginUpdate())
     return false;
 
+  // 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().InMilliseconds() >=

[grt (UTC plus 2), 2017/04/20 12:48:53]

nit:
  if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate)

[chengx, 2017/04/20 21:01:36]

Done.

+      kTimeOutForJumplistUpdateInMS)
+    return false;
+
   // We allocate 60% of the given JumpList slots to "most-visited" items
   // and 40% to "recently-closed" items, respectively.
   // Nevertheless, if there are not so many items in |recently_closed_pages|,
   // we give the remaining slots to "most-visited" items.
   // The default maximum number of items to display in JumpList is 10.
   // https://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx
   const int kMostVisited = 60;
   const int kRecentlyClosed = 40;
   const int kTotal = kMostVisited + kRecentlyClosed;
   size_t most_visited_items =
@@ skipping 22 matching lines @@
     // Create icon files for shortcuts in the "Most Visited" category.
     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");
 
+  // Records the time cost of AddCustomCategory in JumpListUpdater.
+  base::ElapsedTimer add_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;

[grt (UTC plus 2), 2017/04/20 12:48:53]

don't these early returns skew the data in WinJumplist.UpdateJumpListDuration?

should you also not delete the icon files in these early return cases since
they're unused? could this be contributing to the explosion of icon files?

[chengx, 2017/04/20 21:01:36]

Re: Since I am logging all its 4 steps' (BeginUpdate, CommitUpdate, etc) running
time to UMA, WinJumplist.UpdateJumpListDuration as a whole is not that important
now. Actually, I've retired this metric in the new patch set because originally
we have 
CreateIcons1 --> CreateIcons2 --> AddCategory1 --> AddCategory2
Now it's changed to
CreateIcons1 --> AddCategory1 --> CreateIcons2 --> AddCategory2

Since if AddCategory1 returns early, there's no need to run CreateIcons2.
However after this change, UpdateJumpListDuration is no longer accurate, so it's
retired.
Re: I agree this is the correct thing to do, but only if the situation allows to
do so which however seems not. Here are more details.

CreateIconFiles must be called before AddCustomCategory, which means the the new
icons are already created in the jumplist folder. Due to the imperfectness of
file deletion, now CreateIconFiles is only called when the JumpListIcons folder
is empty, i.e., when all the old icons are deleted. I think this is the safest
way to ensure the folder is not bloated. It's really a dilemma.
Re: No, it won't. These newly created but unused icons will be deleted in the
next jumplist update. If the deletion fails, no more new icons will be created. 

[grt (UTC plus 2), 2017/04/21 10:11:25]

Please leave a doc comment here near the first call to CreateIconFiles
explaining that icon files may be left behind on early exit, but that they will
be deleted on the next jumplist update.

   }
 
   // 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;
   }
 
+  // Stops jumplist update if AddCustomCategory times out, as it's very likely
+  // the following CommitUpdate call will also take a long time.
+  if (add_category_timer.Elapsed().InMilliseconds() >=

[grt (UTC plus 2), 2017/04/20 12:48:53]

here, too

[chengx, 2017/04/20 21:01:36]

I decided to only keep a timer for BeginUpdate for now to see what happens
first. So this section of code is gone now.

+      kTimeOutForJumplistUpdateInMS)
+    return false;
+
   // Update the "Tasks" category of the JumpList.
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
     return false;
 
   // Commit this transaction and send the updated JumpList to Windows.
   if (!jumplist_updater.CommitUpdate())
     return false;
 
   return true;
 }
@@ skipping 366 matching lines @@
 void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 }
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
   top_sites->GetMostVisitedURLs(
       base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                  weak_ptr_factory_.GetWeakPtr()),
       false);
 }