Fix a data race on the ref count of AddToHomescreenDataFetcher

chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc

Index: chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
diff --git a/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc b/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
index 576bbc3a43e1e97215e3cc8578a12e86669ff957..91c2491b31ae2641202158b2030e152639f001b6 100644
--- a/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
+++ b/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
@@ -74,6 +74,10 @@ AddToHomescreenDataFetcher::AddToHomescreenDataFetcher(
     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())),
@@ -286,14 +290,15 @@ void AddToHomescreenDataFetcher::OnFaviconFetched(
   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());
 
@@ -304,19 +309,19 @@ void AddToHomescreenDataFetcher::CreateLauncherIconFromFaviconInBackground(
   }
 
   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());
 
@@ -330,10 +335,7 @@ void AddToHomescreenDataFetcher::CreateLauncherIconInBackground(
   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) {