Commit 3f6ca074 authored by Xiaochu Liu's avatar Xiaochu Liu Committed by Commit Bot

Revert "Move OnCustomInstall body to SequencedTaskRunner"

This reverts commit b8a20799.

Reason for revert: it turns out that dbus method call in chrome requires UI thread (a DCHECK is broken). my initial plan to do blocking dbus call in OnCustomInstall is no longer viable. OnCustomInstall still needs to block its calling thread: crbug.com/783495 and i will investigate change in component updater to achieve this. 

more discussions regarding dbus blocking method call are here: https://chromium-review.googlesource.com/c/chromium/src/+/762080

Original change's description:
> Move OnCustomInstall body to SequencedTaskRunner
> 
> Currently, OnCustomInstall is invoked in a background thread but it
> invokes imageloader on UI thread which does not bring any known benefits.
> 
> We move imageloader operation from UI thread over a SequnecedTaskRunner.
> 
> BUG=chromium:780201
> TEST=install is successful on DUT
> 
> Change-Id: I8deb2ebae9ff22fcf992f4c34d6198ddbcad0c70
> Reviewed-on: https://chromium-review.googlesource.com/752622
> Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
> Reviewed-by: Greg Kerr <kerrnel@chromium.org>
> Reviewed-by: Dan Erat <derat@chromium.org>
> Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#515291}

