[Extensions][Task Scheduler] Migrate ExecuteCodeFunction

extensions/browser/api/execute_code_function.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.
 
 #ifndef EXTENSIONS_BROWSER_API_EXECUTE_CODE_FUNCTION_IMPL_H_
 #define EXTENSIONS_BROWSER_API_EXECUTE_CODE_FUNCTION_IMPL_H_
 
 #include "extensions/browser/api/execute_code_function.h"
 
+#include "base/task_scheduler/post_task.h"
+#include "base/threading/thread_restrictions.h"
 #include "extensions/browser/component_extension_resource_manager.h"
 #include "extensions/browser/extension_api_frame_id_map.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/file_reader.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/message_bundle.h"
 #include "net/base/filename_util.h"
@@ skipping 15 matching lines @@
 namespace extensions {
 
 using api::extension_types::InjectDetails;
 
 ExecuteCodeFunction::ExecuteCodeFunction() {
 }
 
 ExecuteCodeFunction::~ExecuteCodeFunction() {
 }
 
-void ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnFileThread(
+void ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnBackgroundThread(
     const std::string& extension_id,
     const base::FilePath& extension_path,
     const std::string& extension_default_locale,
     bool might_require_localization,
     std::string* data) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  base::ThreadRestrictions::AssertIOAllowed();
 
   file_url_ = net::FilePathToFileURL(resource_.GetFilePath());

[karandeepb, 2017/07/13 01:47:48]

It seems to me that net::FilePathToFileURL does not need to be executed on a
sequence with FILE IO. Also, it seems that currently file_url_ is modified on
file thread and used on UI thread (Not sure if other constraints make this
thread safe). 

Maybe it would be a good idea to initialize file_url_ on UI thread and skip the
hop to file thread if might_require_localization is false. Though this should
probably be done separately. (Also, didn't really dive into the code so may be
wrong)

[Devlin, 2017/07/13 18:35:09]

Good find!  I agree that this would be better done separately (to keep this
patch targeted and allow reverting of only one if something goes wrong), but
I've added a TODO.

 
   if (!might_require_localization)
     return;
 
   bool needs_message_substituion =
       data->find(extensions::MessageBundle::kMessageBegin) != std::string::npos;
   if (!needs_message_substituion)
     return;
 
   std::unique_ptr<SubstitutionMap> localization_messages(
       file_util::LoadMessageBundleSubstitutionMap(extension_path, extension_id,
                                                   extension_default_locale));
 
   std::string error;
   MessageBundle::ReplaceMessagesWithExternalDictionary(*localization_messages,
                                                        data, &error);
 }
 
-void ExecuteCodeFunction::GetFileURLAndLocalizeComponentResourceOnFileThread(
+std::unique_ptr<std::string>
+ExecuteCodeFunction::GetFileURLAndLocalizeComponentResourceOnBackgroundThread(
     std::unique_ptr<std::string> data,
     const std::string& extension_id,
     const base::FilePath& extension_path,
     const std::string& extension_default_locale,
     bool might_require_localization) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
-  GetFileURLAndMaybeLocalizeOnFileThread(
+  base::ThreadRestrictions::AssertIOAllowed();
+  GetFileURLAndMaybeLocalizeOnBackgroundThread(
       extension_id, extension_path, extension_default_locale,
       might_require_localization, data.get());
 
-  bool success = true;
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
-                 resource_.relative_path().AsUTF8Unsafe(), success,
-                 base::Passed(std::move(data))));
+  return data;
 }
 
 void ExecuteCodeFunction::DidLoadAndLocalizeFile(
     const std::string& file,
     bool success,
     std::unique_ptr<std::string> data) {
   if (success) {
     if (!base::IsStringUTF8(*data)) {
       error_ = ErrorUtils::FormatErrorMessage(kBadFileEncodingError, file);
       SendResponse(false);
@@ skipping 117 matching lines @@
               ->GetComponentExtensionResourceManager();
   if (component_extension_resource_manager &&
       component_extension_resource_manager->IsComponentExtensionResource(
           resource_.extension_root(),
           resource_.relative_path(),
           &resource_id)) {
     base::StringPiece resource =
         ResourceBundle::GetSharedInstance().GetRawDataResource(resource_id);
     std::unique_ptr<std::string> data(
         new std::string(resource.data(), resource.size()));
-    content::BrowserThread::PostTask(
-        content::BrowserThread::FILE, FROM_HERE,
-        base::Bind(&ExecuteCodeFunction::
-                       GetFileURLAndLocalizeComponentResourceOnFileThread,
-                   this, base::Passed(std::move(data)), extension_id,
-                   extension_path, extension_default_locale,
-                   might_require_localization));
+
+    base::PostTaskWithTraitsAndReplyWithResult(

[karandeepb, 2017/07/13 01:47:48]

Changing to a base::PostTask instead of a sequenced task runner, would cause us
to lost the ordering between the different posted tasks. (which we have
currently using the file thread). Is this still correct?

[Devlin, 2017/07/13 18:35:09]

Yep, this shouldn't be a problem.  This function only does one thing at a time,
so even though there are multiple threads involved, it should only be active on
a single one, which means we don't care about the sequence between posted tasks.

+        FROM_HERE, {base::MayBlock()},

[karandeepb, 2017/07/13 01:47:48]

It would be nice to be explicit about the shutdown behavior and task priority.

[Devlin, 2017/07/13 18:35:09]

Done for shutdown behavior.  For priority, it's difficult to say since we can't
really judge the importance of this task.  I'd rather leave it up to the task
scheduler to find the inherited priority than set an explicit one here.

+        base::BindOnce(
+            &ExecuteCodeFunction::
+                GetFileURLAndLocalizeComponentResourceOnBackgroundThread,
+            this, base::Passed(std::move(data)), extension_id, extension_path,
+            extension_default_locale, might_require_localization),
+        base::BindOnce(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
+                       resource_.relative_path().AsUTF8Unsafe(),
+                       true /* We assume this call always succeeds */));
   } else {
     FileReader::OptionalFileThreadTaskCallback get_file_and_l10n_callback =
-        base::Bind(&ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnFileThread,
-                   this, extension_id, extension_path, extension_default_locale,
-                   might_require_localization);
+        base::Bind(
+            &ExecuteCodeFunction::GetFileURLAndMaybeLocalizeOnBackgroundThread,
+            this, extension_id, extension_path, extension_default_locale,
+            might_require_localization);
 
     scoped_refptr<FileReader> file_reader(new FileReader(
         resource_, get_file_and_l10n_callback,
         base::Bind(&ExecuteCodeFunction::DidLoadAndLocalizeFile, this,
                    resource_.relative_path().AsUTF8Unsafe())));
     file_reader->Start();
   }
 
   return true;
 }
 
 void ExecuteCodeFunction::OnExecuteCodeFinished(const std::string& error,
                                                 const GURL& on_url,
                                                 const base::ListValue& result) {
   if (!error.empty())
     SetError(error);
 
   SendResponse(error.empty());
 }
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_BROWSER_API_EXECUTE_CODE_FUNCTION_IMPL_H_