Fix stability and data racing issues, coalesce more updates for JumpList

chrome/browser/win/jumplist.h

Index: chrome/browser/win/jumplist.h
diff --git a/chrome/browser/win/jumplist.h b/chrome/browser/win/jumplist.h
index 42473cf41ac391eef923b3256415a778ea212294..1bd00914505d535f5ac4507fcda3ce369b22b9c5 100644
--- a/chrome/browser/win/jumplist.h
+++ b/chrome/browser/win/jumplist.h
@@ -15,22 +15,18 @@
 #include "base/containers/flat_map.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/strings/string16.h"
-#include "base/synchronization/lock.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/win/jumplist_updater.h"
-#include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/history/core/browser/top_sites_observer.h"
-#include "components/keyed_service/core/refcounted_keyed_service.h"
+#include "components/keyed_service/core/keyed_service.h"
 #include "components/sessions/core/tab_restore_service.h"
 #include "components/sessions/core/tab_restore_service_observer.h"
-#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
@@ -60,66 +56,12 @@ class Profile;
 // Updating a JumpList requires some file operations and it is not good to
 // update it in a UI thread. To solve this problem, this class posts to a
 // runnable method when it actually updates a JumpList.
-//
-// Note. base::CancelableTaskTracker is not thread safe, so we
-// always delete JumpList on UI thread (the same thread it got constructed on).
 class JumpList : public sessions::TabRestoreServiceObserver,
                  public history::TopSitesObserver,
