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

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 "base/base_paths.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 35 matching lines @@
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/icon_util.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_family.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_rep.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
-using JumpListData = JumpList::JumpListData;
 
 namespace {
 
 // The default maximum number of items to display in JumpList is 10.
 // https://msdn.microsoft.com/library/windows/desktop/dd378398.aspx
 // The "Most visited" and "Recently closed" category titles always take 2 slots.
 // For the remaining 8 slots, we allocate 5 slots to "most-visited" items and 3
 // slots to "recently-closed" items, respectively.
 constexpr size_t kMostVisitedItems = 5;
 constexpr size_t kRecentlyClosedItems = 3;
@@ skipping 122 matching lines @@
 base::FilePath GenerateJumplistIconDirName(
     const base::FilePath& profile_dir,
     const base::FilePath::StringPieceType& suffix) {
   base::FilePath::StringType dir_name(chrome::kJumpListIconDirname);
   suffix.AppendToString(&dir_name);
   return profile_dir.Append(dir_name);
 }
 
 }  // namespace
 
-JumpList::JumpListData::JumpListData() {}
+JumpList::JumpListUpdateResults::JumpListUpdateResults() {}
 
-JumpList::JumpListData::~JumpListData() {}
+JumpList::JumpListUpdateResults::~JumpListUpdateResults() {}
 
 JumpList::JumpList(Profile* profile)
     : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread(
           content::BrowserThread::UI)),
       profile_(profile),
-      jumplist_data_(new base::RefCountedData<JumpListData>),
       task_id_(base::CancelableTaskTracker::kBadTaskId),
       update_jumplist_task_runner_(base::CreateCOMSTATaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
       delete_jumplisticons_task_runner_(
           base::CreateSequencedTaskRunnerWithTraits(
               {base::MayBlock(), base::TaskPriority::BACKGROUND,
                base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
       weak_ptr_factory_(this) {
   DCHECK(Enabled());
   // To update JumpList when a tab is added or removed, we add this object to
   // the observer list of the TabRestoreService class.
   // When we add this object to the observer list, we save the pointer to this
   // TabRestoreService object. This pointer is used when we remove this object
   // from the observer list.
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
   if (!tab_restore_service)
     return;
 
   app_id_ =
       shell_integration::win::GetChromiumModelIdForProfile(profile_->GetPath());
 
   scoped_refptr<history::TopSites> top_sites =

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

nit: move this down just after the comment below with no blank lines between:
  // The comment
  scoped_refptr<>... = ...;
  if (top_sites)
    ...;

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

Done.

       TopSitesFactory::GetForProfile(profile_);
-  if (top_sites) {
-    // Register as TopSitesObserver so that we can update ourselves when the
-    // TopSites changes. TopSites updates itself after a delay. This is
-    // especially noticable when your profile is empty.
+
+  // Register as TopSitesObserver so that we can update ourselves when the
+  // TopSites changes. TopSites updates itself after a delay. This is especially
+  // noticable when your profile is empty.
+  if (top_sites)
     top_sites->AddObserver(this);
-  }
+
   tab_restore_service->AddObserver(this);

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

please add a doc comment explaining why the TRS is being observed.

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

Done.

   pref_change_registrar_.reset(new PrefChangeRegistrar);

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

please add a doc comment explaining why kIncognitoModeAvailability is being
monitored for changes.

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

Done.

   pref_change_registrar_->Init(profile_->GetPrefs());
   pref_change_registrar_->Add(
       prefs::kIncognitoModeAvailability,
       base::Bind(&JumpList::OnIncognitoAvailabilityChanged, this));

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

base::Unretained(this) since you're guaranteed that |this| will outlive
pref_change_registrar_ -- the member's dtor will unregister the pref observer.
please add a comment to this effect as well. for example:
  // base::Unretained is safe since |this| is guaranteed to outlive
pref_change_registrar_.

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

Changed to base::Unretained(this) and comments added.

 }
 
 JumpList::~JumpList() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   Terminate();
 }
 
 // static
 bool JumpList::Enabled() {
   return JumpListUpdater::IsEnabled();
 }
 
 void JumpList::CancelPendingUpdate() {

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

i think this should also cancel a pending MostVisitedURLs fetch via:
  weak_ptr_factory_.InvalidateWeakPtrs();
otherwise, one or more OnMostVisitedURLsAvailable calls could arrive, kicking
off another favicon fetch. i don't think this is desired. see comment in
DeferredTopSitesChanged for more details.

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

Thanks for the reminder! I've updated code to invalidate the weak pointer.

   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (task_id_ != base::CancelableTaskTracker::kBadTaskId) {
     cancelable_task_tracker_.TryCancel(task_id_);
     task_id_ = base::CancelableTaskTracker::kBadTaskId;
   }
 }
 
 void JumpList::Terminate() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  timer_most_visited_.Stop();
-  timer_recently_closed_.Stop();
+  timer_.Stop();
   CancelPendingUpdate();
   if (profile_) {

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

  update_in_progress_ = false;
just before this line

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

Done.

     sessions::TabRestoreService* tab_restore_service =
         TabRestoreServiceFactory::GetForProfile(profile_);
     if (tab_restore_service)
       tab_restore_service->RemoveObserver(this);
     scoped_refptr<history::TopSites> top_sites =
         TopSitesFactory::GetForProfile(profile_);
     if (top_sites)
       top_sites->RemoveObserver(this);
     pref_change_registrar_.reset();
   }
   profile_ = NULL;

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

nit: nullptr throughout

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

Done.

 }
 
 void JumpList::ShutdownOnUIThread() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   Terminate();
 }
 
 void JumpList::OnMostVisitedURLsAvailable(
     const history::MostVisitedURLList& urls) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
+  // There is no need to update the JumpList if the top most visited sites in
+  // display have not changed.
+  if (MostVisitedItemsUnchanged(most_visited_pages_, urls, kMostVisitedItems))
+    return;
 
-    // There is no need to update the JumpList if the top most visited sites in
-    // display have not changed.
-    if (MostVisitedItemsUnchanged(data->most_visited_pages_, urls,
-                                  kMostVisitedItems)) {
-      return;
-    }
+  most_visited_pages_.clear();
 

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

  const size_t num_items = std::min(urls.size(), kMostVisitedItems);
  most_visited_pages_.reserve(num_items);
  icon_urls_.reserve(icon_urls_.size() + num_items);

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

I introduced num_items to avoid duplicated value comparison in the for-loop.
However, most_visited_pages_ and icon_urls_ are std::list rather than
std::vector, so reserve() is not applicable here. 

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

:-)

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

