Can now adjust the number of retries before the blacklist is disabled.

Also added UMA recording of the blacklist disabled and retry succeeded events.  

Deprecating the blacklist start up events 'BLACKLIST_THUNK_SETUP_FAILED' and 
'BLACKLIST_INTERCEPTION_FAILED' as we are switching over to a more coarse 
grained recording of the set up status (this information can be gathered from 
crash reporting now anyways).

BUG=

Review URL: https://codereview.chromium.org/311893005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@277614 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chrome_elf_init_unittest_win.cc

@@ -117,55 +117,29 @@ TEST_F(ChromeBlacklistTrialTest, VerifyFirstRun) {
   ASSERT_EQ(version, GetBlacklistVersion());
 }
 
-TEST_F(ChromeBlacklistTrialTest, SetupFailed) {
-  // Set the registry to indicate that the blacklist setup is running,
-  // which means it failed to run correctly last time for this version.
+TEST_F(ChromeBlacklistTrialTest, BlacklistFailed) {
+  // Ensure when the blacklist set up failed we set the state to disabled for
+  // future runs.
   blacklist_registry_key_->WriteValue(blacklist::kBeaconVersion,
                                       TEXT(CHROME_VERSION_STRING));
   blacklist_registry_key_->WriteValue(blacklist::kBeaconState,
-                                      blacklist::BLACKLIST_SETUP_RUNNING);
-
-   BrowserBlacklistBeaconSetup();
-
-  // Since the blacklist setup failed, it should now be disabled.
-  ASSERT_EQ(blacklist::BLACKLIST_DISABLED, GetBlacklistState());
-}
-
-TEST_F(ChromeBlacklistTrialTest, ThunkSetupFailed) {
-  // Set the registry to indicate that the blacklist thunk setup is running,
-  // which means it failed to run correctly last time for this version.
-  blacklist_registry_key_->WriteValue(blacklist::kBeaconVersion,
-                                      TEXT(CHROME_VERSION_STRING));
-  blacklist_registry_key_->WriteValue(blacklist::kBeaconState,
-                                      blacklist::BLACKLIST_THUNK_SETUP);
-
-  BrowserBlacklistBeaconSetup();
-
-  // Since the blacklist thunk setup failed, it should now be disabled.
-  ASSERT_EQ(blacklist::BLACKLIST_DISABLED, GetBlacklistState());
-}
-
-TEST_F(ChromeBlacklistTrialTest, InterceptionFailed) {
-  // Set the registry to indicate that an interception is running,
-  // which means it failed to run correctly last time for this version.
-  blacklist_registry_key_->WriteValue(blacklist::kBeaconVersion,
-                                      TEXT(CHROME_VERSION_STRING));
-  blacklist_registry_key_->WriteValue(blacklist::kBeaconState,
-                                      blacklist::BLACKLIST_INTERCEPTING);
+                                      blacklist::BLACKLIST_SETUP_FAILED);
 
   BrowserBlacklistBeaconSetup();
 
-  // Since an interception failed, the blacklist should now be disabled.
   ASSERT_EQ(blacklist::BLACKLIST_DISABLED, GetBlacklistState());
 }
 
 TEST_F(ChromeBlacklistTrialTest, VersionChanged) {
-  // Mark the blacklist as disabled for an older version, so it should
-  // get enabled for this new version.
+  // Mark the blacklist as disabled for an older version, it should
+  // get enabled for this new version.  Also record a non-zero number of
+  // setup failures, which should be reset to zero.
   blacklist_registry_key_->WriteValue(blacklist::kBeaconVersion,
                                       L"old_version");
   blacklist_registry_key_->WriteValue(blacklist::kBeaconState,
                                       blacklist::BLACKLIST_DISABLED);
+  blacklist_registry_key_->WriteValue(blacklist::kBeaconAttemptCount,
+                                      blacklist::kBeaconMaxAttempts);
 
   BrowserBlacklistBeaconSetup();
 
@@ -175,6 +149,12 @@ TEST_F(ChromeBlacklistTrialTest, VersionChanged) {
   chrome::VersionInfo version_info;
   base::string16 expected_version(base::UTF8ToUTF16(version_info.Version()));
   ASSERT_EQ(expected_version, GetBlacklistVersion());
+
+  // The counter should be reset.
+  DWORD attempt_count = blacklist::kBeaconMaxAttempts;
+  blacklist_registry_key_->ReadValueDW(blacklist::kBeaconAttemptCount,
+                                       &attempt_count);
+  ASSERT_EQ(static_cast<DWORD>(0), attempt_count);
 }
 
 TEST_F(ChromeBlacklistTrialTest, AddFinchBlacklistToRegistry) {

chrome/browser/chrome_elf_init_win.cc

@@ -40,12 +40,15 @@ enum BlacklistSetupEventType {
   // The blacklist setup code failed to execute.
   BLACKLIST_SETUP_FAILED,
 
-  // The blacklist thunk setup code failed to execute.
+  // Deprecated. The blacklist thunk setup code failed to execute.
   BLACKLIST_THUNK_SETUP_FAILED,
 
-  // The blacklist interception code failed to execute.
+  // Deprecated. The blacklist interception code failed to execute.
   BLACKLIST_INTERCEPTION_FAILED,
 
+  // The blacklist was disabled for this run (after it failed too many times).
+  BLACKLIST_SETUP_DISABLED,
+
   // Always keep this at the end.
   BLACKLIST_SETUP_EVENT_MAX,
 };
@@ -140,26 +143,21 @@ void BrowserBlacklistBeaconSetup() {
   blacklist_registry_key.ReadValueDW(blacklist::kBeaconState, &blacklist_state);
 
   if (blacklist_state == blacklist::BLACKLIST_ENABLED) {
+    // The blacklist was enabled successfully so we record the event (along with
+    // the number of failed previous attempts).
     RecordBlacklistSetupEvent(BLACKLIST_SETUP_RAN_SUCCESSFULLY);
-  } else {
-    switch (blacklist_state) {
-      case blacklist::BLACKLIST_SETUP_RUNNING:
-        RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);
-        break;
-      case blacklist::BLACKLIST_THUNK_SETUP:
-        RecordBlacklistSetupEvent(BLACKLIST_THUNK_SETUP_FAILED);
-        break;
-      case blacklist::BLACKLIST_INTERCEPTING:
-        RecordBlacklistSetupEvent(BLACKLIST_INTERCEPTION_FAILED);
-        break;
-    }
-
-    // Since some part of the blacklist failed, mark it as disabled
-    // for this version.
-    if (blacklist_state != blacklist::BLACKLIST_DISABLED) {
-      blacklist_registry_key.WriteValue(blacklist::kBeaconState,
-                                        blacklist::BLACKLIST_DISABLED);
-    }
+    DWORD attempt_count = 0;
+    blacklist_registry_key.ReadValueDW(blacklist::kBeaconAttemptCount,
+                                       &attempt_count);
+    UMA_HISTOGRAM_COUNTS_100("Blacklist.RetryAttempts.Success", attempt_count);
+  } else if (blacklist_state == blacklist::BLACKLIST_SETUP_FAILED) {
+    // We can set the state to disabled without checking that the maximum number
+    // of attempts was exceeded because blacklist.cc has already done this.
+    RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);
+    blacklist_registry_key.WriteValue(blacklist::kBeaconState,
+                                      blacklist::BLACKLIST_DISABLED);
+  } else if (blacklist_state == blacklist::BLACKLIST_DISABLED) {
+    RecordBlacklistSetupEvent(BLACKLIST_SETUP_DISABLED);
   }
 
   // Find the last recorded blacklist version.
@@ -168,7 +166,8 @@ void BrowserBlacklistBeaconSetup() {
                                    &blacklist_version);
 
   if (blacklist_version != TEXT(CHROME_VERSION_STRING)) {
-    // The blacklist hasn't been enabled for this version yet, so enable it.
+    // The blacklist hasn't been enabled for this version yet, so enable it
+    // and reset the failure count to zero.
     LONG set_version = blacklist_registry_key.WriteValue(
         blacklist::kBeaconVersion,
         TEXT(CHROME_VERSION_STRING));
@@ -177,6 +176,9 @@ void BrowserBlacklistBeaconSetup() {
         blacklist::kBeaconState,
         blacklist::BLACKLIST_ENABLED);
 
+    blacklist_registry_key.WriteValue(blacklist::kBeaconAttemptCount,
+                                      static_cast<DWORD>(0));
+
     // Only report the blacklist as getting setup when both registry writes
     // succeed, since otherwise the blacklist wasn't properly setup.
     if (set_version == ERROR_SUCCESS && set_state == ERROR_SUCCESS)

chrome_elf/blacklist/blacklist.cc

@@ -59,20 +59,54 @@ namespace {
 // determine if the blacklist is enabled for them.
 bool g_blacklist_initialized = false;
 
-// Record that the thunk setup completed succesfully and close the registry
-// key handle since it is no longer needed.
-void RecordSuccessfulThunkSetup(HKEY* key) {
-  if (key != NULL) {
-    DWORD blacklist_state = blacklist::BLACKLIST_SETUP_RUNNING;
-    ::RegSetValueEx(*key,
-                    blacklist::kBeaconState,
-                    0,
-                    REG_DWORD,
-                    reinterpret_cast<LPBYTE>(&blacklist_state),
-                    sizeof(blacklist_state));
-    ::RegCloseKey(*key);
-    key = NULL;
+// Helper to set DWORD registry values.
+DWORD SetDWValue(HKEY* key, const wchar_t* property, DWORD value) {
+  return ::RegSetValueEx(*key,
+                         property,
+                         0,
+                         REG_DWORD,
+                         reinterpret_cast<LPBYTE>(&value),
+                         sizeof(value));
+}
+
+bool GenerateStateFromBeaconAndAttemptCount(HKEY* key, DWORD blacklist_state) {
+  LONG result = 0;
+  if (blacklist_state == blacklist::BLACKLIST_SETUP_RUNNING) {
+    // Some part of the blacklist setup failed last time.  If this has occured
+    // blacklist::kBeaconMaxAttempts times in a row we switch the state to
+    // failed and skip setting up the blacklist.
+    DWORD attempt_count = 0;
+    DWORD attempt_count_size = sizeof(attempt_count);
+    result = ::RegQueryValueEx(*key,
+                               blacklist::kBeaconAttemptCount,
+                               0,
+                               NULL,
+                               reinterpret_cast<LPBYTE>(&attempt_count),
+                               &attempt_count_size);
+
+    if (result == ERROR_FILE_NOT_FOUND)
+      attempt_count = 0;
+    else if (result != ERROR_SUCCESS)
+      return false;
+
+    ++attempt_count;
+    SetDWValue(key, blacklist::kBeaconAttemptCount, attempt_count);
+
+    if (attempt_count >= blacklist::kBeaconMaxAttempts) {
+      blacklist_state = blacklist::BLACKLIST_SETUP_FAILED;
+      SetDWValue(key, blacklist::kBeaconState, blacklist_state);
+      return false;
+    }
+  } else if (blacklist_state == blacklist::BLACKLIST_ENABLED) {
+    // If the blacklist succeeded on the previous run reset the failure
+    // counter.
+    result =
+        SetDWValue(key, blacklist::kBeaconAttemptCount, static_cast<DWORD>(0));
+    if (result != ERROR_SUCCESS) {
+      return false;
+    }
   }
+  return true;
 }
 
 }  // namespace
@@ -102,7 +136,7 @@ bool LeaveSetupBeacon() {
     return false;
 
   // Retrieve the current blacklist state.
-  DWORD blacklist_state = BLACKLIST_DISABLED;
+  DWORD blacklist_state = BLACKLIST_STATE_MAX;
   DWORD blacklist_state_size = sizeof(blacklist_state);
   DWORD type = 0;
   result = ::RegQueryValueEx(key,
@@ -112,21 +146,18 @@ bool LeaveSetupBeacon() {
                              reinterpret_cast<LPBYTE>(&blacklist_state),
                              &blacklist_state_size);
 
-  if (blacklist_state != BLACKLIST_ENABLED ||
-      result != ERROR_SUCCESS || type != REG_DWORD) {
+  if (blacklist_state == BLACKLIST_DISABLED || result != ERROR_SUCCESS ||
+      type != REG_DWORD) {
+    ::RegCloseKey(key);
+    return false;
+  }
+
+  if (!GenerateStateFromBeaconAndAttemptCount(&key, blacklist_state)) {
     ::RegCloseKey(key);
     return false;
   }
 
-  // Mark the blacklist setup code as running so if it crashes the blacklist
-  // won't be enabled for the next run.
-  blacklist_state = BLACKLIST_SETUP_RUNNING;
-  result = ::RegSetValueEx(key,
-                           kBeaconState,
-                           0,
-                           REG_DWORD,
-                           reinterpret_cast<LPBYTE>(&blacklist_state),
-                           sizeof(blacklist_state));
+  result = SetDWValue(&key, kBeaconState, BLACKLIST_SETUP_RUNNING);
   ::RegCloseKey(key);
 
   return (result == ERROR_SUCCESS);
@@ -147,15 +178,28 @@ bool ResetBeacon() {
   if (result != ERROR_SUCCESS)
     return false;
 
-  DWORD blacklist_state = BLACKLIST_ENABLED;
-  result = ::RegSetValueEx(key,
-                           kBeaconState,
-                           0,
-                           REG_DWORD,
-                           reinterpret_cast<LPBYTE>(&blacklist_state),
-                           sizeof(blacklist_state));
-  ::RegCloseKey(key);
+  DWORD blacklist_state = BLACKLIST_STATE_MAX;
+  DWORD blacklist_state_size = sizeof(blacklist_state);
+  DWORD type = 0;
+  result = ::RegQueryValueEx(key,
+                             kBeaconState,
+                             0,
+                             &type,
+                             reinterpret_cast<LPBYTE>(&blacklist_state),
+                             &blacklist_state_size);
 
+  if (result != ERROR_SUCCESS || type != REG_DWORD) {
+    ::RegCloseKey(key);
+    return false;
+  }
+
+  // Reaching this point with the setup running state means the setup
+  // succeeded and so we reset to enabled.  Any other state indicates that setup
+  // was skipped; in that case we leave the state alone for later recording.
+  if (blacklist_state == BLACKLIST_SETUP_RUNNING)
+    result = SetDWValue(&key, kBeaconState, BLACKLIST_ENABLED);
+
+  ::RegCloseKey(key);
   return (result == ERROR_SUCCESS);
 }
 
@@ -253,7 +297,8 @@ bool Initialize(bool force) {
   if (IsNonBrowserProcess())
     return false;
 
-  // Check to see if a beacon is present, abort if so.
+  // Check to see if the blacklist beacon is still set to running (indicating a
+  // failure) or disabled, and abort if so.
   if (!force && !LeaveSetupBeacon())
     return false;
 
@@ -268,30 +313,6 @@ bool Initialize(bool force) {
   if (!thunk)
     return false;
 
-  // Record that we are starting the thunk setup code.
-  HKEY key = NULL;
-  DWORD disposition = 0;
-  LONG result = ::RegCreateKeyEx(HKEY_CURRENT_USER,
-                                 kRegistryBeaconPath,
-                                 0,
-                                 NULL,
-                                 REG_OPTION_NON_VOLATILE,
-                                 KEY_QUERY_VALUE | KEY_SET_VALUE,
-                                 NULL,
-                                 &key,
-                                 &disposition);
-  if (result == ERROR_SUCCESS) {
-    DWORD blacklist_state = BLACKLIST_THUNK_SETUP;
-    ::RegSetValueEx(key,
-                    kBeaconState,
-                    0,
-                    REG_DWORD,
-                    reinterpret_cast<LPBYTE>(&blacklist_state),
-                    sizeof(blacklist_state));
-  } else {
-    key = NULL;
-  }
-
   BYTE* thunk_storage = reinterpret_cast<BYTE*>(&g_thunk_storage);
 
   // Mark the thunk storage as readable and writeable, since we
@@ -301,7 +322,6 @@ bool Initialize(bool force) {
                       sizeof(g_thunk_storage),
                       PAGE_EXECUTE_READWRITE,
                       &old_protect)) {
-    RecordSuccessfulThunkSetup(&key);
     return false;
   }
 
@@ -353,8 +373,6 @@ bool Initialize(bool force) {
                                                       PAGE_EXECUTE_READ,
                                                       &old_protect);
 
-  RecordSuccessfulThunkSetup(&key);
-
   AddDllsFromRegistryToBlacklist();
 
   return NT_SUCCESS(ret) && page_executable;

chrome_elf/blacklist/blacklist.h

@@ -21,16 +21,16 @@ extern const wchar_t* g_troublesome_dlls[kTroublesomeDllsMaxCount];
 extern NtMapViewOfSectionFunction g_nt_map_view_of_section_func;
 #endif
 
-// Attempts to leave a beacon in the current user's registry hive.
-// If the blacklist beacon doesn't say it is enabled or there are any other
-// errors when creating the beacon, returns false. Otherwise returns true.
-// The intent of the beacon is to act as an extra failure mode protection
-// whereby if Chrome for some reason fails to start during blacklist setup,
-// it will skip blacklisting on the subsequent run.
+// Attempts to leave a beacon in the current user's registry hive. If the
+// blacklist beacon doesn't say it is enabled or there are any other errors when
+// creating the beacon, returns false. Otherwise returns true. The intent of the
+// beacon is to act as an extra failure mode protection whereby if Chrome
+// repeatedly fails to start during blacklist setup, it will skip blacklisting
+// on the subsequent run.
 bool LeaveSetupBeacon();
 
-// Looks for the beacon that LeaveSetupBeacon() creates and resets it to
-// to show the setup was successful.
+// Looks for the setup running beacon that LeaveSetupBeacon() creates and resets
+// it to to show the setup was successful.
 // Returns true if the beacon was successfully set to BLACKLIST_ENABLED.
 bool ResetBeacon();