Fix threading bugs in product label UI.

As mentioned in https://codereview.chromium.org/680393002/#msg6, the current code had some threading issues.

BUG=438953
R=dbeam@chromium.org,satorux@chromium.org
CC=stevenjb@chromium.org

Review URL: https://codereview.chromium.org/780203002

Cr-Commit-Position: refs/heads/master@{#307640}

4 files changed, 76 insertions, 62 deletions

diff --git a/chrome/browser/ui/webui/chromeos/image_source.cc b/chrome/browser/ui/webui/chromeos/image_source.cc
index 726e9f6..84ddaeb 100644
--- a/chrome/browser/ui/webui/chromeos/image_source.cc
+++ b/chrome/browser/ui/webui/chromeos/image_source.cc
@@ -21,9 +21,40 @@ namespace chromeos {
 namespace {
 
 const char* kWhitelistedFiles[] = {
-    "fcc/label.png"
+  "fcc/label.png"
 };
 
+// Callback for user_manager::UserImageLoader.
+void ImageLoaded(
+    const content::URLDataSource::GotDataCallback& got_data_callback,
+    const user_manager::UserImage& user_image) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (user_image.has_raw_image())
+    got_data_callback.Run(new base::RefCountedBytes(user_image.raw_image()));
+  else
+    got_data_callback.Run(NULL);
+}
+
+// Looks for the image at |path| under the shared assets directory.
+void StartOnBlockingPool(
+    const std::string& path,
+    scoped_refptr<UserImageLoader> image_loader,
+    const content::URLDataSource::GotDataCallback& got_data_callback,
+    scoped_refptr<base::MessageLoopProxy> message_loop_proxy) {
+  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,
+                        base::Bind(&ImageLoaded, got_data_callback));
+  } else {
+    message_loop_proxy->PostTask(FROM_HERE,
+                                 base::Bind(got_data_callback, nullptr));
+  }
+}
+
 }  // namespace
 
 ImageSource::ImageSource() : weak_factory_(this) {
@@ -45,17 +76,24 @@ 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_);
+  }
+
+  content::BrowserThread::GetBlockingPool()->PostTask(
+      FROM_HERE,
+      base::Bind(&StartOnBlockingPool,
                  path,
-                 callback));
+                 image_loader_,
+                 got_data_callback,
+                 base::MessageLoopProxy::current()));
 }
 
 std::string ImageSource::GetMimeType(const std::string& path) const {
@@ -66,35 +104,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;
diff --git a/chrome/browser/ui/webui/chromeos/image_source.h b/chrome/browser/ui/webui/chromeos/image_source.h
index 3db5d9b..3d330b0 100644
--- a/chrome/browser/ui/webui/chromeos/image_source.h
+++ b/chrome/browser/ui/webui/chromeos/image_source.h
@@ -38,19 +38,12 @@ class ImageSource : public content::URLDataSource {
       const std::string& path,
       int render_process_id,
       int render_frame_id,
-      const content::URLDataSource::GotDataCallback& callback) override;
+      const content::URLDataSource::GotDataCallback& got_data_callback)
+      override;
+
   std::string GetMimeType(const std::string& path) const override;
 
  private:
-  void StartOnFileThread(
-      const std::string& path,
-      const content::URLDataSource::GotDataCallback& callback);
-
-  // Callback for user_manager::UserImageLoader.
-  void ImageLoaded(
-    const content::URLDataSource::GotDataCallback& callback,
-    const user_manager::UserImage& user_image) const;
-
   // Checks whether we have allowed the image to be loaded.
   bool IsWhitelisted(const std::string& path) const;
 
diff --git a/chrome/browser/ui/webui/help/help_handler.cc b/chrome/browser/ui/webui/help/help_handler.cc
index b88247f..532fe51 100644
--- a/chrome/browser/ui/webui/help/help_handler.cc
+++ b/chrome/browser/ui/webui/help/help_handler.cc
@@ -49,6 +49,7 @@
 #include "base/i18n/time_formatting.h"
 #include "base/prefs/pref_service.h"
 #include "base/sys_info.h"
+#include "base/task_runner_util.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
@@ -126,6 +127,19 @@ bool CanChangeChannel(Profile* profile) {
   return false;
 }
 
+// Reads the file containing the FCC label text, if found. Must be called from
+// the blocking pool.
+std::string ReadFCCLabelText() {
+  const base::FilePath asset_dir(FILE_PATH_LITERAL(chrome::kChromeOSAssetPath));
+  const base::FilePath label_file_path =
+      asset_dir.AppendASCII(kFCCLabelTextPath);
+
+  std::string contents;
+  if (base::ReadFileToString(label_file_path, &contents))
+    return contents;
+  return std::string();
+}
+
 #endif  // defined(OS_CHROMEOS)
 
 }  // namespace
@@ -403,9 +417,12 @@ void HelpHandler::OnPageLoaded(const base::ListValue* args) {
   version_updater_->GetChannel(false,
       base::Bind(&HelpHandler::OnTargetChannel, weak_factory_.GetWeakPtr()));
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&HelpHandler::LoadFCCLabelText, weak_factory_.GetWeakPtr()));
+  base::PostTaskAndReplyWithResult(
+      content::BrowserThread::GetBlockingPool(),
+      FROM_HERE,
+      base::Bind(&ReadFCCLabelText),
+      base::Bind(&HelpHandler::OnFCCLabelTextRead,
+                 weak_factory_.GetWeakPtr()));
 #endif
 }
 
@@ -587,16 +604,11 @@ void HelpHandler::OnTargetChannel(const std::string& channel) {
       "help.HelpPage.updateTargetChannel", base::StringValue(channel));
 }
 
-void HelpHandler::LoadFCCLabelText() {
-  base::FilePath path(std::string(chrome::kChromeOSAssetPath) +
-                      kFCCLabelTextPath);
-  std::string contents;
-  if (base::ReadFileToString(path, &contents)) {
-    // Remove unnecessary whitespace.
-    base::StringValue label(base::CollapseWhitespaceASCII(contents, true));
-    web_ui()->CallJavascriptFunction("help.HelpPage.setProductLabelText",
-                                     label);
-  }
+void HelpHandler::OnFCCLabelTextRead(const std::string& text) {
+  // Remove unnecessary whitespace.
+  web_ui()->CallJavascriptFunction(
+      "help.HelpPage.setProductLabelText",
+      base::StringValue(base::CollapseWhitespaceASCII(text, true)));
 }
 
 #endif // defined(OS_CHROMEOS)
diff --git a/chrome/browser/ui/webui/help/help_handler.h b/chrome/browser/ui/webui/help/help_handler.h
index e3d9443..0c8577d 100644
--- a/chrome/browser/ui/webui/help/help_handler.h
+++ b/chrome/browser/ui/webui/help/help_handler.h
@@ -91,8 +91,8 @@ class HelpHandler : public content::WebUIMessageHandler,
   void OnCurrentChannel(const std::string& channel);
   void OnTargetChannel(const std::string& channel);
 
-  // Callback for loading FCC label alt text.
-  void LoadFCCLabelText();
+  // Callback for setting the FCC label alt text.
+  void OnFCCLabelTextRead(const std::string& text);
 #endif
 
   // Specialized instance of the VersionUpdater used to update the browser.