TBR=derat@chromium.org,waffles@chromium.org,ejcaruso@chromium.org,kerrnel@chromium.org,xiaochu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:780201
Change-Id: Ieb0b29ee30dcbd6208bd1c2fcd176cbb10ad48d4
Reviewed-on: https://chromium-review.googlesource.com/775561
Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: default avatarDan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517257}
parent 2bbad6bf
...@@ -10,7 +10,6 @@ ...@@ -10,7 +10,6 @@
#include "base/optional.h" #include "base/optional.h"
#include "base/path_service.h" #include "base/path_service.h"
#include "base/task_scheduler/post_task.h" #include "base/task_scheduler/post_task.h"
#include "base/task_scheduler/task_traits.h"
#include "chrome/browser/browser_process.h" #include "chrome/browser/browser_process.h"
#include "chrome/browser/component_updater/component_installer_errors.h" #include "chrome/browser/component_updater/component_installer_errors.h"
#include "chromeos/dbus/dbus_thread_manager.h" #include "chromeos/dbus/dbus_thread_manager.h"
...@@ -43,11 +42,14 @@ ...@@ -43,11 +42,14 @@
{"sha2hashstr", \ {"sha2hashstr", \
"6d24de30f671da5aee6d463d9e446cafe9ddac672800a9defe86877dcde6c466"}}}}; "6d24de30f671da5aee6d463d9e446cafe9ddac672800a9defe86877dcde6c466"}}}};
using content::BrowserThread;
namespace component_updater { namespace component_updater {
using ConfigMap = std::map<std::string, std::map<std::string, std::string>>; using ConfigMap = std::map<std::string, std::map<std::string, std::string>>;
void LogRegistrationResult(base::Optional<bool> result) { void LogRegistrationResult(base::Optional<bool> result) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (!result.has_value()) { if (!result.has_value()) {
DVLOG(1) << "Call to imageloader service failed."; DVLOG(1) << "Call to imageloader service failed.";
return; return;
...@@ -61,6 +63,7 @@ void LogRegistrationResult(base::Optional<bool> result) { ...@@ -61,6 +63,7 @@ void LogRegistrationResult(base::Optional<bool> result) {
void ImageLoaderRegistration(const std::string& version, void ImageLoaderRegistration(const std::string& version,
const base::FilePath& install_dir, const base::FilePath& install_dir,
const std::string& name) { const std::string& name) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
chromeos::ImageLoaderClient* loader = chromeos::ImageLoaderClient* loader =
chromeos::DBusThreadManager::Get()->GetImageLoaderClient(); chromeos::DBusThreadManager::Get()->GetImageLoaderClient();
if (loader) { if (loader) {
...@@ -87,13 +90,8 @@ CrOSComponentInstallerPolicy::CrOSComponentInstallerPolicy( ...@@ -87,13 +90,8 @@ CrOSComponentInstallerPolicy::CrOSComponentInstallerPolicy(
cell = stoul(strstream.substr(0, 2), nullptr, 16); cell = stoul(strstream.substr(0, 2), nullptr, 16);
strstream.erase(0, 2); strstream.erase(0, 2);
} }
task_runner_ = base::CreateSingleThreadTaskRunnerWithTraits(
{base::MayBlock(), base::TaskPriority::BACKGROUND,
base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN});
} }
CrOSComponentInstallerPolicy::~CrOSComponentInstallerPolicy() {}
bool CrOSComponentInstallerPolicy::SupportsGroupPolicyEnabledComponentUpdates() bool CrOSComponentInstallerPolicy::SupportsGroupPolicyEnabledComponentUpdates()
const { const {
return true; return true;
...@@ -111,8 +109,9 @@ CrOSComponentInstallerPolicy::OnCustomInstall( ...@@ -111,8 +109,9 @@ CrOSComponentInstallerPolicy::OnCustomInstall(
if (!manifest.GetString("version", &version)) { if (!manifest.GetString("version", &version)) {
return ToInstallerResult(update_client::InstallError::GENERIC_ERROR); return ToInstallerResult(update_client::InstallError::GENERIC_ERROR);
} }
task_runner_->PostTask(FROM_HERE, base::BindOnce(&ImageLoaderRegistration, BrowserThread::PostTask(
version, install_dir, name)); BrowserThread::UI, FROM_HERE,
base::BindOnce(&ImageLoaderRegistration, version, install_dir, name));
return update_client::CrxInstaller::Result(update_client::InstallError::NONE); return update_client::CrxInstaller::Result(update_client::InstallError::NONE);
} }
......
...@@ -10,7 +10,6 @@ ...@@ -10,7 +10,6 @@
#include <string> #include <string>
#include <vector> #include <vector>
#include "base/sequenced_task_runner.h"
#include "chromeos/dbus/dbus_method_call_status.h" #include "chromeos/dbus/dbus_method_call_status.h"
#include "components/component_updater/component_installer.h" #include "components/component_updater/component_installer.h"
#include "components/component_updater/component_updater_service.h" #include "components/component_updater/component_updater_service.h"
...@@ -46,7 +45,7 @@ struct ComponentConfig { ...@@ -46,7 +45,7 @@ struct ComponentConfig {
class CrOSComponentInstallerPolicy : public ComponentInstallerPolicy { class CrOSComponentInstallerPolicy : public ComponentInstallerPolicy {
public: public:
explicit CrOSComponentInstallerPolicy(const ComponentConfig& config); explicit CrOSComponentInstallerPolicy(const ComponentConfig& config);
~CrOSComponentInstallerPolicy() override; ~CrOSComponentInstallerPolicy() override {}
private: private:
FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest, IsCompatibleOrNot); FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest, IsCompatibleOrNot);
...@@ -77,8 +76,6 @@ class CrOSComponentInstallerPolicy : public ComponentInstallerPolicy { ...@@ -77,8 +76,6 @@ class CrOSComponentInstallerPolicy : public ComponentInstallerPolicy {
std::string name; std::string name;
std::string env_version; std::string env_version;
uint8_t kSha2Hash_[crypto::kSHA256Length] = {}; uint8_t kSha2Hash_[crypto::kSHA256Length] = {};
// Use task_runner_ for scheduling all imageloader operations.
scoped_refptr<base::SequencedTaskRunner> task_runner_;
DISALLOW_COPY_AND_ASSIGN(CrOSComponentInstallerPolicy); DISALLOW_COPY_AND_ASSIGN(CrOSComponentInstallerPolicy);
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment