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

chrome/browser/win/jumplist.h

 // Copyright (c) 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.
 
 #ifndef CHROME_BROWSER_WIN_JUMPLIST_H_
 #define CHROME_BROWSER_WIN_JUMPLIST_H_
 
 #include <stddef.h>
 
 #include <list>
 #include <memory>
 #include <string>
 #include <utility>
 
 #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"

[grt (UTC plus 2), 2017/06/09 10:42:58]

unused! hooray!

[chengx, 2017/06/10 05:56:18]

Done. Sorry I forgot to update the header files.

 #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"

[grt (UTC plus 2), 2017/06/09 10:42:58]

move to .cc

[chengx, 2017/06/10 05:56:18]

Done.

 #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/sessions/core/tab_restore_service.h"
 #include "components/sessions/core/tab_restore_service_observer.h"
 #include "content/public/browser/browser_thread.h"

[grt (UTC plus 2), 2017/06/09 10:42:58]

unused

[chengx, 2017/06/10 05:56:18]

I think it's used in JumpList constructor. 

JumpList::JumpList(Profile* profile)
: RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread(
          content::BrowserThread::UI)), ...

[grt (UTC plus 2), 2017/06/12 07:15:33]

Please move it to the .cc file as per iwyu.

[chengx, 2017/06/12 19:13:24]