Acknowledged.

-    data->most_visited_pages_.clear();
+  for (size_t i = 0; i < urls.size() && i < kMostVisitedItems; i++) {

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

...; i < num_items; ++i) {

(note: always prefer pre-increment)

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

Done.

+    const history::MostVisitedURL& url = urls[i];
+    scoped_refptr<ShellLinkItem> link = CreateShellLink();
+    std::string url_string = url.url.spec();
+    base::string16 url_string_wide = base::UTF8ToUTF16(url_string);
+    link->GetCommandLine()->AppendArgNative(url_string_wide);
+    link->GetCommandLine()->AppendSwitchASCII(switches::kWinJumplistAction,
+                                              jumplist::kMostVisitedCategory);
+    link->set_title(!url.title.empty() ? url.title : url_string_wide);
+    link->set_url(url_string);
+    most_visited_pages_.push_back(link);
+    icon_urls_.push_back(std::make_pair(url_string, link));

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

  icon_urls_.emplace_back(std::move(url_string), std::move(link));

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

Done.

+  }
 
-    for (size_t i = 0; i < urls.size() && i < kMostVisitedItems; i++) {
-      const history::MostVisitedURL& url = urls[i];
-      scoped_refptr<ShellLinkItem> link = CreateShellLink();
-      std::string url_string = url.url.spec();
-      base::string16 url_string_wide = base::UTF8ToUTF16(url_string);
-      link->GetCommandLine()->AppendArgNative(url_string_wide);
-      link->GetCommandLine()->AppendSwitchASCII(
-          switches::kWinJumplistAction, jumplist::kMostVisitedCategory);
-      link->set_title(!url.title.empty() ? url.title : url_string_wide);
-      link->set_url(url_string);
-      data->most_visited_pages_.push_back(link);
-      data->icon_urls_.push_back(std::make_pair(url_string, link));
-    }
-    data->most_visited_pages_have_updates_ = true;
-  }
+  most_visited_pages_have_updates_ = true;
 
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
 
 void JumpList::TabRestoreServiceChanged(sessions::TabRestoreService* service) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // if we have a pending favicon request, cancel it here (it is out of date).
+  recently_closed_has_pending_notification_ = true;
+
+  if (update_in_progress_)

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

please document this. for example:
  // Postpone handling this notification until a pending update completes.

and put the same comment in TopSitesChanged

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

Done.

+    return;
+
+  // if we have a pending favicon request, cancel it here as it's out of date.
   CancelPendingUpdate();
 
-  // Initialize the one-shot timer to update the the "Recently Closed" category
-  // in a while. If there is already a request queued then cancel it and post
-  // the new request. This ensures that JumpList update of the "Recently Closed"
-  // category won't happen until there has been a brief quiet period, thus
-  // avoiding update storms.
-  if (timer_recently_closed_.IsRunning()) {
-    timer_recently_closed_.Reset();
+  // 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.
+  if (timer_.IsRunning()) {

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

this block is repeated three times. please pull it out into its own function.
ResetQuietPeriodTimer or something like that would have a nice ring to it.

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

I have added a new InitializeTimerForUpdate method and put everything in it. 

+    timer_.Reset();
   } else {
-    timer_recently_closed_.Start(
+    timer_.Start(
         FROM_HERE, kDelayForJumplistUpdate,
-        base::Bind(&JumpList::DeferredTabRestoreServiceChanged,
-                   base::Unretained(this)));
+        base::Bind(&JumpList::DeferredChanged, base::Unretained(this)));

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

please add a comment such as:
  // base::Unretained is safe since |this| is guaranteed to outlive timer_.

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

Done.

   }
 }
 
 void JumpList::TabRestoreServiceDestroyed(
     sessions::TabRestoreService* service) {}
 
 bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
