[App banners] Start addressing race conditions

chrome/browser/android/banners/app_banner_manager.cc

Index: chrome/browser/android/banners/app_banner_manager.cc
diff --git a/chrome/browser/android/banners/app_banner_manager.cc b/chrome/browser/android/banners/app_banner_manager.cc
index 0f0dcb56ae3f0191bd1af44bada9142329f874aa..4eff4b9565e06b3d678e0314817b1a526eff421c 100644
--- a/chrome/browser/android/banners/app_banner_manager.cc
+++ b/chrome/browser/android/banners/app_banner_manager.cc
@@ -45,6 +45,44 @@ using base::android::ConvertUTF16ToJavaString;
 namespace {
 const char kBannerTag[] = "google-play-id";
 base::TimeDelta gTimeDeltaForTesting;
+}  // namespace
+
+// Fetches a bitmap and deletes itself when completed.
+class BannerBitmapFetcher : public chrome::BitmapFetcher,
+                            public chrome::BitmapFetcherDelegate {
+ public:
+  BannerBitmapFetcher(const GURL& image_url,
+                      banners::AppBannerManager* delegate);
+
+  // Prevents informing the AppBannerManager that the fetch has completed.
+  void Cancel();

[benwells, 2015/02/16 01:58:43]

This doesn't appear to be called by anything.

[gone, 2015/02/17 22:56:52]

Fixed, good catch.

+
+  // chrome::BitmapFetcherDelegate overrides
+  void OnFetchComplete(const GURL url, const SkBitmap* icon) override;
+
+ private:
+  banners::AppBannerManager* delegate_;
+  bool is_cancelled_;
+};
+
+BannerBitmapFetcher::BannerBitmapFetcher(
+    const GURL& image_url,
+    banners::AppBannerManager* delegate)
+    : chrome::BitmapFetcher(image_url, this),
+      delegate_(delegate),
+      is_cancelled_(false) {
+}
+
+void BannerBitmapFetcher::Cancel() {
+  is_cancelled_ = true;
+}
+
+void BannerBitmapFetcher::OnFetchComplete(const GURL url,
+                                          const SkBitmap* icon) {
+  if (!is_cancelled_)
+    delegate_->OnFetchComplete(this, url, icon);
+
+  delete this;
 }
 
 namespace banners {
@@ -72,7 +110,6 @@ void AppBannerManager::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
   // Clear current state.
-  fetcher_.reset();
   app_title_ = base::string16();
   web_app_data_ = content::Manifest();
   native_app_data_.Reset();
@@ -172,9 +209,19 @@ bool AppBannerManager::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void AppBannerManager::OnFetchComplete(const GURL url, const SkBitmap* bitmap) {
-  fetcher_.reset();
-  if (!bitmap || url != app_icon_url_) {
+void AppBannerManager::OnFetchComplete(BannerBitmapFetcher* fetcher,
+                                       const GURL url,
+                                       const SkBitmap* icon) {
+  for (BitmapFetcherVector::iterator itr = active_fetchers_.begin();
+      itr != active_fetchers_.end();
+      ++itr) {
+    if (*itr == fetcher) {
+      active_fetchers_.erase(itr);
+      break;
+    }
+ }
+
+  if (!icon || url != app_icon_url_) {
     DVLOG(1) << "Failed to retrieve image: " << url;

[benwells, 2015/02/16 01:58:43]

Nit: This log message is kind of inaccurate if url != app_iconurl_.

[gone, 2015/02/17 22:56:52]

Just removed the message.  It's not useful in the current pipeline.

     return;
   }
@@ -195,7 +242,7 @@ void AppBannerManager::OnFetchComplete(const GURL url, const SkBitmap* bitmap) {
     weak_infobar_ptr = AppBannerInfoBarDelegate::CreateForNativeApp(
         service,
         app_title_,
-        new SkBitmap(*bitmap),
+        new SkBitmap(*icon),
         native_app_data_,
         native_app_package_);
   } else if (!web_app_data_.IsEmpty()){
@@ -206,7 +253,7 @@ void AppBannerManager::OnFetchComplete(const GURL url, const SkBitmap* bitmap) {
     weak_infobar_ptr = AppBannerInfoBarDelegate::CreateForWebApp(
         service,
         app_title_,
-        new SkBitmap(*bitmap),
+        new SkBitmap(*icon),
         web_app_data_);
   }
 
@@ -260,7 +307,7 @@ bool AppBannerManager::OnAppDetailsRetrieved(JNIEnv* env,
 }
 
 int AppBannerManager::GetNumActiveFetchers(JNIEnv* env, jobject obj) {
-  return fetcher_.get() ? 1 : 0;
+  return active_fetchers_.size();

[benwells, 2015/02/16 01:58:43]

As far as I can tell, this is the only reason to have active_fetchers_. Could
you just keep track of a count instead, and count++ and count-- in the places
you currently add / remove to this list?

If you think you need the container, you could use a set instead as it doesn't
appear the ordering is necessary.

[gone, 2015/02/17 22:56:52]

Started going the set route and realized that a weakly held pointer accomplished
the same thing.  Revamped in light of that.

 }
 
 bool AppBannerManager::FetchIcon(const GURL& image_url) {
@@ -270,8 +317,9 @@ bool AppBannerManager::FetchIcon(const GURL& image_url) {
   // Begin asynchronously fetching the app icon.
   Profile* profile =
       Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  fetcher_.reset(new chrome::BitmapFetcher(image_url, this));
-  fetcher_.get()->Start(
+  BannerBitmapFetcher* fetcher = new BannerBitmapFetcher(image_url, this);
+  active_fetchers_.push_back(fetcher);
+  fetcher->Start(
       profile->GetRequestContext(),
       std::string(),
       net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE,