Fix threading bugs in product label UI.

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

 // Copyright 2014 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/ui/webui/chromeos/image_source.h"
 
 #include "base/bind.h"
 #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"
 
 using content::BrowserThread;
 
 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) {
   base::SequencedWorkerPool* blocking_pool =
       BrowserThread::GetBlockingPool();
   task_runner_ = blocking_pool->GetSequencedTaskRunnerWithShutdownBehavior(
       blocking_pool->GetSequenceToken(),
       base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
 }
 
 ImageSource::~ImageSource() {
 }
 
 std::string ImageSource::GetSource() const {
   return chrome::kChromeOSAssetHost;
 }
 
 void ImageSource::StartDataRequest(
     const std::string& path,
     int render_process_id,
     int render_frame_id,
     const content::URLDataSource::GotDataCallback& callback) {
   if (!IsWhitelisted(path)) {
     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 {
   std::string mime_type;
   std::string ext = base::FilePath(path).Extension();
   if (!ext.empty())
     net::GetWellKnownMimeTypeFromExtension(ext.substr(1), &mime_type);
   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;
 }
 
 }  // namespace chromeos