-                      size_t max_items,
-                      JumpListData* data) {
+                      size_t max_items) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  data->list_lock_.AssertAcquired();
 
-  // This code adds the URL and the title strings of the given tab to |data|.
-  if (data->recently_closed_pages_.size() >= max_items)
+  // This code adds the URL and the title strings of the given tab to the
+  // JumpList variables.
+  if (recently_closed_pages_.size() >= max_items)
     return false;
 
   scoped_refptr<ShellLinkItem> link = CreateShellLink();
   const sessions::SerializedNavigationEntry& current_navigation =
       tab.navigations.at(tab.current_navigation_index);
   std::string url = current_navigation.virtual_url().spec();
   link->GetCommandLine()->AppendArgNative(base::UTF8ToUTF16(url));
   link->GetCommandLine()->AppendSwitchASCII(switches::kWinJumplistAction,
                                             jumplist::kRecentlyClosedCategory);
   link->set_title(current_navigation.title());
   link->set_url(url);
-  data->recently_closed_pages_.push_back(link);
-  data->icon_urls_.push_back(std::make_pair(std::move(url), std::move(link)));
+  recently_closed_pages_.push_back(link);
+  icon_urls_.push_back(std::make_pair(std::move(url), std::move(link)));

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

  icon_urls_.emplace_back(std::move(url), std::move(link));

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

Done.

 
   return true;
 }
 
 void JumpList::AddWindow(const sessions::TabRestoreService::Window& window,
-                         size_t max_items,
-                         JumpListData* data) {
+                         size_t max_items) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  data->list_lock_.AssertAcquired();
 
   // This code enumerates all the tabs in the given window object and add their

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

this comment adds nothing over the doc comment in the .h and the code below.
please remove it.

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

Done.

-  // URLs and titles to |data|.
+  // URLs and titles to the JumpList variables.
   DCHECK(!window.tabs.empty());
 
   for (const auto& tab : window.tabs) {
-    if (!AddTab(*tab, max_items, data))
+    if (!AddTab(*tab, max_items))
       return;
   }
 }
 
 void JumpList::StartLoadingFavicon() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   base::ElapsedTimer timer;

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

do the early return before this line:

  if (icon_urls_.empty()) {
    // No more favicons are needed by the application JumpList. Schedule an
    // update on a background thread.
    PostRunUpdate();
    return;
  }

then the rest of the code is a bit simpler:
  base::ElapsedTimer timer;
  GURL url = GURL(icon_urls_.front().first);
  ...

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

Done.

 
   GURL url;
-  bool waiting_for_icons = true;
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    waiting_for_icons = !data->icon_urls_.empty();
-    if (waiting_for_icons) {
-      // Ask FaviconService if it has a favicon of a URL.
-      // When FaviconService has one, it will call OnFaviconDataAvailable().
-      url = GURL(data->icon_urls_.front().first);
-    }
+
+  bool waiting_for_icons = !icon_urls_.empty();
+  if (waiting_for_icons) {
+    // Ask FaviconService if it has a favicon of a URL.
+    // When FaviconService has one, it will call OnFaviconDataAvailable().
+    url = GURL(icon_urls_.front().first);
   }
 
   if (!waiting_for_icons) {
     // No more favicons are needed by the application JumpList. Schedule a
     // RunUpdateJumpList call.
     PostRunUpdate();
     return;
   }
 
   favicon::FaviconService* favicon_service =
       FaviconServiceFactory::GetForProfile(profile_,
                                            ServiceAccessType::EXPLICIT_ACCESS);
   task_id_ = favicon_service->GetFaviconImageForPageURL(
       url,
       base::Bind(&JumpList::OnFaviconDataAvailable, base::Unretained(this)),

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

please add a comment such as:
  // base::Unretained is safe since |this| is guaranteed to outlive
cancelable_task_tracker_.

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

Done.

       &cancelable_task_tracker_);
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/717236
   UMA_HISTOGRAM_TIMES("WinJumplist.StartLoadingFaviconDuration",
                       timer.Elapsed());
 }
 
 void JumpList::OnFaviconDataAvailable(
     const favicon_base::FaviconImageResult& image_result) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   base::ElapsedTimer timer;
 
   // If there is currently a favicon request in progress, it is now outdated,
   // as we have received another, so nullify the handle from the old request.
   task_id_ = base::CancelableTaskTracker::kBadTaskId;
-  // Lock the list to set icon data and pop the url.
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    // Attach the received data to the ShellLinkItem object.
-    // This data will be decoded by the RunUpdateJumpList
-    // method.
-    if (!image_result.image.IsEmpty() && !data->icon_urls_.empty() &&
-        data->icon_urls_.front().second.get()) {
-      gfx::ImageSkia image_skia = image_result.image.AsImageSkia();
-      image_skia.EnsureRepsForSupportedScales();
-      std::unique_ptr<gfx::ImageSkia> deep_copy(image_skia.DeepCopy());
-      data->icon_urls_.front().second->set_icon_image(*deep_copy);
-    }
 
