Add a triggered profile reset mechanism.

chrome/browser/profile_resetter/triggered_profile_resetter_win.cc

+// Copyright 2015 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.
+
+#include "chrome/browser/profile_resetter/triggered_profile_resetter.h"
+
+#include "base/metrics/histogram_macros.h"
+#include "base/prefs/pref_service.h"
+#include "base/win/registry.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/pref_names.h"
+
+// The registry path where the TriggeredReset values get set. Note that this

[grt (UTC plus 2), 2015/09/14 14:35:34]

wrap these constants in the unnamed namespace

[robertshield, 2015/09/21 04:16:23]

Done.

+// uses the same path for both SxS (Canary) and non-SxS Chrome.
+const wchar_t kTriggeredResetRegistryPath[] =
+    L"Software\\" PRODUCT_STRING_PATH L"\\TriggeredReset";
+
+const wchar_t kTriggeredResetToolName[] = L"ToolName";
+const wchar_t kTriggeredResetTimestamp[] = L"Timestamp";
+
+void TriggeredProfileResetter::Activate() {

[engedy, 2015/09/14 23:28:56]

I'm sad that this code is not tested at all. Could it be? :-)

[robertshield, 2015/09/21 04:16:23]

Done.

+#if DCHECK_IS_ON()
+  activate_called_ = true;
+#endif
+
+  // System profiles don't contain user settings.
+  if (!profile_ || profile_->IsSystemProfile())

[engedy, 2015/09/14 23:28:56]

I am not sure about the default behavior of BCKS creation for Incognito
profiles, I assume you have verified that it does not mess with any of the logic
here?

[robertshield, 2015/09/21 04:16:23]

Yep, incognito mode does work correctly.

[engedy, 2015/09/21 12:56:10]

Acknowledged.

+    return;
+
+  base::win::RegKey reset_reg_key(HKEY_CURRENT_USER,
+                                  kTriggeredResetRegistryPath,
+                                  KEY_QUERY_VALUE | KEY_SET_VALUE);

[engedy, 2015/09/14 16:08:29]

nit: I think KEY_SET_VALUE is not needed.

[robertshield, 2015/09/21 04:16:23]

Done.

+  if (!reset_reg_key.Valid())
+    return;
+
+  int64 timestamp = 0;
+  if (reset_reg_key.ReadInt64(kTriggeredResetTimestamp, &timestamp) !=
+      ERROR_SUCCESS) {
+    return;
+  }
+
+  // A reset trigger time was found. Compare it to the trigger time stored
+  // in this profile. If different, reset the profile and persist the new
+  // time.
+  PrefService* pref_service = profile_->GetPrefs();
+  const int64 preference_timestamp =
+      pref_service->GetInt64(prefs::kLastProfileResetTimestamp);
+
+  if (profile_->IsNewProfile()) {
+    // New profiles should never be reset. Instead, persist the time stamp
+    // directly.
+    pref_service->SetInt64(prefs::kLastProfileResetTimestamp, timestamp);
+  } else if (timestamp != preference_timestamp) {

[engedy, 2015/09/14 16:08:29]

Trying to think of any edge cases here. Any reasons not to check "is greater"
here?

[robertshield, 2015/09/21 04:16:23]

I didn't do that since the machine clock could well shift, causing a future run
of a cleanup tool to not trigger a reset (if the machine clock gets set back)
or, worse, a profile to be repeatedly reset (if the machine clock goes forward).


I figure strict equality is likely to handle most such cases.

[engedy, 2015/09/21 12:56:10]

Fair enough, I just wanted to make sure this has been considered.

[robertshield, 2015/09/21 22:04:30]

Thanks!

+    DVLOG(1) << "Profile reset detected.";
+
+    has_reset_trigger_ = true;
+    UMA_HISTOGRAM_BOOLEAN("Profile.TriggeredReset", true);

[engedy, 2015/09/14 16:08:29]

I think it could be more useful to record a histogram sample every time this
function is exercised  (i.e., record falses value into the histogram as well),
so it would give a reference value as well intrinsically.

[Alexei Svitkine (slow), 2015/09/14 17:05:43]

Agree.

[robertshield, 2015/09/21 04:16:23]

Ok, figured we'd have this reference value implicitly via one of the profile
creation metrics (e.g. Profile.CreateAndInitializeProfile), but I will add it
here as well.

[robertshield, 2015/09/21 04:16:23]

Acknowledged.

+
+    if (reset_reg_key.ReadValue(kTriggeredResetToolName, &tool_name_) !=
+        ERROR_SUCCESS) {
+      DVLOG(1) << "Failed to read triggered profile reset tool name.";

[engedy, 2015/09/14 16:08:29]

I know this is a edge case, but wouldn't it be better to just abort in this
case? The UI filled in with an empty string in place of the AV tool will
probably not be very nice.

[robertshield, 2015/09/21 04:16:23]

I had planned to have a default placeholder of "A third party tool", though
perhaps aborting works too. Let me leave it as is and I'll abort instead if the
UX folk don't like having a default. 

[engedy, 2015/09/21 12:56:10]

Sounds good to me.

[robertshield, 2015/09/21 22:04:30]

Acknowledged.

+    } else if (tool_name_.length() > kMaxToolNameLength) {
+      tool_name_.resize(kMaxToolNameLength);
+    }
+
+    pref_service->SetInt64(prefs::kLastProfileResetTimestamp, timestamp);
+  }
+}