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..e9bc5f5c34c7f4c2463a0b01e1377b193ca6df2d 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,30 @@ namespace chromeos {
 namespace {
 
 const char* kWhitelistedFiles[] = {
-    "fcc/label.png"
+  "fcc/label.png"
 };
 
+// Checks if the path exists and starts loading the image.
+void StartOnBlockingPool(
+    const std::string& path,
+    scoped_refptr<UserImageLoader> image_loader,
+    const UserImageLoader::LoadedCallback& image_loaded_callback,
+    const content::URLDataSource::GotDataCallback& url_data_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;
+    }
+  }
+
+  url_data_callback.Run(NULL);

[stevenjb, 2014/12/05 19:31:53]

This invokes |url_data_callback| (which is just |callback| below, we should name
these all consistently, e.g. |got_data_callback|) on the Worker thread, whereas
|image_loader| will call |image_loaded_callback| on the thread it was
constructed on (see below) which runs |url_data_callback| on that same thread.

To fix this we should probably use PostTaskAndReplyWithResult and return false
here (and true above), then invoke |url_data_callback| in the reply if the
result is false.

[michaelpg, 2014/12/05 22:42:11]

Done.

+  return;
+}
+
 }  // namespace
 
 ImageSource::ImageSource() : weak_factory_(this) {
@@ -50,12 +73,22 @@ void ImageSource::StartDataRequest(
     callback.Run(NULL);
     return;
   }
-  BrowserThread::PostTask(

[michaelpg, 2014/12/05 09:44:43]

So this was also using the deprecated FILE thread, and also not correct. We need
to create the UserImageLoader on the thread that manages its lifetime (the UI
thread), in part because it retains as reference to the thread it was created
on. This becomes more apparent when we switch to using a sequenced task runner.

Also we don't want to re-create a new UserImageLoader each time an image is
requested.

-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&ImageSource::StartOnFileThread,
+
+  if (!image_loader_) {
+    image_loader_ = new UserImageLoader(ImageDecoder::DEFAULT_CODEC,
+                                        task_runner_);

[stevenjb, 2014/12/05 19:31:53]

Constructing this here means that |callback| will be called on this thread
instead of the file/worker thread (see UserImageLoader() and
foreground_task_runner_). 

This is probably what we actually want, but it means that we should ensure that
|callback| always gets called from this thread.

[michaelpg, 2014/12/05 22:42:11]

Done.

+  }
+
+  UserImageLoader::LoadedCallback image_loaded_callback =
+      base::Bind(&ImageSource::ImageLoaded,
                  weak_factory_.GetWeakPtr(),
-                 path,
-                 callback));
+                 callback);
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&StartOnBlockingPool,
+                                    path,
+                                    image_loader_,
+                                    image_loaded_callback,
+                                    callback));
 }
 
 std::string ImageSource::GetMimeType(const std::string& path) const {
@@ -66,25 +99,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,