-    if (!data->icon_urls_.empty())
-      data->icon_urls_.pop_front();
+  // Attach the received data to the ShellLinkItem object. This data will be
+  // decoded by the RunUpdateJumpList method.
+  if (!image_result.image.IsEmpty() && !icon_urls_.empty() &&
+      icon_urls_.front().second.get()) {
+    gfx::ImageSkia image_skia = image_result.image.AsImageSkia();
+    image_skia.EnsureRepsForSupportedScales();
+    std::unique_ptr<gfx::ImageSkia> deep_copy(image_skia.DeepCopy());
+    icon_urls_.front().second->set_icon_image(*deep_copy);
   }
 
+  if (!icon_urls_.empty())

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

this condition is checked twice. how about:
  if (!icon_urls_.empty()) {
    if (!image_result.image.IsEmpty() && icon_urls_.front().second.get()) {
      ...
    }
    icon_urls_.pop_front();
  }

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

Agreed this is the right thing to do. Done.

+    icon_urls_.pop_front();
+
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/717236
   UMA_HISTOGRAM_TIMES("WinJumplist.OnFaviconDataAvailableDuration",
                       timer.Elapsed());
 
   // Check whether we need to load more favicons.
   StartLoadingFavicon();
 }
 
 void JumpList::OnIncognitoAvailabilityChanged() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  bool waiting_for_icons = true;
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    waiting_for_icons = !data->icon_urls_.empty();
-  }
+  bool waiting_for_icons = !icon_urls_.empty();
 
   // Since neither the "Most Visited" category nor the "Recently Closed"
   // category changes, mark the flags so that icon files for those categories
   // won't be updated later on.
   if (!waiting_for_icons)

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

  if (icon_urls_.empty()))

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

Done.

     PostRunUpdate();
 }
 
 void JumpList::PostRunUpdate() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   TRACE_EVENT0("browser", "JumpList::PostRunUpdate");
   if (!profile_)

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

can this ever be true? as long as Terminate() really terminates everything, i'm
not sure how it could be. please remove it unless you can determine an odd
sequence of events that could lead to it being true.

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

I've removed it.

     return;
 
+  update_in_progress_ = true;
+
   base::FilePath profile_dir = profile_->GetPath();
 
   // Check if incognito windows (or normal windows) are disabled by policy.
   IncognitoModePrefs::Availability incognito_availability =
       IncognitoModePrefs::GetAvailability(profile_->GetPrefs());
 
-  // Post a task to update the JumpList, which consists of 1) delete old icons,
-  // 2) create new icons, 3) notify the OS.
-  update_jumplist_task_runner_->PostTask(
+  // Make local copies of JumpList member variables and use them for an update.
+  ShellLinkItemList local_most_visited_pages = most_visited_pages_;
+  ShellLinkItemList local_recently_closed_pages = recently_closed_pages_;
+
+  bool most_visited_pages_have_updates = most_visited_pages_have_updates_;
+  bool recently_closed_pages_have_updates = recently_closed_pages_have_updates_;
+
+  JumpListUpdateResults* update_results = new JumpListUpdateResults();

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

  auto update_results = base::MakeUnique<JumpListUpdateResults>();
and std::move it as needed below.

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

Done.

+  update_results->most_visited_icons_in_update_ = most_visited_icons_;
+  update_results->recently_closed_icons_in_update = recently_closed_icons_;
+
+  // Post a task to update the JumpList, which consists of 1) create new icons,
+  // 2) delete old icons, 3) notify the OS.
+  update_jumplist_task_runner_->PostTaskAndReply(

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

PTAR returns a bool indicating whether or not the task may run at some point in
the future. to handle this case, i suggest:
  if (!update_jumplist_task_runner_->PostTaskAndReply(...)) {
    OnRunUpdateCompletion(base::MakeUnique<JumpListUpdateResults>());
  }
so that the instance resets its state properly.

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

Thanks for the reminder! This is cool!

       FROM_HERE,
-      base::Bind(&JumpList::RunUpdateJumpList, this, incognito_availability,
-                 app_id_, profile_dir, base::RetainedRef(jumplist_data_)));
+      base::Bind(&JumpList::RunUpdateJumpList, app_id_, profile_dir,
+                 local_most_visited_pages, local_recently_closed_pages,
+                 most_visited_pages_have_updates,
+                 recently_closed_pages_have_updates, incognito_availability,
+                 update_results),
+      base::Bind(&JumpList::OnRunUpdateCompletion, base::Unretained(this),

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

base::Unretained -> weak_ptr_factory_.GetWeakPtr()

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

Done.

+                 base::Passed(base::WrapUnique(update_results))));
 
