Fix threading bugs in product label UI.

chrome/browser/ui/webui/chromeos/image_source.cc

Index: chrome/browser/ui/webui/chromeos/image_source.cc
diff --git a/chrome/browser/ui/webui/chromeos/image_source.cc b/chrome/browser/ui/webui/chromeos/image_source.cc
index 726e9f6e371ff133ecb85c3cd13edbcc3c20c635..2d807107a476f4760643c412d1918de6f26a8ec5 100644
--- a/chrome/browser/ui/webui/chromeos/image_source.cc
+++ b/chrome/browser/ui/webui/chromeos/image_source.cc
@@ -21,9 +21,46 @@ namespace chromeos {
 namespace {
 
 const char* kWhitelistedFiles[] = {
-    "fcc/label.png"
+  "fcc/label.png"
 };
 
+// Looks for the image at |path| under the shared assets directory. If found,
+// starts loading the image and returns true. Otherwise, returns false.
+bool StartOnBlockingPool(
+    const std::string& path,
+    scoped_refptr<UserImageLoader> image_loader,
+    const UserImageLoader::LoadedCallback& image_loaded_callback) {
+  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+
+  const base::FilePath asset_dir(FILE_PATH_LITERAL(chrome::kChromeOSAssetPath));
+  const base::FilePath image_path = asset_dir.AppendASCII(path);
+  if (base::PathExists(image_path)) {
+    image_loader->Start(image_path.value(), 0, image_loaded_callback);
+    return true;
+  }
+
+  return false;
+}
+
+// Callback after attempting to start loading the image. Runs
+// |got_data_callback| if loading wasn't started.
+void LoadImageAttempted(
+    const content::URLDataSource::GotDataCallback& got_data_callback,
+    bool load_started) {

[satorux1, 2014/12/09 05:56:40]

maybe add a DCHECK to ensure this runs on the UI thread?

[michaelpg, 2014/12/10 00:48:51]

fn removed

+  if (!load_started)
+    got_data_callback.Run(NULL);
+}
+
+// Callback for user_manager::UserImageLoader.
+void ImageLoaded(

[satorux1, 2014/12/09 05:56:40]

maybe add a DCHECK to ensure this runs on the UI thread? image_loader->Start()
will run the callback on the thread where the image loader was created, but it's
not obvious (in fact I had to read the code of user_image_loader.cc)

[michaelpg, 2014/12/10 00:48:51]

Done.

+    const content::URLDataSource::GotDataCallback& got_data_callback,
+    const user_manager::UserImage& user_image) {
+  if (user_image.has_raw_image())
+    got_data_callback.Run(new base::RefCountedBytes(user_image.raw_image()));
+  else
+    got_data_callback.Run(NULL);
+}
+
 }  // namespace
 
 ImageSource::ImageSource() : weak_factory_(this) {
@@ -45,17 +82,27 @@ void ImageSource::StartDataRequest(
     const std::string& path,
     int render_process_id,
     int render_frame_id,
-    const content::URLDataSource::GotDataCallback& callback) {
+    const content::URLDataSource::GotDataCallback& got_data_callback) {
   if (!IsWhitelisted(path)) {
-    callback.Run(NULL);
+    got_data_callback.Run(NULL);
     return;
   }
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&ImageSource::StartOnFileThread,
-                 weak_factory_.GetWeakPtr(),
+
+  if (!image_loader_) {
+    image_loader_ = new UserImageLoader(ImageDecoder::DEFAULT_CODEC,
+                                        task_runner_);
+  }
+
+  UserImageLoader::LoadedCallback image_loaded_callback =
+      base::Bind(&ImageLoaded, got_data_callback);
+  base::PostTaskAndReplyWithResult(
+      content::BrowserThread::GetBlockingPool(),
+      FROM_HERE,
+      base::Bind(&StartOnBlockingPool,
                  path,
-                 callback));
+                 image_loader_,
+                 image_loaded_callback),
+      base::Bind(LoadImageAttempted, got_data_callback));

[satorux1, 2014/12/09 05:56:40]

got_data_callback can be called either from LoadImageAttempted() or 
ImageLoaded() which I found rather confusing. Seems to me that
PostTaskAndReplyWithResult() is not a good tool here. How about using PostTask()
like:

base::PostTask(
  ...
  base::Bind(&StartOnBlockingPool, path, got_data_callback,
             base::MessageLoopProxy::current()));

In StartOnBlockingPool(), you can use the message loop proxy to post a task to
the original thread, like:

if (base::PathExists(image_path)) {
  image_loader->Start(...);
} else {
  message_loop_proxy->PostTask(..., base::Bind(got_data_dallback, NULL);
}

[michaelpg, 2014/12/10 00:48:51]

I could not find a way to run base::Bind on got_data_callback like in your
example. Is it less confusing if I get rid of LoadImageAttempted and pass an
empty UserImage (which should be cheap) to ImageLoaded instead?

Or, should it be possible to create a Callback by binding another Callback's Run
method to itself? Something like:

base::Bind(&content::URLDataSource::GotDataCallback::Run,
base::Unretained(&got_data_callback), NULL)

gives me something I can't really parse: http://pastebin.com/0Tqt3Gx0

[satorux1, 2014/12/10 01:18:17]

Try passing nullptr instead of NULL.

[michaelpg, 2014/12/10 01:45:11]

Done.

 }
 
 std::string ImageSource::GetMimeType(const std::string& path) const {
@@ -66,35 +113,6 @@ std::string ImageSource::GetMimeType(const std::string& path) const {
   return mime_type;
 }
 
-void ImageSource::StartOnFileThread(
-    const std::string& path,
-    const content::URLDataSource::GotDataCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  base::FilePath file_path(chrome::kChromeOSAssetPath + path);
-  if (!base::PathExists(file_path)) {
-    callback.Run(NULL);
-    return;
-  }
-
-  image_loader_ = new UserImageLoader(ImageDecoder::DEFAULT_CODEC,
-                                      task_runner_);
-  image_loader_->Start(file_path.value(),
-                       0,
-                       base::Bind(&ImageSource::ImageLoaded,
-                                  weak_factory_.GetWeakPtr(),
-                                  callback));
-}
-
-void ImageSource::ImageLoaded(
-    const content::URLDataSource::GotDataCallback& callback,
-    const user_manager::UserImage& user_image) const {
-  if (user_image.has_raw_image())
-    callback.Run(new base::RefCountedBytes(user_image.raw_image()));
-  else
-    callback.Run(NULL);
-}
-
 bool ImageSource::IsWhitelisted(const std::string& path) const {
   const char** end = kWhitelistedFiles + arraysize(kWhitelistedFiles);
   return std::find(kWhitelistedFiles, end, path) != end;