-                 public RefcountedKeyedService {
+                 public KeyedService {
  public:
-  struct JumpListData {
-    JumpListData();
-    ~JumpListData();
-
-    // Lock for most_visited_pages_, recently_closed_pages_, icon_urls_
-    // as they may be used by up to 2 threads.
-    base::Lock list_lock_;
-
-    // A list of URLs we need to retrieve their favicons,
-    // protected by the list_lock_.
-    typedef std::pair<std::string, scoped_refptr<ShellLinkItem> > URLPair;
-    std::list<URLPair> icon_urls_;
-
-    // Items in the "Most Visited" category of the application JumpList,
-    // protected by the list_lock_.
-    ShellLinkItemList most_visited_pages_;
-
-    // Items in the "Recently Closed" category of the application JumpList,
-    // protected by the list_lock_.
-    ShellLinkItemList recently_closed_pages_;
-
-    // A boolean flag indicating if "Most Visited" category of the JumpList
-    // has new updates therefore its icons need to be updated.
-    // By default, this flag is set to false. If there's any change in
-    // TabRestoreService, this flag will be set to true.
-    bool most_visited_pages_have_updates_ = false;
-
-    // A boolean flag indicating if "Recently Closed" category of the JumpList
-    // has new updates therefore its icons need to be updated.
-    // By default, this flag is set to false. If there's any change in TopSites
-    // service, this flag will be set to true.
-    bool recently_closed_pages_have_updates_ = false;
-  };
-
-  // Observer callback for TabRestoreService::Observer to notify when a tab is
-  // added or removed.
-  void TabRestoreServiceChanged(sessions::TabRestoreService* service) override;
-
-  // Observer callback to notice when our associated TabRestoreService
-  // is destroyed.
-  void TabRestoreServiceDestroyed(
-      sessions::TabRestoreService* service) override;
-
-  // Cancel a pending jumplist update.
-  void CancelPendingUpdate();
-
-  // Terminate the jumplist: cancel any pending updates and stop observing
-  // the Profile and its services. This must be called before the |profile_|
-  // is destroyed.
-  void Terminate();
-
-  // RefcountedKeyedService:
-  void ShutdownOnUIThread() override;
+  // KeyedService:
+  void Shutdown() override;
 
   // Returns true if the custom JumpList is enabled.
   static bool Enabled();
@@ -129,19 +71,41 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   explicit JumpList(Profile* profile);  // Use JumpListFactory instead

[grt (UTC plus 2), 2017/06/12 20:43:39]

nit: move these methods down below the types as per
https://google.github.io/styleguide/cppguide.html#Declaration_Order

[chengx, 2017/06/12 22:05:30]

Factory methods and the destructor moved down below "struct UpdateResults".

   ~JumpList() override;
 
-  enum class JumpListCategory { kMostVisited, kRecentlyClosed };
+  using URLIconCache = base::flat_map<std::string, base::FilePath>;
 
-  // Adds a new ShellLinkItem for |tab| to |data| provided that doing so will
-  // not exceed |max_items|.
-  bool AddTab(const sessions::TabRestoreService::Tab& tab,
-              size_t max_items,
-              JumpListData* data);
+  // Holds results of the RunUpdateJumpList run.
+  struct UpdateResults {
+    UpdateResults();
+    ~UpdateResults();
 
-  // Adds a new ShellLinkItem for each tab in |window| to |data| provided that
-  // doing so will not exceed |max_items|.
+    // Icon file paths of the most visited links, indexed by tab url.
+    // Holding a copy of most_visited_icons_ initially, it's updated by the
+    // JumpList update run. If the update run succeeds, it overwrites
+    // most_visited_icons_.
+    URLIconCache most_visited_icons_in_update_;

[grt (UTC plus 2), 2017/06/12 20:43:39]

nit: omit the trailing underscore for these public struct members.

[chengx, 2017/06/12 22:05:30]

Done.

+
+    // icon file paths of the recently closed links, indexed by tab url.
+    // Holding a copy of recently_closed_icons_ initially, it's updated by the
+    // JumpList update run. If the update run succeeds, it overwrites
+    // recently_closed_icons_.
+    URLIconCache recently_closed_icons_in_update;
+
+    // A flag indicating if a JumpList update run is successful.
+    bool update_success_ = false;
+
+    // A flag indicating if there is a timeout in notifying the JumpList update
+    // to shell. Note that this variable is independent of update_success_.
+    bool update_timeout_ = false;
+  };
+
+  // Adds a new ShellLinkItem for |tab| to the JumpList data provided that doing
+  // so will not exceed |max_items|.
+  bool AddTab(const sessions::TabRestoreService::Tab& tab, size_t max_items);
+
+  // Adds a new ShellLinkItem for each tab in |window| to the JumpList data
+  // provided that doing so will not exceed |max_items|.
   void AddWindow(const sessions::TabRestoreService::Window& window,
-                 size_t max_items,
-                 JumpListData* data);
+                 size_t max_items);
 
   // Starts loading a favicon for each URL in |icon_urls_|.
   // This function sends a query to HistoryService.
@@ -149,10 +113,9 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // decompresses collected favicons and updates a JumpList.
   void StartLoadingFavicon();
 
-  // A callback function for HistoryService that notify when a requested favicon
-  // is available.
-  // To avoid file operations, this function just attaches the given data to
-  // a ShellLinkItem object.
+  // Callback for HistoryService that notifies when a requested favicon is
+  // available. To avoid file operations, this function just attaches the given
+  // data to a ShellLinkItem object.
   void OnFaviconDataAvailable(
       const favicon_base::FaviconImageResult& image_result);
 
@@ -165,65 +128,86 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // Callback for changes to the incognito mode availability pref.
   void OnIncognitoAvailabilityChanged();
 
-  // Posts tasks to update the JumpList and delete any obsolete JumpList related
-  // folders.
-  void PostRunUpdate();
+  // sessions::TabRestoreServiceObserver:
+  void TabRestoreServiceChanged(sessions::TabRestoreService* service) override;
+  void TabRestoreServiceDestroyed(
+      sessions::TabRestoreService* service) override;
 
-  // history::TopSitesObserver implementation.
+  // history::TopSitesObserver:
   void TopSitesLoaded(history::TopSites* top_sites) override;
   void TopSitesChanged(history::TopSites* top_sites,
                        ChangeReason change_reason) override;
 
-  // Called on a timer to update the most visited URLs after requests storms
-  // have subsided.
-  void DeferredTopSitesChanged();
+  // Initialize the one-shot timer to update the JumpList in a while. If there
+  // is already a request queued then cancel it and post the new request. This
+  // ensures that JumpList update won't happen until there has been a brief
+  // quiet period, thus avoiding update storms.
+  void InitializeTimerForUpdate();
 
-  // Called on a timer to update the "Recently Closed" category of JumpList
-  // after requests storms have subsided.
-  void DeferredTabRestoreServiceChanged();
+  // Called on a timer after requests storms have subsided. Calls APIs
+  // ProcessTopSitesNotification and ProcessTabRestoreNotification on
+  // demand to do the actual work.
+  void OnDelayTimer();
 
-  // Deletes icon files of |category| in |icon_dir| which are not in the cache
-  // anymore.
-  void DeleteIconFiles(const base::FilePath& icon_dir,
-                       JumpListCategory category);
+  // Processes notifications from TopSites service.
+  void ProcessTopSitesNotification();
 
-  // Creates at most |max_items| icon files of |category| in |icon_dir| for the
-  // asynchrounously loaded icons stored in |item_list|.
-  // Returns the number of new icon files created.
-  int CreateIconFiles(const base::FilePath& icon_dir,
-                      const ShellLinkItemList& item_list,
-                      size_t max_items,
-                      JumpListCategory category);
+  // Processes notifications from TabRestore service.
+  void ProcessTabRestoreServiceNotification();
 
-  // Updates icon files in |icon_dir|, which includes deleting old icons and
-  // creating at most |slot_limit| new icons for |page_list|.
+  // Posts tasks to update the JumpList and delete any obsolete JumpList related
+  // folders.
+  void PostRunUpdate();
+
+  // Deletes icon files in |icon_dir| which are not in |icon_cache| anymore.
+  static void DeleteIconFiles(const base::FilePath& icon_dir,
+                              URLIconCache* icon_cache);
+
+  // In |icon_dir|, creates at most |max_items| icon files which are not in
+  // |icon_cache| for the asynchrounously loaded icons stored in |item_list|.
+  // |icon_cache| is also updated for newly created icons.
   // Returns the number of new icon files created.
-  int UpdateIconFiles(const base::FilePath& icon_dir,
-                      const ShellLinkItemList& page_list,
-                      size_t slot_limit,
-                      JumpListCategory category);
-
-  // Updates the jumplist, once all the data has been fetched. This method calls
-  // UpdateJumpList() to do most of the work.
-  void RunUpdateJumpList(
-      IncognitoModePrefs::Availability incognito_availability,
+  static int CreateIconFiles(const base::FilePath& icon_dir,
+                             const ShellLinkItemList& item_list,
+                             size_t max_items,
+                             URLIconCache* icon_cache);
+
+  // Updates icon files for |page_list| in |icon_dir|, which consists of
+  // 1) creating at most |slot_limit| new icons which are not in |icon_cache|;
+  // 2) deleting old icons which are not in |icon_cache|.
+  // Returns the number of new icon files created.
+  static int UpdateIconFiles(const base::FilePath& icon_dir,
+                             const ShellLinkItemList& page_list,
+                             size_t slot_limit,
+                             URLIconCache* icon_cache);
+
+  // Updates the application JumpList, which consists of 1) create new icon
+  // files; 2) delete obsolete 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.
+  static void RunUpdateJumpList(
       const base::string16& app_id,
       const base::FilePath& profile_dir,
-      base::RefCountedData<JumpListData>* ref_counted_data);
+      const ShellLinkItemList& most_visited_pages,
+      const ShellLinkItemList& recently_closed_pages,
+      bool most_visited_pages_have_updates,
+      bool recently_closed_pages_have_updates,
+      IncognitoModePrefs::Availability incognito_availability,
+      UpdateResults* update_results);
 
-  // Updates the application JumpList, which consists of 1) delete old icon
-  // files; 2) create new icon files; 3) notify the OS. This method is called
-  // from RunUpdateJumpList().
-  // 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.
-  bool UpdateJumpList(const base::string16& app_id,
-                      const base::FilePath& profile_dir,
-                      const ShellLinkItemList& most_visited_pages,
-                      const ShellLinkItemList& recently_closed_pages,
-                      bool most_visited_pages_have_updates,
-                      bool recently_closed_pages_have_updates,
-                      IncognitoModePrefs::Availability incognito_availability);
+  // Callback for RunUpdateJumpList that notifies when it finishes running.
+  // Updates certain JumpList member variables and/or triggers a new JumpList
+  // update based on |update_results|.
+  void OnRunUpdateCompletion(std::unique_ptr<UpdateResults> update_results);
+
+  // Cancel a pending JumpList update.
+  void CancelPendingUpdate();
+
+  // Terminate the JumpList: cancel any pending updates and stop observing
+  // the Profile and its services. This must be called before the |profile_|
+  // is destroyed.
+  void Terminate();
 
   // Tracks FaviconService tasks.
   base::CancelableTaskTracker cancelable_task_tracker_;
@@ -231,39 +215,58 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // The Profile object is used to listen for events.
   Profile* profile_;
 
-  // Lives on the UI thread.
+  // Manages the registration of pref change observers.
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
-  // App id to associate with the jump list.
+  // App id to associate with the JumpList.
   base::string16 app_id_;
 
-  // Timer for requesting delayed updates of the "Most Visited" category of
-  // jumplist.
-  base::OneShotTimer timer_most_visited_;
+  // Timer for requesting delayed JumpList updates.
+  base::OneShotTimer timer_;
 
-  // Timer for requesting delayed updates of the "Recently Closed" category of
-  // jumplist.
-  base::OneShotTimer timer_recently_closed_;
+  // A list of URLs we need to retrieve their favicons,
+  using UrlAndLinkItem = std::pair<std::string, scoped_refptr<ShellLinkItem>>;

[grt (UTC plus 2), 2017/06/12 20:43:39]

nit: move this up to the top of the private: section as per declaration order

[chengx, 2017/06/12 22:05:30]

Done.

+  std::list<UrlAndLinkItem> icon_urls_;
 
-  // Number of updates to skip to alleviate the machine when a previous update
-  // was too slow. Updates will be resumed when this reaches 0 again.
-  int updates_to_skip_ = 0;
+  // Items in the "Most Visited" category of the JumpList.
+  ShellLinkItemList most_visited_pages_;
+
+  // Items in the "Recently Closed" category of the JumpList.
+  ShellLinkItemList recently_closed_pages_;
+
+  // The icon file paths of the most visited links, indexed by tab url.
+  URLIconCache most_visited_icons_;
+
+  // The icon file paths of the recently closed links, indexed by tab url.
+  URLIconCache recently_closed_icons_;
+
+  // A flag indicating if TopSites service has notifications.
+  bool top_sites_has_pending_notification_ = false;
+
+  // A flag indicating if TabRestore service has notifications.
+  bool tab_restore_has_pending_notification_ = false;
+
+  // A flag indicating if "Most Visited" category should be updated.
+  bool most_visited_should_update_ = false;
+
+  // A flag indicating if "Recently Closed" category should be updated.
+  bool recently_closed_should_update_ = false;
+
+  // A boolean flag indicating if there's a JumpList update task already posted
+  // or currently running.
+  bool update_in_progress_ = false;
 
   // A boolean flag indicating if a session has at least one tab closed.
   bool has_tab_closed_ = false;
 
-  // Holds data that can be accessed from multiple threads.
-  scoped_refptr<base::RefCountedData<JumpListData>> jumplist_data_;
-
-  // The icon file paths of the most visited links and the recently closed links
-  // in the current jumplist, indexed by tab url, respectively.
-  // They may only be accessed on update_jumplist_task_runner_.
-  base::flat_map<std::string, base::FilePath> most_visited_icons_;
-  base::flat_map<std::string, base::FilePath> recently_closed_icons_;
+  // Number of updates to skip to alleviate the machine when a previous update
+  // was too slow. Updates will be resumed when this reaches 0 again.
+  int updates_to_skip_ = 0;
 
   // Id of last favicon task. It's used to cancel current task if a new one
   // comes in before it finishes.
-  base::CancelableTaskTracker::TaskId task_id_;
+  base::CancelableTaskTracker::TaskId task_id_ =
+      base::CancelableTaskTracker::kBadTaskId;
 
   // A task runner running tasks to update the JumpList.
   scoped_refptr<base::SingleThreadTaskRunner> update_jumplist_task_runner_;
@@ -274,7 +277,7 @@ class JumpList : public sessions::TabRestoreServiceObserver,
 
   SEQUENCE_CHECKER(sequence_checker_);
 
-  // For callbacks may be run after destruction.
+  // For callbacks may run after destruction.
   base::WeakPtrFactory<JumpList> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(JumpList);