-  // Post a task to delete JumpListIcons folder as it's no longer needed.
-  // Now we have JumpListIconsMostVisited folder and JumpListIconsRecentClosed
-  // folder instead.
+  // Post a task to delete folders JumpListIcons and JumpListIconsOld as they

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

wdyt of moving this stuff down into OnRunUpdateCompletion so that it doesn't
contend with the update?

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

nit: "...to delete the Foo and Bar folders as they..."

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

Comments updated.

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

SGTM. Done.

+  // are no longer needed. Now we have folders JumpListIcons{MostVisited,
+  // RecentClosed} instead.
+
   base::FilePath icon_dir =
       GenerateJumplistIconDirName(profile_dir, FILE_PATH_LITERAL(""));
-
   delete_jumplisticons_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DeleteDirectory, std::move(icon_dir), kFileDeleteLimit));
 
-  // Post a task to delete JumpListIconsOld folder as it's no longer needed.
   base::FilePath icon_dir_old =
       GenerateJumplistIconDirName(profile_dir, FILE_PATH_LITERAL("Old"));
-
   delete_jumplisticons_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DeleteDirectory, std::move(icon_dir_old), kFileDeleteLimit));
 }
 
 void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 }
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
-  // If we have a pending favicon request, cancel it here (it is out of date).
+  most_visited_has_pending_notification_ = true;

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

"Most Visited" is the name of the jumplist section, whereas "Top Sites" is the
thing in the browser that is being observed. I would rename this variable to
top_sites_has_pending_notification_ or top_sites_updated_ (short and sweet). if
you like that, please change recently_closed_has_pending_notification_ to
recently_closed_updated_, too.

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

  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

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

Thanks for the suggestion. I prefer top_sites_has_pending_notification_ though,
because there is already a variable called most_visited_should_update_. TopSites
having notifications doesn't mean most-visited category needs to be updated,
because TopSites cares about the top 24 urls, while the most-visited category
only cares about the top 5. I do need these two sets of flags for different
purposes. I've also renamed other variables accordingly.

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

DCHECK added.

+
+  if (update_in_progress_)
+    return;
+
+  // If we have a pending favicon request, cancel it here as it's out of date.
   CancelPendingUpdate();
 
-  // Initialize the one-shot timer to update the the "Most visited" category in
-  // a while. If there is already a request queued then cancel it and post the
-  // new request. This ensures that JumpList update of the "Most visited"
-  // category won't happen until there has been a brief quiet period, thus
-  // avoiding update storms.
-  if (timer_most_visited_.IsRunning()) {
-    timer_most_visited_.Reset();
+  // 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.
+  if (timer_.IsRunning()) {
+    timer_.Reset();
   } else {
-    timer_most_visited_.Start(
+    timer_.Start(
         FROM_HERE, kDelayForJumplistUpdate,
-        base::Bind(&JumpList::DeferredTopSitesChanged, base::Unretained(this)));
+        base::Bind(&JumpList::DeferredChanged, base::Unretained(this)));
+  }
+}
+
+void JumpList::DeferredChanged() {
+  if (updates_to_skip_ > 0) {

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

  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

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

Done.

+    --updates_to_skip_;
+    return;
+  }
+
+  // Retrieve the recently closed URLs synchronously.
+  if (recently_closed_has_pending_notification_) {
+    recently_closed_has_pending_notification_ = false;
+    DeferredTabRestoreServiceChanged();
+  }
+
+  // If TopSites has updates, retrieve the URLs asynchronously, and on its
+  // completion, trigger favicon loading.
+  // Otherwise, call StartLoadingFavicon directly to start favicon loading.
+  if (most_visited_has_pending_notification_) {
+    most_visited_has_pending_notification_ = false;
+    DeferredTopSitesChanged();
+  } else {
+    StartLoadingFavicon();
   }
 }
 
 void JumpList::DeferredTopSitesChanged() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (updates_to_skip_ > 0) {
-    --updates_to_skip_;
-    return;
-  }
-
   // Opening the first tab in one session triggers a TopSite history sync.
   // Delay this sync till the first tab is closed to allow the "recently closed"
   // category from last session to stay longer.
   if (!has_tab_closed_)
     return;
 
   scoped_refptr<history::TopSites> top_sites =
       TopSitesFactory::GetForProfile(profile_);
   if (top_sites) {
     top_sites->GetMostVisitedURLs(

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

i think you need to do something to deal with an outstanding GetMostVisitedURLs
call so that subsequent TabRestoreServiceChanged or TopSitesChanged
notifications can be handled properly. then, of course, you need to decide how
to handle this case. i can think of a few ways:

- clear most_visited_has_pending_notification_ in OnMostVisitedURLsAvailable
rather than in DeferredChanged. you're then clear to use
weak_ptr_factory_.InvalidateWeakPtrs() in CancelPendingUpdate() (as i suggested
elsewhere) to cancel a previous call to GetMostVisitedURLs. it won't stop the
call, but it'll ignore the result so that you don't end up with two calls to
OnMostVisitedURLsAvailable. this is probably the simplest thing.

- maintain state that there's an outstanding call to GetMostVisitedURLs so that
subsequent calls to DeferredTopSitesChanged don't re-issue a new one. i'm less
hot about this idea.

- maybe there are other ways?

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

Thanks for the detailed suggestions! I'll go with the first idea.

         base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                    weak_ptr_factory_.GetWeakPtr()),
         false);
   }
 }
 
 void JumpList::DeferredTabRestoreServiceChanged() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (updates_to_skip_ > 0) {
-    --updates_to_skip_;
-    return;
-  }
-
-  // Force a TopSite history sync when closing a first tab in one session.
-  if (!has_tab_closed_) {
-    has_tab_closed_ = true;
-    scoped_refptr<history::TopSites> top_sites =
-        TopSitesFactory::GetForProfile(profile_);
-    if (top_sites)
-      top_sites->SyncWithHistory();
-  }
-
   // Create a list of ShellLinkItems from the "Recently Closed" pages.
   // As noted above, we create a ShellLinkItem objects with the following
   // parameters.
   // * arguments
   //   The last URL of the tab object.
   // * title
   //   The title of the last URL.
   // * icon
   //   An empty string. This value is to be updated in OnFaviconDataAvailable().
 
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
 
