Clean up the IconLoader.

chrome/browser/icon_manager.cc

 // Copyright (c) 2011 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/icon_manager.h"
 
 #include <memory>
 #include <tuple>
 
 #include "base/bind.h"
-#include "base/stl_util.h"
 #include "base/task_runner.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 
 namespace {
 
 void RunCallbackIfNotCanceled(
     const base::CancelableTaskTracker::IsCanceledCallback& is_canceled,
     const IconManager::IconRequestCallback& callback,
     gfx::Image* image) {
   if (is_canceled.Run())
     return;
   callback.Run(image);
 }
 
 }  // namespace
 
 struct IconManager::ClientRequest {
   IconRequestCallback callback;
   base::FilePath file_path;
   IconLoader::IconSize size;
 };
 
 IconManager::IconManager() {
 }
 
 IconManager::~IconManager() {

[sky, 2016/12/15 22:36:05]

In reviewing this code it seems like IconManager is racy. In particular
IconManager is not ref counted, but IconLoader is. IconLoader references
IconManager through it's a delegate. This means it's possible for IconLoader to
outlive it's delegate. Seems this function should null out the delegates (and
you should keep the null check in IconLoader::NotifyDelegate).

But, this isn't new. If you could file a bug with todo that would be great.

[Avi (use Gerrit), 2016/12/15 23:01:10]

IconManager is the only user of IconLoader. Why is IconLoader ref counted at
all?

I'm going to land this, but then want to make one more pass at cleanup.

-  base::STLDeleteValues(&icon_cache_);
 }
 
-gfx::Image* IconManager::LookupIconFromFilepath(const base::FilePath& file_name,
+gfx::Image* IconManager::LookupIconFromFilepath(const base::FilePath& file_path,
                                                 IconLoader::IconSize size) {
-  GroupMap::iterator it = group_cache_.find(file_name);
-  if (it != group_cache_.end())
-    return LookupIconFromGroup(it->second, size);
+  auto group_it = group_cache_.find(file_path);
+  if (group_it == group_cache_.end())
+    return nullptr;
 
-  return NULL;
-}
+  CacheKey key(group_it->second, size);
+  auto icon_it = icon_cache_.find(key);
+  if (icon_it == icon_cache_.end())
+    return nullptr;
 
-gfx::Image* IconManager::LookupIconFromGroup(const IconGroupID& group,
-                                             IconLoader::IconSize size) {
-  IconMap::iterator it = icon_cache_.find(CacheKey(group, size));
-  if (it != icon_cache_.end())
-    return it->second;
-
-  return NULL;
+  return icon_it->second.get();
 }
 
 base::CancelableTaskTracker::TaskId IconManager::LoadIcon(
-    const base::FilePath& file_name,
+    const base::FilePath& file_path,
     IconLoader::IconSize size,
     const IconRequestCallback& callback,
     base::CancelableTaskTracker* tracker) {
-  IconLoader* loader = new IconLoader(file_name, size, this);
+  IconLoader* loader = new IconLoader(file_path, size, this);
   loader->AddRef();

[sky, 2016/12/15 22:36:05]

This code is equally error prone. Seems like the map key should be a
scoped_refptr, that way no explicit addref/release. But again, this isn't new.

[Avi (use Gerrit), 2016/12/15 23:01:10]

Will address in followup.

   loader->Start();
 
   base::CancelableTaskTracker::IsCanceledCallback is_canceled;
   base::CancelableTaskTracker::TaskId id =
       tracker->NewTrackedTaskId(&is_canceled);
   IconRequestCallback callback_runner = base::Bind(
       &RunCallbackIfNotCanceled, is_canceled, callback);
 
-  ClientRequest client_request = { callback_runner, file_name, size };
-  requests_[loader] = client_request;
+  requests_[loader] = {callback_runner, file_path, size};
   return id;
 }
 
 // IconLoader::Delegate implementation -----------------------------------------
 
-bool IconManager::OnGroupLoaded(IconLoader* loader,
-                                const IconGroupID& group) {
-  ClientRequests::iterator rit = requests_.find(loader);
-  if (rit == requests_.end()) {
-    NOTREACHED();
-    return false;
-  }
-
-  gfx::Image* result = LookupIconFromGroup(group, rit->second.size);
-  if (!result) {
-    return false;
-  }
-
-  return OnImageLoaded(loader, result, group);
-}
-
-bool IconManager::OnImageLoaded(
-    IconLoader* loader, gfx::Image* result, const IconGroupID& group) {
-  ClientRequests::iterator rit = requests_.find(loader);
+void IconManager::OnImageLoaded(IconLoader* loader,
+                                std::unique_ptr<gfx::Image> result,
+                                const IconLoader::IconGroup& group) {
+  auto request_it = requests_.find(loader);
+  DCHECK(request_it != requests_.end());
 
   // Balances the AddRef() in LoadIcon().
   loader->Release();
 
-  // Look up our client state.
-  if (rit == requests_.end()) {
-    NOTREACHED();
-    return false;  // Return false to indicate result should be deleted.
-  }
+  const ClientRequest& client_request = request_it->second;
 
-  const ClientRequest& client_request = rit->second;
-
-  // Cache the bitmap. Watch out: |result| may be NULL to indicate a current
-  // failure. We assume that if we have an entry in |icon_cache_|
-  // it must not be NULL.
+  // Cache the bitmap. Watch out: |result| may be null, which indicates a
+  // failure. We assume that if we have an entry in |icon_cache_| it must not be
+  // null.
   CacheKey key(group, client_request.size);
-  IconMap::iterator it = icon_cache_.find(key);
-  if (it != icon_cache_.end()) {
-    if (!result) {
-      delete it->second;
-      icon_cache_.erase(it);
-    } else if (result != it->second) {
-      it->second->SwapRepresentations(result);
-      delete result;
-      result = it->second;
-    }
-  } else if (result) {
-    icon_cache_[key] = result;
+  if (result) {
+    client_request.callback.Run(result.get());
+    icon_cache_[key] = std::move(result);
+  } else {
+    client_request.callback.Run(nullptr);
+    icon_cache_.erase(key);
   }
 
   group_cache_[client_request.file_path] = group;
 
-  // Inform our client that the request has completed.
-  client_request.callback.Run(result);
-  requests_.erase(rit);
-
-  return true;  // Indicates we took ownership of result.
+  requests_.erase(request_it);
 }
 
-IconManager::CacheKey::CacheKey(const IconGroupID& group,
+IconManager::CacheKey::CacheKey(const IconLoader::IconGroup& group,
                                 IconLoader::IconSize size)
-    : group(group),
-      size(size) {
-}
+    : group(group), size(size) {}
 
 bool IconManager::CacheKey::operator<(const CacheKey &other) const {
   return std::tie(group, size) < std::tie(other.group, other.size);
 }