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

Unified Diff: chrome/browser/safe_browsing/srt_fetcher_win.cc

Issue 2226133005: Add support for the ExperimentalSwReporterEngine field trial. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Refactor, split some prep work into a different patch 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/safe_browsing/srt_fetcher_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_win.cc b/chrome/browser/safe_browsing/srt_fetcher_win.cc
index 208d8e5596ce76959cda565106edda832f16ccd0..ae23ee9ff906cd9c512f1103b9e1e2ed1ef006b0 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_win.cc
@@ -7,6 +7,7 @@
#include <stdint.h>
#include <memory>
+#include <queue>
#include <vector>
#include "base/bind.h"
@@ -16,7 +17,7 @@
#include "base/files/file_path.h"
#include "base/macros.h"
#include "base/metrics/field_trial.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram.h"
#include "base/metrics/sparse_histogram.h"
#include "base/process/launch.h"
#include "base/strings/string_number_conversions.h"
@@ -120,8 +121,20 @@ const char kScanTimesMetricName[] = "SoftwareReporter.UwSScanTimes";
const char kMemoryUsedMetricName[] = "SoftwareReporter.MemoryUsed";
// Reports metrics about the software reporter via UMA (and sometimes Rappor).
+//
+// This will format the names of the histograms at runtime by adding an
+// optional suffix, so the UMA_HISTOGRAM macros won't work. Use RecordHistogram
+// and related helper methods instead.
class UMAHistogramReporter {
public:
+ UMAHistogramReporter(const std::string& suffix = std::string())
+ : suffix_(suffix),
+ registry_key_(suffix.empty()
+ ? kSoftwareRemovalToolRegistryKey
+ : base::StringPrintf(L"%s\\%s",
+ kSoftwareRemovalToolRegistryKey,
+ base::UTF8ToUTF16(suffix))) {}
+
// Reports the software reporter tool's version via UMA.
void ReportVersion(const base::Version& version) const {
DCHECK(!version.components().empty());
@@ -132,7 +145,7 @@ class UMAHistogramReporter {
minor_version = version.components()[version.components().size() - 2];
else
minor_version = version.components()[0];
- UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.MinorVersion", minor_version);
+ RecordSparseHistogram("SoftwareReporter.MinorVersion", minor_version);
// The major version for X.Y.Z is X*256^3+Y*256+Z. If there are additional
// components, only the first three count, and if there are less than 3, the
@@ -147,31 +160,31 @@ class UMAHistogramReporter {
DCHECK_LT(version.components()[2], 0x100U);
major_version += version.components()[2];
}
- UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.MajorVersion", major_version);
+ RecordSparseHistogram("SoftwareReporter.MajorVersion", major_version);
}
void ReportExitCode(int exit_code) const {
- UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.ExitCode", exit_code);
+ RecordSparseHistogram("SoftwareReporter.ExitCode", exit_code);
}
// Reports UwS found by the software reporter tool via UMA and RAPPOR.
- void ReportFoundUwS() const {
- base::win::RegKey reporter_key(HKEY_CURRENT_USER,
- kSoftwareRemovalToolRegistryKey,
+ void ReportFoundUwS(bool use_rappor) const {
+ base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
KEY_QUERY_VALUE | KEY_SET_VALUE);
std::vector<base::string16> found_uws_strings;
if (reporter_key.Valid() &&
reporter_key.ReadValues(kFoundUwsValueName, &found_uws_strings) ==
ERROR_SUCCESS) {
- rappor::RapporService* rappor_service =
- g_browser_process->rappor_service();
+ rappor::RapporService* rappor_service = nullptr;
+ if (use_rappor)
+ rappor_service = g_browser_process->rappor_service();
bool parse_error = false;
for (const base::string16& uws_string : found_uws_strings) {
// All UwS ids are expected to be integers.
uint32_t uws_id = 0;
if (base::StringToUint(uws_string, &uws_id)) {
- UMA_HISTOGRAM_SPARSE_SLOWLY(kFoundUwsMetricName, uws_id);
+ RecordSparseHistogram(kFoundUwsMetricName, uws_id);
if (rappor_service) {
rappor_service->RecordSample(kFoundUwsMetricName,
rappor::COARSE_RAPPOR_TYPE,
@@ -185,7 +198,7 @@ class UMAHistogramReporter {
// Clean up the old value.
reporter_key.DeleteValue(kFoundUwsValueName);
- UMA_HISTOGRAM_BOOLEAN(kFoundUwsReadErrorMetricName, parse_error);
+ RecordBooleanHistogram(kFoundUwsReadErrorMetricName, parse_error);
}
}
@@ -193,14 +206,13 @@ class UMAHistogramReporter {
// by
// the tool itself in the Windows registry.
void ReportMemoryUsage() const {
- base::win::RegKey reporter_key(HKEY_CURRENT_USER,
- kSoftwareRemovalToolRegistryKey,
+ base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
KEY_QUERY_VALUE | KEY_SET_VALUE);
DWORD memory_used = 0;
if (reporter_key.Valid() &&
reporter_key.ReadValueDW(kMemoryUsedValueName, &memory_used) ==
ERROR_SUCCESS) {
- UMA_HISTOGRAM_MEMORY_KB(kMemoryUsedMetricName, memory_used);
+ RecordMemoryKBHistogram(kMemoryUsedMetricName, memory_used);
reporter_key.DeleteValue(kMemoryUsedValueName);
}
}
@@ -208,13 +220,13 @@ class UMAHistogramReporter {
// Report the SwReporter run time with UMA both as reported by the tool via
// the registry and as measured by |ReporterRunner|.
void ReportRuntime(const base::TimeDelta& reporter_running_time) const {
- UMA_HISTOGRAM_LONG_TIMES("SoftwareReporter.RunningTimeAccordingToChrome",
+ RecordLongTimesHistogram("SoftwareReporter.RunningTimeAccordingToChrome",
reporter_running_time);
- base::win::RegKey reporter_key(
- HKEY_CURRENT_USER, kSoftwareRemovalToolRegistryKey, KEY_ALL_ACCESS);
+ base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
+ KEY_ALL_ACCESS);
if (!reporter_key.Valid()) {
- UMA_HISTOGRAM_ENUMERATION(
+ RecordEnumerationHistogram(
kRunningTimeErrorMetricName,
REPORTER_RUNNING_TIME_ERROR_REGISTRY_KEY_INVALID,
REPORTER_RUNNING_TIME_ERROR_MAX);
@@ -243,31 +255,31 @@ class UMAHistogramReporter {
base::TimeDelta registry_run_time =
base::Time::FromInternalValue(end_time_value) -
base::Time::FromInternalValue(start_time_value);
- UMA_HISTOGRAM_LONG_TIMES("SoftwareReporter.RunningTime",
+ RecordLongTimesHistogram("SoftwareReporter.RunningTime",
registry_run_time);
- UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
- REPORTER_RUNNING_TIME_ERROR_NO_ERROR,
- REPORTER_RUNNING_TIME_ERROR_MAX);
+ RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+ REPORTER_RUNNING_TIME_ERROR_NO_ERROR,
+ REPORTER_RUNNING_TIME_ERROR_MAX);
} else if (!has_start_time && !has_end_time) {
- UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
- REPORTER_RUNNING_TIME_ERROR_MISSING_BOTH_TIMES,
- REPORTER_RUNNING_TIME_ERROR_MAX);
+ RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+ REPORTER_RUNNING_TIME_ERROR_MISSING_BOTH_TIMES,
+ REPORTER_RUNNING_TIME_ERROR_MAX);
} else if (!has_start_time) {
- UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
- REPORTER_RUNNING_TIME_ERROR_MISSING_START_TIME,
- REPORTER_RUNNING_TIME_ERROR_MAX);
+ RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+ REPORTER_RUNNING_TIME_ERROR_MISSING_START_TIME,
+ REPORTER_RUNNING_TIME_ERROR_MAX);
} else {
DCHECK(!has_end_time);
- UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
- REPORTER_RUNNING_TIME_ERROR_MISSING_END_TIME,
- REPORTER_RUNNING_TIME_ERROR_MAX);
+ RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+ REPORTER_RUNNING_TIME_ERROR_MISSING_END_TIME,
+ REPORTER_RUNNING_TIME_ERROR_MAX);
}
}
// Report the UwS scan times of the software reporter tool via UMA.
void ReportScanTimes() const {
base::string16 scan_times_key_path = base::StringPrintf(
- L"%ls\\%ls", kSoftwareRemovalToolRegistryKey, kScanTimesSubKey);
+ L"%ls\\%ls", registry_key_.c_str(), kScanTimesSubKey);
base::win::RegKey scan_times_key(
HKEY_CURRENT_USER, scan_times_key_path.c_str(), KEY_ALL_ACCESS);
if (!scan_times_key.Valid())
@@ -284,30 +296,104 @@ class UMAHistogramReporter {
ERROR_SUCCESS) {
base::TimeDelta scan_time =
base::TimeDelta::FromInternalValue(raw_scan_time);
- base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
- kScanTimesMetricName,
- base::HistogramBase::kUmaTargetedHistogramFlag);
- if (histogram) {
- // We report the number of seconds plus one because it can take less
- // than one second to scan some UwS and the count passed to |AddCount|
- // must be at least one.
- histogram->AddCount(uws_id, scan_time.InSeconds() + 1);
- }
+ // We report the number of seconds plus one because it can take less
+ // than one second to scan some UwS and the count passed to |AddCount|
+ // must be at least one.
+ RecordSparseHistogramCount(kScanTimesMetricName, uws_id,
+ scan_time.InSeconds() + 1);
}
}
// Clean up by deleting the scan times key, which is a subkey of the main
// reporter key.
scan_times_key.Close();
- base::win::RegKey reporter_key(HKEY_CURRENT_USER,
- kSoftwareRemovalToolRegistryKey,
+ base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
KEY_ENUMERATE_SUB_KEYS);
if (reporter_key.Valid())
reporter_key.DeleteKey(kScanTimesSubKey);
}
+
+ void RecordReporterStep(SwReporterUmaValue value) {
+ RecordEnumerationHistogram("SoftwareReporter.Step", value, SW_REPORTER_MAX);
+ }
+
+ private:
+ std::string FullName(const std::string& name) const {
+ if (suffix_.empty())
+ return name;
+ return base::StringPrintf("%s_%s", name, suffix_);
+ }
+
+ template <typename HistogramType, typename SampleType>
+ void RecordHistogram(const std::string& name, SampleType sample) const {
+ base::HistogramBase* histogram = HistogramType::FactoryGet(
+ FullName(name), base::HistogramBase::kUmaTargetedHistogramFlag);
+ if (histogram)
+ histogram->Add(sample);
+ }
+
+ template <typename SampleType>
+ void RecordBooleanHistogram(const std::string& name,
+ SampleType sample) const {
+ RecordHistogram<base::BooleanHistogram, SampleType>(name, sample);
+ }
+
+ template <typename SampleType>
+ void RecordEnumerationHistogram(const std::string& name,
+ SampleType sample,
+ SampleType boundary) const {
+ // See HISTOGRAM_ENUMERATION_WITH_FLAG.
+ base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
+ FullName(name), 1, boundary, boundary + 1,
+ base::HistogramBase::kUmaTargetedHistogramFlag);
+ if (histogram)
+ histogram->Add(sample);
+ }
+
+ template <typename SampleType>
+ void RecordLongTimesHistogram(const std::string& name,
+ SampleType sample) const {
+ // See UMA_HISTOGRAM_LONG_TIMES.
+ base::HistogramBase* histogram = base::Histogram::FactoryTimeGet(
+ FullName(name), base::TimeDelta::FromMilliseconds(1),
+ base::TimeDelta::FromHours(1), 100,
+ base::HistogramBase::kUmaTargetedHistogramFlag);
+ if (histogram)
+ histogram->AddTime(sample);
+ }
+
+ template <typename SampleType>
+ void RecordMemoryKBHistogram(const std::string& name,
+ SampleType sample) const {
+ // See UMA_HISTOGRAM_MEMORY_KB.
+ base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
+ FullName(name), 1000, 500000, 50,
+ base::HistogramBase::kUmaTargetedHistogramFlag);
+ if (histogram)
+ histogram->Add(sample);
+ }
+
+ template <typename SampleType>
+ void RecordSparseHistogram(const std::string& name, SampleType sample) const {
+ RecordHistogram<base::SparseHistogram, SampleType>(name, sample);
+ }
+
+ template <typename SampleType>
+ void RecordSparseHistogramCount(const std::string& name,
+ SampleType sample,
+ int count) const {
+ base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
+ FullName(name), base::HistogramBase::kUmaTargetedHistogramFlag);
+ if (histogram)
+ histogram->AddCount(sample, count);
+ }
+
+ std::string suffix_;
+ std::wstring registry_key_;
};
void RecordReporterStepHistogram(SwReporterUmaValue value) {
- UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.Step", value, SW_REPORTER_MAX);
+ UMAHistogramReporter uma;
+ uma.RecordReporterStep(value);
}
void DisplaySRTPrompt(const base::FilePath& download_path) {
@@ -371,12 +457,13 @@ int LaunchAndWaitForExit(const SwReporterInvocation& invocation) {
// This exit code is used to identify that a reporter run didn't happen, so
// the result should be ignored and a rerun scheduled for the usual delay.
int exit_code = kReporterFailureExitCode;
+ UMAHistogramReporter uma(invocation.suffix);
if (reporter_process.IsValid()) {
- RecordReporterStepHistogram(SW_REPORTER_START_EXECUTION);
+ uma.RecordReporterStep(SW_REPORTER_START_EXECUTION);
bool success = reporter_process.WaitForExit(&exit_code);
DCHECK(success);
} else {
- RecordReporterStepHistogram(SW_REPORTER_FAILED_TO_START);
+ uma.RecordReporterStep(SW_REPORTER_FAILED_TO_START);
}
return exit_code;
}
@@ -512,7 +599,7 @@ class ReporterRunner : public chrome::BrowserListObserver {
public:
// Starts the sequence of attempts to run the reporter.
static void Run(
- const SwReporterInvocation& invocation,
+ const std::queue<SwReporterInvocation>& invocations,
const base::Version& version,
const scoped_refptr<base::TaskRunner>& main_thread_task_runner,
const scoped_refptr<base::TaskRunner>& blocking_task_runner) {
@@ -521,11 +608,11 @@ class ReporterRunner : public chrome::BrowserListObserver {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
// There's nothing to do if the path and version of the reporter has not
// changed, we just keep running the tasks that are running now.
- if (instance_->invocation_ == invocation && instance_->version_.IsValid() &&
- instance_->version_ == version)
+ if (instance_->invocations_ == invocations &&
+ instance_->version_.IsValid() && instance_->version_ == version)
return;
- instance_->invocation_ = invocation;
+ instance_->invocations_ = invocations;
instance_->version_ = version;
instance_->main_thread_task_runner_ = main_thread_task_runner;
instance_->blocking_task_runner_ = blocking_task_runner;
@@ -536,6 +623,34 @@ class ReporterRunner : public chrome::BrowserListObserver {
}
}
+ // Launch the command line at the head of the queue.
+ void LaunchNextInvocation(
+ const std::queue<SwReporterInvocation>& invocations) {
+ DCHECK(!invocations.empty());
+ // Make a non-const copy of the queue so it can be manipulated.
+ auto remaining_invocations = invocations;
+ auto next_invocation = remaining_invocations.front();
+ remaining_invocations.pop();
+
+ // It's OK to simply |PostTaskAndReplyWithResult| so that
+ // |LaunchAndWaitForExit| doesn't need to access |main_thread_task_runner_|
+ // since the callback is not delayed and the test task runner won't need to
+ // force it.
+ base::PostTaskAndReplyWithResult(
+ blocking_task_runner_.get(), FROM_HERE,
+
+ // Send the head of the queue to the launch callback.
+ base::Bind(&LaunchAndWaitForExit, next_invocation),
+
+ // Send the tail of the queue to the next callback, which will handle
+ // the
+ // results of the first launch and then recursively launch everything in
+ // the tail.
+ base::Bind(&ReporterRunner::ReporterDone, base::Unretained(this),
+ base::Time::Now(), version_, next_invocation,
+ remaining_invocations));
+ }
+
private:
ReporterRunner() : first_run_(true) {}
~ReporterRunner() override {}
@@ -553,9 +668,12 @@ class ReporterRunner : public chrome::BrowserListObserver {
// This method is called on the UI thread when the reporter run has completed.
// This is run as a task posted from an interruptible worker thread so should
// be resilient to unexpected shutdown.
- void ReporterDone(const base::Time& reporter_start_time,
- const base::Version& version,
- int exit_code) {
+ void ReporterDone(
+ const base::Time& reporter_start_time,
+ const base::Version& version,
+ const SwReporterInvocation& finished_invocation,
+ const std::queue<SwReporterInvocation>& remaining_invocations,
+ int exit_code) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (g_reporter_done_notifier_)
@@ -573,12 +691,14 @@ class ReporterRunner : public chrome::BrowserListObserver {
if (exit_code == kReporterFailureExitCode)
return;
- UMAHistogramReporter uma;
+ UMAHistogramReporter uma(finished_invocation.suffix);
uma.ReportVersion(version);
uma.ReportExitCode(exit_code);
- uma.ReportFoundUwS();
+ uma.ReportFoundUwS(!finished_invocation.is_experimental /*use_rappor*/);
+
+ // Only save results from the canonical version of the software.
PrefService* local_state = g_browser_process->local_state();
- if (local_state) {
+ if (local_state && !finished_invocation.is_experimental) {
local_state->SetInteger(prefs::kSwReporterLastExitCode, exit_code);
local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
base::Time::Now().ToInternalValue());
@@ -587,16 +707,26 @@ class ReporterRunner : public chrome::BrowserListObserver {
uma.ReportScanTimes();
uma.ReportMemoryUsage();
+ if (!remaining_invocations.empty()) {
+ // Only the experimental version should have multiple invocations.
+ DCHECK(finished_invocation.is_experimental);
+ LaunchNextInvocation(remaining_invocations);
+ }
+
+ // Only continue to launch the prompt for the canonical version.
+ if (finished_invocation.is_experimental)
+ return;
+
if (!IsInSRTPromptFieldTrialGroups()) {
// Knowing about disabled field trial is more important than reporter not
// finding anything to remove, so check this case first.
- RecordReporterStepHistogram(SW_REPORTER_NO_PROMPT_FIELD_TRIAL);
+ uma.RecordReporterStep(SW_REPORTER_NO_PROMPT_FIELD_TRIAL);
return;
}
if (exit_code != kSwReporterPostRebootCleanupNeeded &&
exit_code != kSwReporterCleanupNeeded) {
- RecordReporterStepHistogram(SW_REPORTER_NO_PROMPT_NEEDED);
+ uma.RecordReporterStep(SW_REPORTER_NO_PROMPT_NEEDED);
return;
}
@@ -608,7 +738,7 @@ class ReporterRunner : public chrome::BrowserListObserver {
// decide when it's time to download the SRT and when to display the prompt.
Browser* browser = chrome::FindLastActive();
if (!browser) {
- RecordReporterStepHistogram(SW_REPORTER_NO_BROWSER);
+ uma.RecordReporterStep(SW_REPORTER_NO_BROWSER);
BrowserList::AddObserver(this);
} else {
MaybeFetchSRT(browser, version_);
@@ -645,15 +775,8 @@ class ReporterRunner : public chrome::BrowserListObserver {
if (g_launch_ready_notifier_)
g_launch_ready_notifier_.Run();
- // It's OK to simply |PostTaskAndReplyWithResult| so that
- // |LaunchAndWaitForExit| doesn't need to access
- // |main_thread_task_runner_| since the callback is not delayed and the
- // test task runner won't need to force it.
- base::PostTaskAndReplyWithResult(
- blocking_task_runner_.get(), FROM_HERE,
- base::Bind(&LaunchAndWaitForExit, invocation_),
- base::Bind(&ReporterRunner::ReporterDone, base::Unretained(this),
- base::Time::Now(), version_));
+ if (!invocations_.empty())
+ LaunchNextInvocation(invocations_);
} else {
main_thread_task_runner_->PostDelayedTask(
FROM_HERE,
@@ -663,7 +786,7 @@ class ReporterRunner : public chrome::BrowserListObserver {
}
bool first_run_;
- SwReporterInvocation invocation_;
+ std::queue<SwReporterInvocation> invocations_;
base::Version version_;
scoped_refptr<base::TaskRunner> main_thread_task_runner_;
scoped_refptr<base::TaskRunner> blocking_task_runner_;
@@ -684,24 +807,36 @@ ReporterRunner* ReporterRunner::instance_ = nullptr;
} // namespace
-SwReporterInvocation::SwReporterInvocation() : command_line(0, nullptr) {}
+SwReporterInvocation::SwReporterInvocation()
+ : command_line(0, nullptr), is_experimental(false) {}
SwReporterInvocation::SwReporterInvocation(const base::FilePath& exe_path)
- : command_line(exe_path) {}
+ : command_line(exe_path), is_experimental(false) {}
SwReporterInvocation::SwReporterInvocation(
const base::CommandLine& command_line)
- : command_line(command_line) {}
+ : command_line(command_line), is_experimental(false) {}
bool SwReporterInvocation::operator==(const SwReporterInvocation& other) const {
- return command_line.argv() == other.command_line.argv();
+ return command_line.argv() == other.command_line.argv() &&
+ suffix == other.suffix && is_experimental == other.is_experimental;
}
void RunSwReporter(const SwReporterInvocation& invocation,
const base::Version& version,
scoped_refptr<base::TaskRunner> main_thread_task_runner,
scoped_refptr<base::TaskRunner> blocking_task_runner) {
- ReporterRunner::Run(invocation, version, main_thread_task_runner,
+ std::queue<SwReporterInvocation> invocations;
+ invocations.push(invocation);
+ ReporterRunner::Run(invocations, version, main_thread_task_runner,
+ blocking_task_runner);
+}
+
+void RunSwReporters(const std::queue<SwReporterInvocation>& invocations,
+ const base::Version& version,
+ scoped_refptr<base::TaskRunner> main_thread_task_runner,
+ scoped_refptr<base::TaskRunner> blocking_task_runner) {
+ ReporterRunner::Run(invocations, version, main_thread_task_runner,
blocking_task_runner);
}

Powered by Google App Engine
This is Rietveld 408576698