Add New Histogram Stats to see where Blacklist Failures are Occuring

We seem to be seeing more setup failures than expected so add so new
UMA stats to try and narrow down where the problem is occuring.

(This crashes would occur before breakpad is loaded so we don't have
any other way to gather this data for the moment).

NOTRY=True
TBR=sky@chromium.org, asvitkine@chromium.org
BUG=329023

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

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

6 files changed, 105 insertions, 11 deletions

diff --git a/chrome/browser/chrome_elf_init_unittest_win.cc b/chrome/browser/chrome_elf_init_unittest_win.cc
index e08f69f..2cf68b4 100644
--- a/chrome/browser/chrome_elf_init_unittest_win.cc
+++ b/chrome/browser/chrome_elf_init_unittest_win.cc
@@ -92,6 +92,30 @@ TEST_F(ChromeBlacklistTrialTest, SetupFailed) {
   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.
+  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.
+  blacklist_registry_key_.WriteValue(blacklist::kBeaconState,
+                                     blacklist::BLACKLIST_INTERCEPTING);
+
+  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.
diff --git a/chrome/browser/chrome_elf_init_win.cc b/chrome/browser/chrome_elf_init_win.cc
index 672b0a3..424f533 100644
--- a/chrome/browser/chrome_elf_init_win.cc
+++ b/chrome/browser/chrome_elf_init_win.cc
@@ -30,6 +30,12 @@ enum BlacklistSetupEventType {
   // The blacklist setup code failed to execute.
   BLACKLIST_SETUP_FAILED,
 
+  // The blacklist thunk setup code failed to execute.
+  BLACKLIST_THUNK_SETUP_FAILED,
+
+  // The blacklist interception code failed to execute.
+  BLACKLIST_INTERCEPTION_FAILED,
+
   // Always keep this at the end.
   BLACKLIST_SETUP_EVENT_MAX,
 };
@@ -91,13 +97,25 @@ void BrowserBlacklistBeaconSetup() {
     // Record the results of the latest blacklist setup.
     if (blacklist_state == blacklist::BLACKLIST_ENABLED) {
       RecordBlacklistSetupEvent(BLACKLIST_SETUP_RAN_SUCCESSFULLY);
-    } else if (blacklist_state == blacklist::BLACKLIST_SETUP_RUNNING) {
-      RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);
-
-      // Since the setup failed, mark the blacklist as disabled so we don't
-      // try it again for this version.
-      blacklist_registry_key.WriteValue(blacklist::kBeaconState,
-                                        blacklist::BLACKLIST_DISABLED);
+    } 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, ensure it is now disabled
+      // for this version.
+      if (blacklist_state != blacklist::BLACKLIST_DISABLED) {
+        blacklist_registry_key.WriteValue(blacklist::kBeaconState,
+                                          blacklist::BLACKLIST_DISABLED);
+      }
     }
   }
 }
diff --git a/chrome_elf/blacklist/blacklist.cc b/chrome_elf/blacklist/blacklist.cc
index f9eb065..34a498e 100644
--- a/chrome_elf/blacklist/blacklist.cc
+++ b/chrome_elf/blacklist/blacklist.cc
@@ -142,6 +142,22 @@ bool IsNonBrowserProcess() {
   return (command_line && wcsstr(command_line, L"--type"));
 }
 
+// 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;
+  }
+}
+
 }  // namespace
 
 namespace blacklist {
@@ -238,7 +254,7 @@ bool ResetBeacon() {
 
 int BlacklistSize() {
   int size = -1;
-  while(blacklist::g_troublesome_dlls[++size] != NULL);
+  while (blacklist::g_troublesome_dlls[++size] != NULL) {}
 
   return size;
 }
@@ -248,7 +264,7 @@ bool AddDllToBlacklist(const wchar_t* dll_name) {
   // We need to leave one space at the end for the null pointer.
   if (blacklist_size + 1 >= kTroublesomeDllsMaxCount)
     return false;
-  for (int i=0; i < blacklist_size; ++i) {
+  for (int i = 0; i < blacklist_size; ++i) {
     if (!_wcsicmp(g_troublesome_dlls[i], dll_name))
       return true;
   }
@@ -306,6 +322,30 @@ bool Initialize(bool force) {
   // Tells the resolver to patch already patched functions.
   const bool kRelaxed = true;
 
+  // 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;
+  }
+
   // Create a thunk via the appropriate ServiceResolver instance.
   sandbox::ServiceResolverThunk* thunk;
 #if defined(_WIN64)
@@ -332,8 +372,10 @@ bool Initialize(bool force) {
   if (!VirtualProtect(&g_thunk_storage,
                       sizeof(g_thunk_storage),
                       PAGE_EXECUTE_READWRITE,
-                      &old_protect))
+                      &old_protect)) {
+    RecordSuccessfulThunkSetup(&key);
     return false;
+  }
 
   thunk->AllowLocalPatches();
 
@@ -355,6 +397,8 @@ bool Initialize(bool force) {
                                         PAGE_EXECUTE_READ,
                                         &old_protect);
 
+  RecordSuccessfulThunkSetup(&key);
+
   return NT_SUCCESS(ret) && page_executable;
 }
 
diff --git a/chrome_elf/blacklist/blacklist.h b/chrome_elf/blacklist/blacklist.h
index 17a511d..7e01354 100644
--- a/chrome_elf/blacklist/blacklist.h
+++ b/chrome_elf/blacklist/blacklist.h
@@ -27,6 +27,12 @@ enum BlacklistState {
   // The blacklist setup code is running. If this is still set at startup,
   // it means the last setup crashed.
   BLACKLIST_SETUP_RUNNING,
+  // The blacklist thunk setup code is running. If this is still set at startup,
+  // it means the last setup crashed during thunk setup.
+  BLACKLIST_THUNK_SETUP,
+  // The blacklist code is currently intercepting MapViewOfSection. If this is
+  // still set at startup, it means we crashed during interception.
+  BLACKLIST_INTERCEPTING,
   // Always keep this at the end.
   BLACKLIST_STATE_MAX,
 };
diff --git a/chrome_elf/blacklist/blacklist_interceptions.cc b/chrome_elf/blacklist/blacklist_interceptions.cc
index 6b01438..2163446 100644
--- a/chrome_elf/blacklist/blacklist_interceptions.cc
+++ b/chrome_elf/blacklist/blacklist_interceptions.cc
@@ -224,8 +224,8 @@ SANDBOX_INTERCEPT NTSTATUS WINAPI BlNtMapViewOfSection(
       g_nt_unmap_view_of_section_func(process, *base);
       ret = STATUS_UNSUCCESSFUL;
     }
-
   }
+
   return ret;
 }
 
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 880aa6b..bfcb34f 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -22854,6 +22854,8 @@ other types of suffix sets.
   <int value="0" label="Blacklist enabled"/>
   <int value="1" label="Blacklist ran successfully."/>
   <int value="2" label="Blacklist failed."/>
+  <int value="3" label="Blacklist thunk setup failed."/>
+  <int value="4" label="Blacklist interception failed."/>
 </enum>
 
 <enum name="BluetoothPairingMethod" type="int">