[App banners] Start addressing race conditions

chrome/browser/android/banners/app_banner_manager.cc

 // Copyright 2014 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/android/banners/app_banner_manager.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 27 matching lines @@
 #include "ui/gfx/screen.h"
 
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF8ToJavaString;
 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 {
 
 AppBannerManager::AppBannerManager(JNIEnv* env, jobject obj)
     : weak_java_banner_view_manager_(env, obj), weak_factory_(this) {
 }
 
 AppBannerManager::~AppBannerManager() {
 }
 
 void AppBannerManager::Destroy(JNIEnv* env, jobject obj) {
   delete this;
 }
 
 void AppBannerManager::ReplaceWebContents(JNIEnv* env,
                                           jobject obj,
                                           jobject jweb_contents) {
   content::WebContents* web_contents =
       content::WebContents::FromJavaWebContents(jweb_contents);
   Observe(web_contents);
 }
 
 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();
   native_app_package_ = std::string();
 }
 
 void AppBannerManager::DidFinishLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url) {
   if (render_frame_host->GetParent())
@@ skipping 79 matching lines @@
 bool AppBannerManager::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(AppBannerManager, message)
     IPC_MESSAGE_HANDLER(ChromeViewHostMsg_DidRetrieveMetaTagContent,
                         OnDidRetrieveMetaTagContent)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   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;
   }
 
   JNIEnv* env = base::android::AttachCurrentThread();
   ScopedJavaLocalRef<jobject> jobj = weak_java_banner_view_manager_.get(env);
   if (jobj.is_null())
     return;
 
   InfoBarService* service = InfoBarService::FromWebContents(web_contents());
 
   AppBannerInfoBar* weak_infobar_ptr = nullptr;
   if (!native_app_data_.is_null()) {
     RecordCouldShowBanner(native_app_package_);
     if (!CheckIfShouldShow(native_app_package_))
       return;
 
     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()){
     RecordCouldShowBanner(web_app_data_.start_url.spec());
     if (!CheckIfShouldShow(web_app_data_.start_url.spec()))
       return;
 
     weak_infobar_ptr = AppBannerInfoBarDelegate::CreateForWebApp(
         service,
         app_title_,
-        new SkBitmap(*bitmap),
+        new SkBitmap(*icon),
         web_app_data_);
   }
 
   if (weak_infobar_ptr != nullptr)
     banners::TrackDisplayEvent(DISPLAY_CREATED);
 }
 
 void AppBannerManager::OnDidRetrieveMetaTagContent(
     bool success,
     const std::string& tag_name,
@@ skipping 33 matching lines @@
     return false;
 
   std::string image_url = ConvertJavaStringToUTF8(env, jicon_url);
   app_title_ = ConvertJavaStringToUTF16(env, japp_title);
   native_app_package_ = ConvertJavaStringToUTF8(env, japp_package);
   native_app_data_.Reset(env, japp_data);
   return FetchIcon(GURL(image_url));
 }
 
 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) {
   if (!web_contents())
     return false;
 
   // 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,
       net::LOAD_NORMAL);
   app_icon_url_ = image_url;
   return true;
 }
 
 int AppBannerManager::GetPreferredIconSize() {
   JNIEnv* env = base::android::AttachCurrentThread();
@@ skipping 30 matching lines @@
 void SetTimeDeltaForTesting(JNIEnv* env, jclass clazz, jint days) {
   gTimeDeltaForTesting = base::TimeDelta::FromDays(days);
 }
 
 // Register native methods
 bool RegisterAppBannerManager(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace banners