-  {
-    JumpListData* data = &jumplist_data_->data;
-    base::AutoLock auto_lock(data->list_lock_);
-    data->recently_closed_pages_.clear();
+  recently_closed_pages_.clear();
 
-    for (const auto& entry : tab_restore_service->entries()) {
-      if (data->recently_closed_pages_.size() >= kRecentlyClosedItems)
+  for (const auto& entry : tab_restore_service->entries()) {
+    if (recently_closed_pages_.size() >= kRecentlyClosedItems)
+      break;
+    switch (entry->type) {
+      case sessions::TabRestoreService::TAB:
+        AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
+               kRecentlyClosedItems);
         break;
-      switch (entry->type) {
-        case sessions::TabRestoreService::TAB:
-          AddTab(static_cast<const sessions::TabRestoreService::Tab&>(*entry),
-                 kRecentlyClosedItems, data);
-          break;
-        case sessions::TabRestoreService::WINDOW:
-          AddWindow(
-              static_cast<const sessions::TabRestoreService::Window&>(*entry),
-              kRecentlyClosedItems, data);
-          break;
-      }
+      case sessions::TabRestoreService::WINDOW:
+        AddWindow(
+            static_cast<const sessions::TabRestoreService::Window&>(*entry),
+            kRecentlyClosedItems);
+        break;
     }
-
-    data->recently_closed_pages_have_updates_ = true;
   }
 
-  // Send a query that retrieves the first favicon.
-  StartLoadingFavicon();
+  recently_closed_pages_have_updates_ = true;
+
+  // Force a TopSite history sync when closing a first tab in one session.
+  if (!has_tab_closed_) {
+    has_tab_closed_ = true;
+    scoped_refptr<history::TopSites> top_sites =
+        TopSitesFactory::GetForProfile(profile_);
+    if (top_sites)
+      top_sites->SyncWithHistory();
+  }
 }
 
+// static
 void JumpList::DeleteIconFiles(const base::FilePath& icon_dir,
-                               JumpListCategory category) {
-  base::flat_map<std::string, base::FilePath>* source_map = nullptr;
-  switch (category) {
-    case JumpListCategory::kMostVisited:
-      source_map = &most_visited_icons_;
-      break;
-    case JumpListCategory::kRecentlyClosed:
-      source_map = &recently_closed_icons_;
-      break;
-  }
-
+                               URLIconCache* icon_cache) {
   // Put all cached icon file paths into a set.
   base::flat_set<base::FilePath> cached_files;
-  cached_files.reserve(source_map->size());
+  cached_files.reserve(icon_cache->size());
 
-  for (const auto& url_path_pair : *source_map)
+  for (const auto& url_path_pair : *icon_cache)
     cached_files.insert(url_path_pair.second);
 
   DeleteNonCachedFiles(icon_dir, cached_files);
 }
 
