Fix a data race on the ref count of AddToHomescreenDataFetcher

chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc

 // Copyright 2015 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/webapps/add_to_homescreen_data_fetcher.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/location.h"

[dominickn, 2017/02/16 23:24:45]

Nit: #include "base/task_runner_util.h" for base::PostTaskAndReplyWithResult

[tzik, 2017/02/17 04:58:20]

Done.

 #include "base/strings/string16.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/android/offline_pages/offline_page_utils.h"
 #include "chrome/browser/android/shortcut_helper.h"
 #include "chrome/browser/android/webapk/webapk_web_manifest_checker.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/installable/installable_manager.h"
 #include "chrome/browser/manifest/manifest_icon_selector.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
@@ skipping 45 matching lines @@
 AddToHomescreenDataFetcher::AddToHomescreenDataFetcher(
     content::WebContents* web_contents,
     int ideal_icon_size_in_px,
     int minimum_icon_size_in_px,
     int ideal_splash_image_size_in_px,
     int minimum_splash_image_size_in_px,
     int badge_size_in_px,
     bool check_webapk_compatibility,
     Observer* observer)
     : WebContentsObserver(web_contents),
+      background_task_runner_(
+          content::BrowserThread::GetBlockingPool()
+              ->GetTaskRunnerWithShutdownBehavior(
+                  base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
       weak_observer_(observer),
       shortcut_info_(GetShortcutUrl(web_contents->GetBrowserContext(),
                                     web_contents->GetLastCommittedURL())),
       data_timeout_timer_(false, false),
       ideal_icon_size_in_px_(ideal_icon_size_in_px),
       minimum_icon_size_in_px_(minimum_icon_size_in_px),
       ideal_splash_image_size_in_px_(ideal_splash_image_size_in_px),
       minimum_splash_image_size_in_px_(minimum_splash_image_size_in_px),
       badge_size_in_px_(badge_size_in_px),
       check_webapk_compatibility_(check_webapk_compatibility),
@@ skipping 192 matching lines @@
       shortcut_info_.url, icon_types, threshold_to_get_any_largest_icon,
       base::Bind(&AddToHomescreenDataFetcher::OnFaviconFetched, this),
       &favicon_task_tracker_);
 }
 
 void AddToHomescreenDataFetcher::OnFaviconFetched(
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   if (!web_contents() || !weak_observer_ || is_icon_saved_)
     return;
 
-  content::BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior(
-      FROM_HERE, base::Bind(&AddToHomescreenDataFetcher::
-                                CreateLauncherIconFromFaviconInBackground,
-                            this, bitmap_result),
-      base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+  base::PostTaskAndReplyWithResult(
+      background_task_runner_.get(), FROM_HERE,
+      base::Bind(&AddToHomescreenDataFetcher::
+                     CreateLauncherIconFromFaviconInBackground,
+                 base::Unretained(this), bitmap_result),

[dominickn, 2017/02/16 23:24:45]

Is it safe to use base::Unretained here? AddToHomescreenDataFetcher can be
destroyed by the user dismissing the add to homescreen dialog in Java, which
destroys the AddToHomescreenManager object which carries the owning
scoped_refptr to this.

I think it's possible for that to happen whilst the image processing is
happening on the background thread, leading to a method being called on a
released object when the reply is called.

Or will the destruction of the task runner in this object also ensure that the
task is safely canceled if this object is gone (though it's a scoped_refptr so
probably it won't be destroyed and the task still run)?

[tzik, 2017/02/17 04:58:20]

Yes, it's safe, since a reference is implicitly retained by the Reply part of
Callback.
Added RetainedRef annotation here to make it more readable.

+      base::Bind(&AddToHomescreenDataFetcher::NotifyObserver, this));
 }
 
-void AddToHomescreenDataFetcher::CreateLauncherIconFromFaviconInBackground(
+SkBitmap AddToHomescreenDataFetcher::CreateLauncherIconFromFaviconInBackground(
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
 
   SkBitmap raw_icon;
   if (bitmap_result.is_valid()) {
     gfx::PNGCodec::Decode(bitmap_result.bitmap_data->front(),
                           bitmap_result.bitmap_data->size(), &raw_icon);
   }
 
   shortcut_info_.best_primary_icon_url = bitmap_result.icon_url;
-  CreateLauncherIconInBackground(raw_icon);
+  return CreateLauncherIconInBackground(raw_icon);
 }
 
 void AddToHomescreenDataFetcher::CreateLauncherIcon(const SkBitmap& raw_icon) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  content::BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior(
-      FROM_HERE,
+  base::PostTaskAndReplyWithResult(
+      background_task_runner_.get(), FROM_HERE,
       base::Bind(&AddToHomescreenDataFetcher::CreateLauncherIconInBackground,
-                 this, raw_icon),
-      base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+                 base::Unretained(this), raw_icon),
+      base::Bind(&AddToHomescreenDataFetcher::NotifyObserver, this));
 }
 
-void AddToHomescreenDataFetcher::CreateLauncherIconInBackground(
+SkBitmap AddToHomescreenDataFetcher::CreateLauncherIconInBackground(
     const SkBitmap& raw_icon) {
   DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
 
   SkBitmap primary_icon;
   bool is_generated = false;
   if (weak_observer_) {
     primary_icon = weak_observer_->FinalizeLauncherIconInBackground(
         raw_icon, shortcut_info_.url, &is_generated);
   }
 
   if (is_generated)
     shortcut_info_.best_primary_icon_url = GURL();
 
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&AddToHomescreenDataFetcher::NotifyObserver, this,
-                 primary_icon));
+  return primary_icon;
 }
 
 void AddToHomescreenDataFetcher::NotifyObserver(const SkBitmap& primary_icon) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!web_contents() || !weak_observer_ || is_icon_saved_)
     return;
 
   is_icon_saved_ = true;
   primary_icon_ = primary_icon;
   is_ready_ = true;
   weak_observer_->OnDataAvailable(shortcut_info_, primary_icon_, badge_icon_);
 }