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..9c9c7da868eee50f63d842e5218e0c966a8d1e54 100644
--- a/chrome/browser/ui/webui/chromeos/image_source.cc
+++ b/chrome/browser/ui/webui/chromeos/image_source.cc
@@ -8,9 +8,11 @@
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/path_service.h"
 #include "base/sequenced_task_runner.h"
 #include "chrome/browser/chromeos/login/users/avatar/user_image_loader.h"
 #include "chrome/common/url_constants.h"
+#include "chromeos/chromeos_paths.h"
 #include "components/user_manager/user_image/user_image.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/mime_util.h"
@@ -21,9 +23,29 @@ namespace chromeos {
 namespace {
 
 const char* kWhitelistedFiles[] = {
-    "fcc/label.png"
+  "fcc/label.png"
 };
 
+// Checks if the image can be found, and starts loading the image if so.
+// Returns true if the image is found and false otherwise.

[stevenjb, 2014/12/05 23:28:07]

More relevant is that it returns true if it starts loading the image. Comment
could just be something like:

// Searches the shared assets directory for the image. If found,
// starts loading the image and returns true, otherwise returns false.

[michaelpg, 2014/12/08 00:58:19]

Done.

+bool StartOnBlockingPool(
+    const std::string& path,
+    scoped_refptr<UserImageLoader> image_loader,
+    const UserImageLoader::LoadedCallback& image_loaded_callback) {
+  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+
+  base::FilePath shared_assets_dir;
+  if (PathService::Get(DIR_SHARED_ASSETS, &shared_assets_dir)) {
+    base::FilePath file_path = shared_assets_dir.AppendASCII(path);
+    if (base::PathExists(file_path)) {
+      image_loader->Start(file_path.value(), 0, image_loaded_callback);
+      return true;
+    }
+  }
+
+  return false;
+}
+
 }  // namespace
 
 ImageSource::ImageSource() : weak_factory_(this) {
@@ -45,17 +67,31 @@ 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,
+
+  if (!image_loader_) {
+    image_loader_ = new UserImageLoader(ImageDecoder::DEFAULT_CODEC,
+                                        task_runner_);
+  }
+
+  UserImageLoader::LoadedCallback image_loaded_callback =
+      base::Bind(&ImageSource::ImageLoaded,
                  weak_factory_.GetWeakPtr(),
+                 got_data_callback);
+  base::PostTaskAndReplyWithResult(
+      content::BrowserThread::GetBlockingPool(),
+      FROM_HERE,
+      base::Bind(&StartOnBlockingPool,
                  path,
-                 callback));
+                 image_loader_,
+                 image_loaded_callback),
+      base::Bind(&ImageSource::LoadImageAttempted,
+                 weak_factory_.GetWeakPtr(),
+                 got_data_callback));
 }
 
 std::string ImageSource::GetMimeType(const std::string& path) const {
@@ -66,33 +102,20 @@ 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::LoadImageAttempted(
+    const content::URLDataSource::GotDataCallback& got_data_callback,
+    bool load_started) {
+  if (!load_started)
+    got_data_callback.Run(NULL);

[stevenjb, 2014/12/05 23:28:07]

This could be an anonymous non-member function. |got_data_callback| should make
its own lifetime assurances (i.e. it shouldn't rely on cancellation on
destruction of this class, it should be safe to call got_data_callback.Run() at
any time).

[michaelpg, 2014/12/08 00:58:19]

Done, along with ImageLoaded.

 }
 
 void ImageSource::ImageLoaded(
-    const content::URLDataSource::GotDataCallback& callback,
+    const content::URLDataSource::GotDataCallback& got_data_callback,
     const user_manager::UserImage& user_image) const {
   if (user_image.has_raw_image())
-    callback.Run(new base::RefCountedBytes(user_image.raw_image()));
+    got_data_callback.Run(new base::RefCountedBytes(user_image.raw_image()));
   else
-    callback.Run(NULL);
+    got_data_callback.Run(NULL);
 }
 
 bool ImageSource::IsWhitelisted(const std::string& path) const {