+// static
 int JumpList::CreateIconFiles(const base::FilePath& icon_dir,
                               const ShellLinkItemList& item_list,
                               size_t max_items,
-                              JumpListCategory category) {
+                              URLIconCache* icon_cache) {
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration");
 
   int icons_created = 0;
 
-  // Reuse icons for urls that were already present in the jumplist for this
-  // category.
-
-  base::flat_map<std::string, base::FilePath>* source_map = nullptr;
-  switch (category) {
-    case JumpListCategory::kMostVisited:
-      source_map = &most_visited_icons_;
-      break;
-    case JumpListCategory::kRecentlyClosed:
-      source_map = &recently_closed_icons_;
-      break;
-  }
-
-  base::flat_map<std::string, base::FilePath> updated_map;
-
+  // Reuse icons for urls that already present in the current JumpList.
+  URLIconCache updated_map;
   for (ShellLinkItemList::const_iterator iter = item_list.begin();
        iter != item_list.end() && max_items > 0; ++iter, --max_items) {
     ShellLinkItem* item = iter->get();
-    auto cache_iter = source_map->find(item->url());
-    if (cache_iter != source_map->end()) {
+    auto cache_iter = icon_cache->find(item->url());
+    if (cache_iter != icon_cache->end()) {
       item->set_icon(cache_iter->second.value(), 0);
       updated_map[item->url()] = cache_iter->second;
     } else {
       base::FilePath icon_path;
       if (CreateIconFile(item->icon_image(), icon_dir, &icon_path)) {
         ++icons_created;
         item->set_icon(icon_path.value(), 0);
         updated_map[item->url()] = icon_path;
       }
     }
   }
-  source_map->swap(updated_map);
+  icon_cache->swap(updated_map);
 
   return icons_created;
 }
 
+// static
 int JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
                               const ShellLinkItemList& page_list,
                               size_t slot_limit,
-                              JumpListCategory category) {
+                              URLIconCache* icon_cache) {
   int icons_created = 0;
 
-  // Maximum number of icon files that each JumpList icon folder may hold, which
-  // is set to 2 times the normal amount.
-  size_t icon_limit =
-      2 * ((category == JumpListCategory::kMostVisited) ? kMostVisitedItems
-                                                        : kRecentlyClosedItems);
-
   // Clear the JumpList icon folder at |icon_dir| and the cache when
-  // 1) "Most visited" category updates for the 1st time after Chrome is
-  //     launched. This actually happens right after Chrome is launched.
-  // 2) "Recently closed" category updates for the 1st time after Chrome is
-  //     launched.
-  // 3) The number of icons in |icon_dir| has exceeded the limit.
-  if ((category == JumpListCategory::kMostVisited &&
-       most_visited_icons_.empty()) ||
-      (category == JumpListCategory::kRecentlyClosed &&
-       recently_closed_icons_.empty()) ||
-      FilesExceedLimitInDir(icon_dir, icon_limit)) {
+  // 1) |icon_cache| is empty. This happens when "Most visited" or "Recently
+  //    closed" category updates for the 1st time after Chrome is launched.
+  // 2) The number of icons in |icon_dir| has exceeded the limit.
+  if (icon_cache->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) {
     DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
-    most_visited_icons_.clear();
-    recently_closed_icons_.clear();
+    icon_cache->clear();
     // Create new icons only when the directory exists and is empty.
     if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
       icons_created +=
-          CreateIconFiles(icon_dir, page_list, slot_limit, category);
+          CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
   } else if (base::CreateDirectory(icon_dir)) {
-    icons_created += CreateIconFiles(icon_dir, page_list, slot_limit, category);
-    DeleteIconFiles(icon_dir, category);
+    icons_created +=
+        CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache);
+    DeleteIconFiles(icon_dir, icon_cache);
   }
 
   return icons_created;
 }
 
-bool JumpList::UpdateJumpList(
+// static
+void JumpList::RunUpdateJumpList(
     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) {
+    IncognitoModePrefs::Availability incognito_availability,
+    JumpListUpdateResults* update_results) {
   if (!JumpListUpdater::IsEnabled())
-    return true;
+    return;
+
+  DCHECK(update_results);
 
   JumpListUpdater jumplist_updater(app_id);
 
   base::ElapsedTimer begin_update_timer;
 
-  if (!jumplist_updater.BeginUpdate())
-    return false;
+  if (!jumplist_updater.BeginUpdate()) {
+    update_results->update_success_ = false;
+    return;
+  }
 
   // Discard this JumpList update if JumpListUpdater::BeginUpdate takes longer
   // than the maximum allowed time, as it's very likely the following update
   // steps will also take a long time. As we've not updated the icons on the
   // disk, discarding this update wont't affect the current JumpList used by OS.
   if (begin_update_timer.Elapsed() >= kTimeOutForJumplistBeginUpdate) {
-    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
-    return false;
+    update_results->update_success_ = false;
+    update_results->update_timeout_ = true;
+    return;
   }
 
   // Record the desired number of icons created in this JumpList update.
   int icons_created = 0;
 
   // Update the icons for "Most Visisted" category of the JumpList if needed.
   if (most_visited_pages_have_updates) {
     base::FilePath icon_dir_most_visited = GenerateJumplistIconDirName(
         profile_dir, FILE_PATH_LITERAL("MostVisited"));
 
-    icons_created +=
-        UpdateIconFiles(icon_dir_most_visited, most_visited_pages,
-                        kMostVisitedItems, JumpListCategory::kMostVisited);
+    icons_created += UpdateIconFiles(
+        icon_dir_most_visited, most_visited_pages, kMostVisitedItems,
+        &update_results->most_visited_icons_in_update_);
   }
 
   // Update the icons for "Recently Closed" category of the JumpList if needed.
   if (recently_closed_pages_have_updates) {
     base::FilePath icon_dir_recent_closed = GenerateJumplistIconDirName(
         profile_dir, FILE_PATH_LITERAL("RecentClosed"));
 
     icons_created += UpdateIconFiles(
         icon_dir_recent_closed, recently_closed_pages, kRecentlyClosedItems,
-        JumpListCategory::kRecentlyClosed);
+        &update_results->recently_closed_icons_in_update);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   UMA_HISTOGRAM_COUNTS_100("WinJumplist.CreateIconFilesCount", icons_created);
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
 
   base::ElapsedTimer add_custom_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, kMostVisitedItems)) {
-    return false;
+    update_results->update_success_ = false;
+    return;
   }
 
   // Update the "Recently Closed" category of the JumpList.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED), recently_closed_pages,
           kRecentlyClosedItems)) {
-    return false;
+    update_results->update_success_ = false;
+    return;
   }
 
   // If JumpListUpdater::AddCustomCategory or JumpListUpdater::CommitUpdate
   // takes longer than the maximum allowed time, skip the next
   // |kUpdatesToSkipUnderHeavyLoad| updates. This update should be finished
   // because we've already updated the icons on the disk. If discarding this
   // update from here, some items in the current JumpList may not have icons
   // as they've been delete from the disk. In this case, the background color of
   // the JumpList panel is used instead, which doesn't look nice.
 
   if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory)