Done.

 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 class SequencedTaskRunner;
 }
 
 namespace chrome {
 struct FaviconImageResult;
@@ skipping 10 matching lines @@
 // * Retrieving strings from the application resource;
 // * Adding COM objects to JumpList, etc.
 //
 // This class observes the tabs and policies of the given Profile and updates
 // the JumpList whenever a change is detected.
 //
 // 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

[grt (UTC plus 2), 2017/06/09 10:42:57]

since all class methods are now required to run on the UI thread, i think this
comment can disappear.

[chengx, 2017/06/10 05:56:18]

Agreed. Done.

 // always delete JumpList on UI thread (the same thread it got constructed on).
 class JumpList : public sessions::TabRestoreServiceObserver,
                  public history::TopSitesObserver,
                  public RefcountedKeyedService {

[grt (UTC plus 2), 2017/06/09 10:42:58]

can this be a regular KeyedService? i don't see any evidence of refcounting
being used.

[chengx, 2017/06/10 05:56:18]

I think refcounting is used when one user (i.e., one profile) has multiple
Chrome windows (each window may contain lots of tabs) open. Please see
crrev/2323603002 where refcounting was introduced.

[grt (UTC plus 2), 2017/06/12 07:15:32]

Making it a keyed service accomplishes that (one instance per Profile rather
than one per browser window). I don't see any reason for it to be refcounted.

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

I've made it not refcounted.

  public:
-  struct JumpListData {
-    JumpListData();
-    ~JumpListData();
+  using URLIconCache = base::flat_map<std::string, base::FilePath>;
 
-    // Lock for most_visited_pages_, recently_closed_pages_, icon_urls_
-    // as they may be used by up to 2 threads.
-    base::Lock list_lock_;
+  // Holds results of the RunUpdateJumpList run.
+  struct JumpListUpdateResults {

[grt (UTC plus 2), 2017/06/09 10:42:58]

nit: just UpdateResults since this is within JumpList. also, can this type be
private?

[chengx, 2017/06/10 05:56:18]

Done. Renamed to UpdateResults and made private.

+    JumpListUpdateResults();
+    ~JumpListUpdateResults();
 
-    // 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_;
+    // 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_;
 
-    // Items in the "Most Visited" category of the application JumpList,
-    // protected by the list_lock_.
-    ShellLinkItemList most_visited_pages_;
+    // 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;
 
-    // Items in the "Recently Closed" category of the application JumpList,
-    // protected by the list_lock_.
-    ShellLinkItemList recently_closed_pages_;
+    // A flag indicating if a JumpList update run is successful.
+    bool update_success_ = true;

[grt (UTC plus 2), 2017/06/09 10:42:58]

initialize to false and set it to true in the one success case rather than
initializing to true and setting to false in the N failure cases.

[chengx, 2017/06/10 05:56:18]

Done.

 
-    // 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;
+    // 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;
   };
 
   // Observer callback for TabRestoreService::Observer to notify when a tab is

[grt (UTC plus 2), 2017/06/09 10:42:57]

nit: remove the comments on these two overrides. the only comment needed is:
  // sessions::TabRestoreServiceObserver:
just above the two of them.

please use the same notation for the other overrides as well. thanks.

[grt (UTC plus 2), 2017/06/09 10:42:58]

nit: move these overrides down into the private: section just before the
TopSitesObserver overrides. the inconsistency here has always bugged me. :-)

[chengx, 2017/06/10 05:56:18]

Done with moving the TopSitesObserver overrides to the private section down
below.

[chengx, 2017/06/10 05:56:19]

Done. Comments updated.

   // 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.
+  // Cancel a pending JumpList update.
   void CancelPendingUpdate();

[grt (UTC plus 2), 2017/06/09 10:42:58]

this should be private:

[chengx, 2017/06/10 05:56:18]

Done.

 
-  // Terminate the jumplist: cancel any pending updates and stop observing
+  // 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();

[grt (UTC plus 2), 2017/06/09 10:42:57]

private as well

[chengx, 2017/06/10 05:56:18]

Done.

 
   // RefcountedKeyedService:
   void ShutdownOnUIThread() override;
 
   // Returns true if the custom JumpList is enabled.
   static bool Enabled();
 
  private:
   friend JumpListFactory;
   explicit JumpList(Profile* profile);  // Use JumpListFactory instead
   ~JumpList() override;
 
-  enum class JumpListCategory { kMostVisited, kRecentlyClosed };
+  // 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 |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|.
+  // 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.
   // When finishing loading all favicons, this function posts a task that
   // 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);
 
   // 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);
 
   // Callback for changes to the incognito mode availability pref.
   void OnIncognitoAvailabilityChanged();
 
   // Posts tasks to update the JumpList and delete any obsolete JumpList related

[grt (UTC plus 2), 2017/06/09 10:42:58]

nit: move this down below DeferredTabRestoreServiceChanged so that the class's
methods are laid out more-or-less in the order they're invoked, grouped by
thread. i would put the static methods below all other methods since they are
their own unit and could potentially live somewhere else (e.g., all in an
unnamed namespace or in a separate class that could be unit-tested
independently).

[chengx, 2017/06/10 05:56:18]

Sure. I moved some but in this header file. I think the massive code movement,
especially in jumplist.cc for sure will mess up the "side-by-side-diff" between
patch sets. So it'll be hard for you to track my patchset-wise changes. How
about postponing this movement until everything else is settled down? I suggest
to reorder the remaining methods using the final patch set. 

[grt (UTC plus 2), 2017/06/12 07:15:32]

Good plan.

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

Acknowledged.

   // folders.
   void PostRunUpdate();
 
   // history::TopSitesObserver implementation.
   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.
+  // Called on a timer after requests storms have subsided. Calls APIs
+  // DeferredTop{SitesChanged, ServiceChanged} on demand to do the actual work.
+  void DeferredChanged();

[grt (UTC plus 2), 2017/06/09 10:42:58]

nit: i think this should either be named after when it is called (e.g.,
OnDelayTimer) or for what it does (e.g., HandlePendingNotifications).

[chengx, 2017/06/10 05:56:18]

I'll go with OnDelayTimer.

+
+  // Update the most visited URLs, called in DeferredChanged().

[grt (UTC plus 2), 2017/06/09 10:42:58]

i don't think it's necessary to document who calls the function. it doesn't
really matter. if the function has only one job, then maybe it's suitable to
call it from multiple places some day.

[chengx, 2017/06/10 05:56:18]

I've updated the comments.

   void DeferredTopSitesChanged();

[grt (UTC plus 2), 2017/06/09 10:42:58]

since the "deferred" part is now taken care of by the method above, this could
now aptly be named ProcessTopSitesNotification, or something like that.

[chengx, 2017/06/10 05:56:18]

Renamed to ProcessTopSitesNotification

 
-  // Called on a timer to update the "Recently Closed" category of JumpList
-  // after requests storms have subsided.
+  // Update the recently closed URLs, called in DeferredChanged().
   void DeferredTabRestoreServiceChanged();

[grt (UTC plus 2), 2017/06/09 10:42:58]

ProcessTabRestoreServiceNotification?

[chengx, 2017/06/10 05:56:18]

SGTM. I've renamed the variable.

 
-  // Deletes icon files of |category| in |icon_dir| which are not in the cache
-  // anymore.
-  void DeleteIconFiles(const base::FilePath& icon_dir,
-                       JumpListCategory category);
+  // Deletes icon files in |icon_dir| which are not in |icon_cache| anymore.
+  static void DeleteIconFiles(const base::FilePath& icon_dir,
+                              URLIconCache* icon_cache);
 
-  // Creates at most |max_items| icon files of |category| in |icon_dir| for the
-  // asynchrounously loaded icons stored in |item_list|.
+  // 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 CreateIconFiles(const base::FilePath& icon_dir,
-                      const ShellLinkItemList& item_list,
-                      size_t max_items,
-                      JumpListCategory category);
+  static int CreateIconFiles(const base::FilePath& icon_dir,
+                             const ShellLinkItemList& item_list,
+                             size_t max_items,
+                             URLIconCache* icon_cache);
 
-  // Updates icon files in |icon_dir|, which includes deleting old icons and
-  // creating at most |slot_limit| new icons for |page_list|.
+  // 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.
-  int UpdateIconFiles(const base::FilePath& icon_dir,
-                      const ShellLinkItemList& page_list,
-                      size_t slot_limit,
-                      JumpListCategory category);
+  static int UpdateIconFiles(const base::FilePath& icon_dir,
+                             const ShellLinkItemList& page_list,
+                             size_t slot_limit,
+                             URLIconCache* icon_cache);
 
-  // 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,
+  // 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,
+      JumpListUpdateResults* 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<JumpListUpdateResults> update_results);
 
   // Tracks FaviconService tasks.
   base::CancelableTaskTracker cancelable_task_tracker_;
 
   // The Profile object is used to listen for events.
   Profile* profile_;
 
   // Lives on the UI thread.

[grt (UTC plus 2), 2017/06/09 10:42:58]

all members live on the UI thread now, so this comment is no longer needed.

[chengx, 2017/06/10 05:56:18]

Done.

   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,
+  typedef std::pair<std::string, scoped_refptr<ShellLinkItem>> URLPair;

[grt (UTC plus 2), 2017/06/09 10:42:57]

wdyt of:
  using UrlAndLinkItem = ...

[chengx, 2017/06/10 05:56:18]

Done.

+  std::list<URLPair> 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 "Most Visited" category has pending notifications.
+  bool most_visited_has_pending_notification_ = false;
+
+  // A flag indicating if "Recently Closed" category has pending notifications.
+  bool recently_closed_has_pending_notification_ = false;
+
+  // A flag indicating if "Most Visited" category has new updates.
+  bool most_visited_pages_have_updates_ = false;
+
+  // A flag indicating if "Recently Closed" category has new updates.
+  bool recently_closed_pages_have_updates_ = 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;
 
   // 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_;

[grt (UTC plus 2), 2017/06/09 10:42:58]

 = base::CancelableTaskTracker::kBadTaskId
and remove from the initializer list in the .cc

[chengx, 2017/06/10 05:56:18]

Done.

 
   // A task runner running tasks to update the JumpList.
   scoped_refptr<base::SingleThreadTaskRunner> update_jumplist_task_runner_;
 
   // 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 be run after destruction.
+  // For callbacks may run after destruction.
   base::WeakPtrFactory<JumpList> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(JumpList);
 };
 
 #endif  // CHROME_BROWSER_WIN_JUMPLIST_H_