Revert of 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 d072da720a4c7f6190043d6859a6e0bb48b5826e..42473cf41ac391eef923b3256415a778ea212294 100644
--- a/chrome/browser/win/jumplist.h
+++ b/chrome/browser/win/jumplist.h
@@ -19,15 +19,18 @@
 #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/keyed_service.h"
+#include "components/keyed_service/core/refcounted_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"
 
@@ -57,58 +60,88 @@
 // 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 KeyedService {
+                 public RefcountedKeyedService {
  public:
-  // KeyedService:
-  void Shutdown() override;
+  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;
 
   // Returns true if the custom JumpList is enabled.
   static bool Enabled();
 
  private:
-  using UrlAndLinkItem = std::pair<std::string, scoped_refptr<ShellLinkItem>>;
-  using URLIconCache = base::flat_map<std::string, base::FilePath>;
-
-  // Holds results of the RunUpdateJumpList run.
-  struct UpdateResults {
-    UpdateResults();
-    ~UpdateResults();
-
-    // 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;
-
-    // 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;
-  };
-
   friend JumpListFactory;
   explicit JumpList(Profile* profile);  // Use JumpListFactory instead
-
   ~JumpList() override;
 
-  // 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|.
+  enum class JumpListCategory { kMostVisited, kRecentlyClosed };
+
+  // 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);
+
+  // Adds a new ShellLinkItem for each tab in |window| to |data| provided that
+  // doing so will not exceed |max_items|.
   void AddWindow(const sessions::TabRestoreService::Window& window,
-                 size_t max_items);
+                 size_t max_items,
+                 JumpListData* data);
 
   // Starts loading a favicon for each URL in |icon_urls_|.
   // This function sends a query to HistoryService.
@@ -116,14 +149,15 @@
   // decompresses collected favicons and updates a JumpList.
   void StartLoadingFavicon();
 
-  // Callback for HistoryService that notifies when a requested favicon is
-  // available. To avoid file operations, this function just attaches the given
-  // |image_result| to a ShellLinkItem object.
+  // 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.
   void OnFaviconDataAvailable(
       const favicon_base::FaviconImageResult& image_result);
 
-  // Callback for TopSites that notifies when |data|, the "Most Visited" list,
-  // is available. This function updates the ShellLinkItemList objects and
+  // Callback for TopSites that notifies when the "Most Visited" list is
+  // available. This function updates the ShellLinkItemList objects and
   // begins the process of fetching favicons for the URLs.
   void OnMostVisitedURLsAvailable(
       const history::MostVisitedURLList& data);
@@ -131,86 +165,65 @@
   // Callback for changes to the incognito mode availability pref.
   void OnIncognitoAvailabilityChanged();
 
-  // sessions::TabRestoreServiceObserver:
-  void TabRestoreServiceChanged(sessions::TabRestoreService* service) override;
-  void TabRestoreServiceDestroyed(
-      sessions::TabRestoreService* service) override;
-
-  // history::TopSitesObserver:
+  // Posts tasks to update the JumpList and delete any obsolete JumpList related
+  // folders.
+  void PostRunUpdate();
+
+  // history::TopSitesObserver implementation.
   void TopSitesLoaded(history::TopSites* top_sites) override;
   void TopSitesChanged(history::TopSites* top_sites,
                        ChangeReason change_reason) override;
 
-  // Initializes 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 after requests storms have subsided. Calls APIs
-  // ProcessTopSitesNotification and ProcessTabRestoreNotification on
-  // demand to do the actual work.
-  void OnDelayTimer();
-
-  // Processes notifications from TopSites service.
-  void ProcessTopSitesNotification();
-
-  // Processes notifications from TabRestore service.
-  void ProcessTabRestoreServiceNotification();
-
-  // 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.
+  // Called on a timer to update the most visited URLs after requests storms
+  // have subsided.
+  void DeferredTopSitesChanged();
+
+  // Called on a timer to update the "Recently Closed" category of JumpList
+  // after requests storms have subsided.
+  void DeferredTabRestoreServiceChanged();
+
+  // Deletes icon files of |category| in |icon_dir| which are not in the cache
+  // anymore.
+  void DeleteIconFiles(const base::FilePath& icon_dir,
+                       JumpListCategory category);
+
+  // 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.
-  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|.
+  int CreateIconFiles(const base::FilePath& icon_dir,
+                      const ShellLinkItemList& item_list,
+                      size_t max_items,
+                      JumpListCategory category);
+
+  // Updates icon files in |icon_dir|, which includes deleting old icons and
+  // creating at most |slot_limit| new icons for |page_list|.
   // 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(
+  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,
       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,
-      UpdateResults* update_results);
-
-  // 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);
-
-  // Cancels a pending JumpList update.
-  void CancelPendingUpdate();
-
-  // Terminates the JumpList, which includes cancelling any pending updates and
-  // stopping observing the Profile and its services. This must be called before
-  // the |profile_| is destroyed.
-  void Terminate();
+      base::RefCountedData<JumpListData>* ref_counted_data);
+
+  // 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);
 
   // Tracks FaviconService tasks.
   base::CancelableTaskTracker cancelable_task_tracker_;
@@ -218,68 +231,50 @@
   // The Profile object is used to listen for events.
   Profile* profile_;
 
-  // Manages the registration of pref change observers.
+  // Lives on the UI thread.
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
-  // App id to associate with the JumpList.
+  // App id to associate with the jump list.
   base::string16 app_id_;
 
-  // Timer for requesting delayed JumpList updates.
-  base::OneShotTimer timer_;
-
-  // A list of URLs we need to retrieve their favicons,
-  std::list<UrlAndLinkItem> icon_urls_;
-
-  // 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 flag indicating if there's a JumpList update task already posted or
-  // currently running.
-  bool update_in_progress_ = false;
-
-  // A flag indicating if a session has at least one tab closed.
-  bool has_tab_closed_ = false;
+  // Timer for requesting delayed updates of the "Most Visited" category of
+  // jumplist.
+  base::OneShotTimer timer_most_visited_;
+
+  // Timer for requesting delayed updates of the "Recently Closed" category of
+  // jumplist.
+  base::OneShotTimer timer_recently_closed_;
 
   // 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;
 
+  // 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_;
+
   // 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::kBadTaskId;
+  base::CancelableTaskTracker::TaskId task_id_;
 
   // A task runner running tasks to update the JumpList.
   scoped_refptr<base::SingleThreadTaskRunner> update_jumplist_task_runner_;
 
-  // A task runner running tasks to delete the JumpListIcons and
-  // JumpListIconsOld folders.
+  // A task runner running tasks to delete JumpListIcons directory and
+  // JumpListIconsOld directory.
   scoped_refptr<base::SequencedTaskRunner> delete_jumplisticons_task_runner_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 
-  // For callbacks may run after destruction.
+  // For callbacks may be run after destruction.
   base::WeakPtrFactory<JumpList> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(JumpList);