-    updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
+    update_results->update_timeout_ = true;
 
   // Update the "Tasks" category of the JumpList.
-  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
-    return false;
+  if (!UpdateTaskCategory(&jumplist_updater, incognito_availability)) {
+    update_results->update_success_ = false;
+    return;
+  }
 
   base::ElapsedTimer commit_update_timer;
 
   // Commit this transaction and send the updated JumpList to Windows.
-  bool commit_result = jumplist_updater.CommitUpdate();
+  if (!jumplist_updater.CommitUpdate())
+    update_results->update_success_ = false;
 
   if (commit_update_timer.Elapsed() >= kTimeOutForJumplistCommitUpdate)
+    update_results->update_timeout_ = true;
+
+  return;

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

omit

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

Done.

+}
+
+void JumpList::OnRunUpdateCompletion(
+    std::unique_ptr<JumpListUpdateResults> update_results) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Update JumpList member variables based on the results from the update run
+  // just finished.
+  if (update_results->update_timeout_)
     updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad;
 
-  return commit_result;
-}
-
-void JumpList::RunUpdateJumpList(
-    IncognitoModePrefs::Availability incognito_availability,
-    const base::string16& app_id,
-    const base::FilePath& profile_dir,
-    base::RefCountedData<JumpListData>* ref_counted_data) {
-  JumpListData* data = &ref_counted_data->data;
-  ShellLinkItemList local_most_visited_pages;
-  ShellLinkItemList local_recently_closed_pages;
-  bool most_visited_pages_have_updates;
-  bool recently_closed_pages_have_updates;
-
-  {
-    base::AutoLock auto_lock(data->list_lock_);
-    // Make sure we are not out of date: if icon_urls_ is not empty, then
-    // another notification has been received since we processed this one
-    if (!data->icon_urls_.empty())
-      return;
-
-    // Make local copies of lists and flags so we can release the lock.
-    local_most_visited_pages = data->most_visited_pages_;
-    local_recently_closed_pages = data->recently_closed_pages_;
-
-    most_visited_pages_have_updates = data->most_visited_pages_have_updates_;
-    recently_closed_pages_have_updates =
-        data->recently_closed_pages_have_updates_;
-
-    // Clear the flags to reflect that we'll take actions on these updates.
-    data->most_visited_pages_have_updates_ = false;
-    data->recently_closed_pages_have_updates_ = false;
+  if (update_results->update_success_) {
+    most_visited_icons_.swap(update_results->most_visited_icons_in_update_);
+    recently_closed_icons_.swap(
+        update_results->recently_closed_icons_in_update);
+    most_visited_pages_have_updates_ = false;
+    recently_closed_pages_have_updates_ = false;
   }
 
-  if (!most_visited_pages_have_updates && !recently_closed_pages_have_updates)
-    return;
+  update_in_progress_ = false;
 
-  // Update the application JumpList. If it fails, reset the flags to true if
-  // they were so that the corresponding JumpList categories will be tried to
-  // update again in the next run.
-  if (!UpdateJumpList(
-          app_id, profile_dir, local_most_visited_pages,
-          local_recently_closed_pages, most_visited_pages_have_updates,
-          recently_closed_pages_have_updates, incognito_availability)) {
-    base::AutoLock auto_lock(data->list_lock_);
-    if (most_visited_pages_have_updates)
-      data->most_visited_pages_have_updates_ = true;
-    if (recently_closed_pages_have_updates)
-      data->recently_closed_pages_have_updates_ = true;
+  // Start another JumpList update if there is any new notification during the
+  // update run just finished.
+  if (most_visited_has_pending_notification_ ||
+      recently_closed_has_pending_notification_) {
+    if (timer_.IsRunning()) {
+      timer_.Reset();
+    } else {
+      timer_.Start(
+          FROM_HERE, kDelayForJumplistUpdate,
+          base::Bind(&JumpList::DeferredChanged, base::Unretained(this)));
+    }
   }
+
+  return;
 }