Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1863)

Unified Diff: chrome/browser/component_updater/sw_reporter_installer_win.cc

Issue 2278013002: Add support for the ExperimentalSwReporterEngine field trial. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Address sorin's review comments and simplify empty suffix handling Created 4 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/component_updater/sw_reporter_installer_win.cc
diff --git a/chrome/browser/component_updater/sw_reporter_installer_win.cc b/chrome/browser/component_updater/sw_reporter_installer_win.cc
index cad962c7d29a8f83f4a2a8fb84770271fee87a19..ef214dc8fde2cd5079a76d9a59e2a3285147e485 100644
--- a/chrome/browser/component_updater/sw_reporter_installer_win.cc
+++ b/chrome/browser/component_updater/sw_reporter_installer_win.cc
@@ -6,6 +6,7 @@
#include <stdint.h>
+#include <algorithm>
#include <map>
#include <memory>
#include <string>
@@ -14,6 +15,8 @@
#include "base/base_paths.h"
#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/feature_list.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/logging.h"
@@ -21,6 +24,8 @@
#include "base/metrics/sparse_histogram.h"
#include "base/path_service.h"
#include "base/strings/string_tokenizer.h"
+#include "base/strings/string_util.h"
+#include "base/sys_info.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/threading/worker_pool.h"
#include "base/time/time.h"
@@ -35,14 +40,15 @@
#include "components/prefs/pref_registry_simple.h"
#include "components/update_client/update_client.h"
#include "components/update_client/utils.h"
+#include "components/variations/variations_associated_data.h"
#include "content/public/browser/browser_thread.h"
namespace component_updater {
namespace {
-// These two sets of values are used to send UMA information and are replicated
-// in the histograms.xml file, so the order MUST NOT CHANGE.
+// These values are used to send UMA information and are replicated in the
+// histograms.xml file, so the order MUST NOT CHANGE.
enum SRTCompleted {
SRT_COMPLETED_NOT_YET = 0,
SRT_COMPLETED_YES = 1,
@@ -117,11 +123,135 @@ void RunSwReporterAfterStartup(
base::WorkerPool::GetTaskRunner(true)));
}
+constexpr base::Feature kExperimentalEngineFeature{
+ "ExperimentalSwReporterEngine", base::FEATURE_DISABLED_BY_DEFAULT};
+
+// Max length of an installer attribute (taken from the regexp in
+// ComponentInstallerTraits::InstallerAttributes).
+constexpr size_t kMaxAttributeLength = 256;
+
+// Max length of the registry and histogram suffix. Fairly arbitrary: the
+// Windows registry accepts much longer keys, but we need to display this
+// string in histograms as well.
+constexpr size_t kMaxSuffixLength = 80;
+
+// These MUST match the values for SwReporterExperimentError in histograms.xml.
+enum ExperimentError {
+ EXPERIMENT_ERROR_BAD_TAG = 0,
+ EXPERIMENT_ERROR_BAD_PARAMS = 1,
+ EXPERIMENT_ERROR_MAX,
+};
+
+void ReportExperimentError(ExperimentError error) {
+ UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.ExperimentErrors", error,
+ EXPERIMENT_ERROR_MAX);
+}
+
+// Ensure |str| contains only alphanumeric characters and characters from
+// |extras|, and is not longer than |max_length|.
+bool ValidateString(const std::string& str,
+ const std::string& extras,
+ size_t max_length) {
+ return str.size() <= max_length &&
+ std::all_of(str.cbegin(), str.cend(), [&extras](char c) {
+ return base::IsAsciiAlpha(c) || base::IsAsciiDigit(c) ||
+ extras.find(c) != std::string::npos;
+ });
+}
+
+// Read the command-line params and an UMA histogram suffix from the manifest,
+// and launch the SwReporter with those parameters. If anything goes wrong the
+// SwReporter should not be run at all - do not fall back to the default.
+void RunExperimentalSwReporter(const base::FilePath& exe_path,
+ const base::Version& version,
+ std::unique_ptr<base::DictionaryValue> manifest,
+ const SwReporterRunner& reporter_runner) {
+ // Don't log an error if the launch_params section is entirely missing. This
+ // can happen when the user already has an older version of the software
+ // reporter component, so |ComponentReady| is called on startup before the
+ // experimental version is downloaded.
+ //
+ // Also don't run the old version, though. We want this user to run the
+ // experimental version once it's been downloaded.
+ if (!manifest->HasKey("launch_params"))
+ return;
+
+ const base::ListValue* parameter_list = nullptr;
+ if (!manifest->GetList("launch_params", &parameter_list)) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+ return;
+ }
+ DCHECK(parameter_list);
+ if (parameter_list->empty()) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+ return;
+ }
+
+ // For now we only support running the reporter once, so only look at the
+ // first set of arguments in the list.
+ const base::DictionaryValue* invocation_params = nullptr;
+ if (!parameter_list->GetDictionary(0, &invocation_params)) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+ return;
+ }
+
+ // The suffix is optional. If present, it must be a short alphanumeric string.
+ std::string suffix;
+ DCHECK(invocation_params);
+ if (invocation_params->HasKey("suffix")) {
+ if (!invocation_params->GetString("suffix", &suffix)) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+ return;
+ }
+ if (!suffix.empty() &&
+ !ValidateString(suffix, std::string(), kMaxSuffixLength)) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+ return;
+ }
+ }
+
+ // Build a command line for the reporter out of the executable path and the
+ // (optional) arguments from the manifest.
+ std::vector<base::string16> argv{exe_path.value()};
+ if (invocation_params->HasKey("arguments")) {
+ const base::ListValue* arguments = nullptr;
+ if (!invocation_params->GetList("arguments", &arguments)) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+ return;
+ }
+ for (const std::unique_ptr<base::Value>& value : *arguments) {
+ base::string16 argument;
+ if (!value->GetAsString(&argument)) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+ return;
+ }
+ if (!argument.empty())
+ argv.push_back(argument);
+ }
+ }
+ base::CommandLine command_line(argv);
+
+ // Add the histogram suffix to the command-line as well, so that the
+ // reporter will add the same suffix to registry keys where it writes
+ // metrics.
+ if (!suffix.empty())
+ command_line.AppendSwitchASCII("registry-suffix", suffix);
+
+ auto invocation =
+ safe_browsing::SwReporterInvocation::FromCommandLine(command_line);
+ invocation.suffix = suffix;
+ invocation.is_experimental = true;
+
+ reporter_runner.Run(invocation, version);
+}
+
} // namespace
SwReporterInstallerTraits::SwReporterInstallerTraits(
- const SwReporterRunner& reporter_runner)
- : reporter_runner_(reporter_runner) {}
+ const SwReporterRunner& reporter_runner,
+ bool is_experimental_engine_supported)
+ : reporter_runner_(reporter_runner),
+ is_experimental_engine_supported_(is_experimental_engine_supported) {}
SwReporterInstallerTraits::~SwReporterInstallerTraits() {}
@@ -152,6 +282,11 @@ void SwReporterInstallerTraits::ComponentReady(
std::unique_ptr<base::DictionaryValue> manifest) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
const base::FilePath exe_path(install_dir.Append(kSwReporterExeName));
+ if (IsExperimentalEngineEnabled()) {
+ RunExperimentalSwReporter(exe_path, version, std::move(manifest),
+ reporter_runner_);
+ return;
+ }
reporter_runner_.Run(
safe_browsing::SwReporterInvocation::FromFilePath(exe_path), version);
}
@@ -171,13 +306,36 @@ std::string SwReporterInstallerTraits::GetName() const {
update_client::InstallerAttributes
SwReporterInstallerTraits::GetInstallerAttributes() const {
- return update_client::InstallerAttributes();
+ update_client::InstallerAttributes attributes;
+ if (IsExperimentalEngineEnabled()) {
+ // Pass the "tag" parameter to the installer; it will be used to choose
+ // which binary is downloaded.
+ constexpr char kTagParam[] = "tag";
+ const std::string tag = variations::GetVariationParamValueByFeature(
+ kExperimentalEngineFeature, kTagParam);
+
+ // If the tag is not a valid attribute (see the regexp in
+ // ComponentInstallerTraits::InstallerAttributes), set it to a valid but
+ // unrecognized value so that nothing will be downloaded.
+ if (tag.empty() || !ValidateString(tag, "_.,;+_=", kMaxAttributeLength)) {
+ ReportExperimentError(EXPERIMENT_ERROR_BAD_TAG);
+ attributes[kTagParam] = "missing_tag";
+ } else {
+ attributes[kTagParam] = tag;
+ }
+ }
+ return attributes;
}
std::vector<std::string> SwReporterInstallerTraits::GetMimeTypes() const {
return std::vector<std::string>();
}
+bool SwReporterInstallerTraits::IsExperimentalEngineEnabled() const {
+ return is_experimental_engine_supported_ &&
+ base::FeatureList::IsEnabled(kExperimentalEngineFeature);
+}
+
void RegisterSwReporterComponent(ComponentUpdateService* cus) {
if (!safe_browsing::IsSwReporterEnabled())
return;
@@ -254,9 +412,15 @@ void RegisterSwReporterComponent(ComponentUpdateService* cus) {
}
}
+ // The experiment is only enabled on x86. There's no way to check this in the
+ // variations config so we'll hard-code it.
+ const bool is_experimental_engine_enabled =
jwd 2016/08/25 17:53:55 Can you change this to be _supported?
Joe Mason 2016/08/25 18:07:17 Done.
+ base::SysInfo::OperatingSystemArchitecture() == "x86";
+
// Install the component.
std::unique_ptr<ComponentInstallerTraits> traits(
- new SwReporterInstallerTraits(base::Bind(&RunSwReporterAfterStartup)));
+ new SwReporterInstallerTraits(base::Bind(&RunSwReporterAfterStartup),
+ is_experimental_engine_enabled));
// |cus| will take ownership of |installer| during installer->Register(cus).
DefaultComponentInstaller* installer =
new DefaultComponentInstaller(std::move(traits));

Powered by Google App Engine
This is Rietveld 408576698