From 7cc259bcf3e2e6f948df3aad20b749d1240422bd Mon Sep 17 00:00:00 2001 From: Kristi Park Date: Thu, 13 May 2021 19:05:54 +0000 Subject: [PATCH] Revert "Change update_client::ReadManifest() to return base::Value." This reverts commit bc041ac5d65c325c6846aa6f96f6ee3bdb471137. Reason for revert: Compile errors, see https://ci.chromium.org/ui/p/chrome/builders/ci/win64-chrome/14513/overview Original change's description: > Change update_client::ReadManifest() to return base::Value. > > Stop returning base::DictionaryValue, which is deprecated. Then > ReadManifest() can stop doing the weird std::unique_ptr release() into > another std::unique_ptr. Then update ComponentInstaller::InstallHelper() > to use base::Value in its out parameter as well. With base::Value, there > is no more GetStringASCII() method, so use base::IsStringASCII() in the > callers. > > Bug: 1187036 > Change-Id: I88b77d4c744fd4be23c5691ac31d5be8a35b411b > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891514 > Reviewed-by: Joshua Pawlicki > Commit-Queue: Lei Zhang > Cr-Commit-Position: refs/heads/master@{#882536} Bug: 1187036 Change-Id: I8b07b993f584d2c598e084cdd5e34a2d0b98973a No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891201 Reviewed-by: Lei Zhang Reviewed-by: Kristi Park Bot-Commit: Rubber Stamper Owners-Override: Lei Zhang Commit-Queue: Kristi Park Cr-Commit-Position: refs/heads/master@{#882575} --- chrome/updater/installer.cc | 12 ++-- .../component_updater/component_installer.cc | 58 ++++++++----------- .../component_updater/component_installer.h | 2 +- components/update_client/test_installer.cc | 10 ++-- components/update_client/utils.cc | 14 +++-- components/update_client/utils.h | 8 +-- 6 files changed, 46 insertions(+), 58 deletions(-) diff --git a/chrome/updater/installer.cc b/chrome/updater/installer.cc index e18d151a582e9b..b58b8e9ee37b20 100644 --- a/chrome/updater/installer.cc +++ b/chrome/updater/installer.cc @@ -124,15 +124,13 @@ Installer::Result Installer::InstallHelper( base::ScopedBlockingCall scoped_blocking_call(FROM_HERE, base::BlockingType::WILL_BLOCK); - base::Value local_manifest = update_client::ReadManifest(unpack_path); - if (!local_manifest.is_dict()) + auto local_manifest = update_client::ReadManifest(unpack_path); + if (!local_manifest) return Result(update_client::InstallError::BAD_MANIFEST); - const std::string* version_ascii = local_manifest.FindStringKey("version"); - if (!version_ascii || !base::IsStringASCII(*version_ascii)) - return Result(update_client::InstallError::INVALID_VERSION); - - const base::Version manifest_version(*version_ascii); + std::string version_ascii; + local_manifest->GetStringASCII("version", &version_ascii); + const base::Version manifest_version(version_ascii); VLOG(1) << "Installed version=" << pv_ << ", installing version=" << manifest_version.GetString(); diff --git a/components/component_updater/component_installer.cc b/components/component_updater/component_installer.cc index 6ff113890b3ee4..bc82389f170bdb 100644 --- a/components/component_updater/component_installer.cc +++ b/components/component_updater/component_installer.cc @@ -18,7 +18,6 @@ #include "base/path_service.h" #include "base/sequenced_task_runner.h" #include "base/single_thread_task_runner.h" -#include "base/strings/string_util.h" #include "base/task/post_task.h" #include "base/task/task_traits.h" #include "base/task/thread_pool.h" @@ -104,19 +103,18 @@ void ComponentInstaller::OnUpdateError(int error) { LOG(ERROR) << "Component update error: " << error; } -Result ComponentInstaller::InstallHelper(const base::FilePath& unpack_path, - base::Value* manifest, - base::Version* version, - base::FilePath* install_path) { - base::Value local_manifest = update_client::ReadManifest(unpack_path); - if (!local_manifest.is_dict()) +Result ComponentInstaller::InstallHelper( + const base::FilePath& unpack_path, + std::unique_ptr* manifest, + base::Version* version, + base::FilePath* install_path) { + auto local_manifest = update_client::ReadManifest(unpack_path); + if (!local_manifest) return Result(InstallError::BAD_MANIFEST); - const std::string* version_ascii = local_manifest.FindStringKey("version"); - if (!version_ascii || !base::IsStringASCII(*version_ascii)) - return Result(InstallError::INVALID_VERSION); - - const base::Version manifest_version(*version_ascii); + std::string version_ascii; + local_manifest->GetStringASCII("version", &version_ascii); + const base::Version manifest_version(version_ascii); VLOG(1) << "Install: version=" << manifest_version.GetString() << " current version=" << current_version_.GetString(); @@ -160,17 +158,14 @@ Result ComponentInstaller::InstallHelper(const base::FilePath& unpack_path, DCHECK(!base::PathExists(unpack_path)); DCHECK(base::PathExists(local_install_path)); - const base::DictionaryValue& local_manifest_dict = - base::Value::AsDictionaryValue(local_manifest); - const Result result = installer_policy_->OnCustomInstall(local_manifest_dict, - local_install_path); + const Result result = + installer_policy_->OnCustomInstall(*local_manifest, local_install_path); if (result.error) return result; - if (!installer_policy_->VerifyInstallation(local_manifest_dict, - local_install_path)) { + if (!installer_policy_->VerifyInstallation(*local_manifest, + local_install_path)) return Result(InstallError::INSTALL_VERIFICATION_FAILED); - } *manifest = std::move(local_manifest); *version = manifest_version; @@ -185,7 +180,7 @@ void ComponentInstaller::Install( std::unique_ptr /*install_params*/, ProgressCallback /*progress_callback*/, Callback callback) { - base::Value manifest; + std::unique_ptr manifest; base::Version version; base::FilePath install_path; const Result result = @@ -201,10 +196,8 @@ void ComponentInstaller::Install( current_install_dir_ = install_path; main_task_runner_->PostTask( - FROM_HERE, - base::BindOnce(&ComponentInstaller::ComponentReady, this, - base::DictionaryValue::From( - base::Value::ToUniquePtrValue(std::move(manifest))))); + FROM_HERE, base::BindOnce(&ComponentInstaller::ComponentReady, this, + std::move(manifest))); main_task_runner_->PostTask(FROM_HERE, base::BindOnce(std::move(callback), result)); } @@ -234,22 +227,20 @@ bool ComponentInstaller::FindPreinstallation( return false; } - base::Value manifest = update_client::ReadManifest(path); - if (!manifest.is_dict()) { + std::unique_ptr manifest = + update_client::ReadManifest(path); + if (!manifest) { DVLOG(1) << "Manifest does not exist: " << path.MaybeAsASCII(); return false; } - std::unique_ptr manifest_dict = - base::DictionaryValue::From( - base::Value::ToUniquePtrValue(std::move(manifest))); - if (!installer_policy_->VerifyInstallation(*manifest_dict, path)) { + if (!installer_policy_->VerifyInstallation(*manifest, path)) { DVLOG(1) << "Installation verification failed: " << path.MaybeAsASCII(); return false; } std::string version_lexical; - if (!manifest_dict->GetStringASCII("version", &version_lexical)) { + if (!manifest->GetStringASCII("version", &version_lexical)) { DVLOG(1) << "Failed to get component version from the manifest."; return false; } @@ -266,7 +257,7 @@ bool ComponentInstaller::FindPreinstallation( registration_info->install_dir = path; registration_info->version = version; - registration_info->manifest = std::move(manifest_dict); + registration_info->manifest = std::move(manifest); return true; } @@ -343,8 +334,7 @@ void ComponentInstaller::StartRegistration( } std::unique_ptr manifest = - base::DictionaryValue::From( - base::Value::ToUniquePtrValue(update_client::ReadManifest(path))); + update_client::ReadManifest(path); if (!manifest || !installer_policy_->VerifyInstallation(*manifest, path)) { PLOG(ERROR) << "Failed to read manifest or verify installation for " << installer_policy_->GetName() << " (" << path.MaybeAsASCII() diff --git a/components/component_updater/component_installer.h b/components/component_updater/component_installer.h index 89a7633106f101..bedc9e02130252 100644 --- a/components/component_updater/component_installer.h +++ b/components/component_updater/component_installer.h @@ -166,7 +166,7 @@ class ComponentInstaller final : public update_client::CrxInstaller { scoped_refptr registration_info); update_client::CrxInstaller::Result InstallHelper( const base::FilePath& unpack_path, - base::Value* manifest, + std::unique_ptr* manifest, base::Version* version, base::FilePath* install_path); void StartRegistration(scoped_refptr registration_info); diff --git a/components/update_client/test_installer.cc b/components/update_client/test_installer.cc index 4a2db7bccb7495..0ea3a62a202f71 100644 --- a/components/update_client/test_installer.cc +++ b/components/update_client/test_installer.cc @@ -11,7 +11,6 @@ #include "base/callback.h" #include "base/files/file_path.h" #include "base/files/file_util.h" -#include "base/strings/string_util.h" #include "base/task/post_task.h" #include "base/task/task_traits.h" #include "base/task/thread_pool.h" @@ -97,12 +96,11 @@ void VersionedTestInstaller::Install( std::unique_ptr /*install_params*/, ProgressCallback progress_callback, Callback callback) { - const base::Value manifest = update_client::ReadManifest(unpack_path); - const std::string* version_string = manifest.FindStringKey("version"); - if (!version_string || !base::IsStringASCII(*version_string)) - return; + const auto manifest = update_client::ReadManifest(unpack_path); + std::string version_string; + manifest->GetStringASCII("version", &version_string); + const base::Version version(version_string); - const base::Version version(*version_string); const base::FilePath path = install_directory_.AppendASCII(version.GetString()); base::CreateDirectory(path.DirName()); diff --git a/components/update_client/utils.cc b/components/update_client/utils.cc index af519eb6e21dad..51738ab578bb68 100644 --- a/components/update_client/utils.cc +++ b/components/update_client/utils.cc @@ -9,8 +9,6 @@ #include #include #include -#include -#include #include "base/callback.h" #include "base/files/file_path.h" @@ -156,17 +154,21 @@ CrxInstaller::Result InstallFunctionWrapper( // TODO(cpu): add a specific attribute check to a component json that the // extension unpacker will reject, so that a component cannot be installed // as an extension. -base::Value ReadManifest(const base::FilePath& unpack_path) { +std::unique_ptr ReadManifest( + const base::FilePath& unpack_path) { base::FilePath manifest = unpack_path.Append(FILE_PATH_LITERAL("manifest.json")); if (!base::PathExists(manifest)) - return base::Value(); + return nullptr; JSONFileValueDeserializer deserializer(manifest); std::string error; std::unique_ptr root = deserializer.Deserialize(nullptr, &error); if (!root) - return base::Value(); - return base::Value::FromUniquePtrValue(std::move(root)); + return nullptr; + if (!root->is_dict()) + return nullptr; + return std::unique_ptr( + static_cast(root.release())); } } // namespace update_client diff --git a/components/update_client/utils.h b/components/update_client/utils.h index 4dee34d187d887..9423392b3efe87 100644 --- a/components/update_client/utils.h +++ b/components/update_client/utils.h @@ -5,6 +5,7 @@ #ifndef COMPONENTS_UPDATE_CLIENT_UTILS_H_ #define COMPONENTS_UPDATE_CLIENT_UTILS_H_ +#include #include #include #include @@ -16,8 +17,8 @@ class GURL; namespace base { +class DictionaryValue; class FilePath; -class Value; } namespace update_client { @@ -71,9 +72,8 @@ CrxInstaller::Result InstallFunctionWrapper( base::OnceCallback callback); // Deserializes the CRX manifest. The top level must be a dictionary. -// Returns a base::Value object of type dictionary on success, or another type -// on failure. -base::Value ReadManifest(const base::FilePath& unpack_path); +std::unique_ptr ReadManifest( + const base::FilePath& unpack_path); // Converts a custom, specific installer error (and optionally extended error) // to an installer result.