[chrome_elf] Big ELF cleanup. Part 1.

chrome_elf/crash/crash_helper.cc

Index: chrome_elf/crash/crash_helper.cc
diff --git a/chrome_elf/chrome_elf_main.cc b/chrome_elf/crash/crash_helper.cc
similarity index 52%
copy from chrome_elf/chrome_elf_main.cc
copy to chrome_elf/crash/crash_helper.cc
index 1a125f0cdb6c0cbf061e132a1050f18e1a1fe8bb..21dddcb00931f0248b06b7eb2850b73ee4c08012 100644
--- a/chrome_elf/chrome_elf_main.cc
+++ b/chrome_elf/crash/crash_helper.cc
@@ -1,39 +1,34 @@
-// Copyright 2013 The Chromium Authors. All rights reserved.
+// Copyright 2016 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_elf/chrome_elf_main.h"
+#include "chrome_elf/crash/crash_helper.h"
 
+#include <assert.h>
 #include <windows.h>
+
 #include <algorithm>
+#include <string>
+#include <vector>
 
-#include "base/lazy_instance.h"
-#include "base/strings/string16.h"
-#include "base/win/iat_patch_function.h"
-#include "build/build_config.h"
 #include "chrome/app/chrome_crash_reporter_client_win.h"
-#include "chrome/install_static/install_util.h"
-#include "chrome_elf/blacklist/blacklist.h"
-#include "chrome_elf/blacklist/crashpad_helper.h"
-#include "chrome_elf/chrome_elf_constants.h"
+#include "chrome_elf/hook_util/hook_util.h"
 #include "components/crash/content/app/crashpad.h"
 #include "components/crash/core/common/crash_keys.h"
+#include "third_party/crashpad/crashpad/client/crashpad_client.h"
 
 namespace {
 
-base::LazyInstance<std::vector<crash_reporter::Report>>::Leaky g_crash_reports =
-    LAZY_INSTANCE_INITIALIZER;
-
 // Gets the exe name from the full path of the exe.
-base::string16 GetExeName() {
+std::wstring GetExeName() {
   wchar_t file_path[MAX_PATH] = {};
-  if (!::GetModuleFileName(nullptr, file_path, arraysize(file_path))) {
+  if (!::GetModuleFileNameW(nullptr, file_path, MAX_PATH)) {
     assert(false);
-    return base::string16();
+    return std::wstring();
   }
-  base::string16 file_name_string = file_path;
+  std::wstring file_name_string = file_path;

[robertshield, 2016/08/02 02:38:40]

You can save a copy here like this:

std::wstring file_name;
file_name.resize(MAX_PATH);
if (!::GetModuleFileNameW(nullptr, &file_name[0], MAX_PATH)) {
...
}

[penny, 2016/08/02 21:07:43]

Done.

   size_t last_slash_pos = file_name_string.find_last_of(L'\\');
-  if (last_slash_pos != base::string16::npos) {
+  if (last_slash_pos != std::wstring::npos) {
     file_name_string = file_name_string.substr(
         last_slash_pos + 1, file_name_string.length() - last_slash_pos);
   }
@@ -42,12 +37,10 @@ base::string16 GetExeName() {
   return file_name_string;
 }
 
-void InitializeCrashReportingForProcess() {
-  // We want to initialize crash reporting only in chrome.exe
-  if (GetExeName() != L"chrome.exe")
-    return;
-  ChromeCrashReporterClient::InitializeCrashReportingForProcess();
-}
+// Global pointer to a vector of crash reports.
+// This structure will be initialized in InitializeCrashReportingForProcess()
+// and cleaned up in DllDetachCrashReportingCleanup().
+std::vector<crash_reporter::Report>* g_crash_reports = nullptr;
 
 // chrome_elf loads early in the process and initializes Crashpad. That in turn
 // uses the SetUnhandledExceptionFilter API to set a top level exception
@@ -63,39 +56,84 @@ void InitializeCrashReportingForProcess() {
 // TODO(ananta).
 // Check if it is possible to fix EAT patching or use sidestep patching for
 // 32 bit and 64 bit for this purpose.
-base::win::IATPatchFunction g_set_unhandled_exception_filter;
+elf_hook::IATHook g_set_unhandled_exception_filter;
 
+// Hook function, which ignores the request to set an unhandled-exception
+// filter.
 LPTOP_LEVEL_EXCEPTION_FILTER WINAPI
 SetUnhandledExceptionFilterPatch(LPTOP_LEVEL_EXCEPTION_FILTER filter) {
   // Don't set the exception filter. Please see above for comments.
   return nullptr;
 }
 
+}  // namespace
+
+//------------------------------------------------------------------------------
+// Public chrome_elf crash APIs
+//------------------------------------------------------------------------------
+
+namespace elf_crash {
+
+void InitializeCrashReportingForProcess() {
+  // We want to initialize crash reporting only in chrome.exe
+  if (GetExeName() != L"chrome.exe") {
+#ifdef _DEBUG
+    assert(false);
+#endif  // _DEBUG
+    return;
+  }
+

[robertshield, 2016/08/02 02:38:40]

Could you also assert in debug mode that g_crash_reports starts off null here?

[penny, 2016/08/02 21:07:43]

Done.

+  // No global objects with destructors, so using a global pointer.
+  // DllMain on detach will clean this up.
+  g_crash_reports = new std::vector<crash_reporter::Report>;
+
+  ChromeCrashReporterClient::InitializeCrashReportingForProcess();
+}
+
+// NOTE: This function will be called from DllMain during DLL_PROCESS_DETACH
+// (while we have the loader lock), so do not misbehave.

[robertshield, 2016/08/02 02:38:40]

This comment also applies to InitializeCrashReportingForProcess(), should have
it in both places.

[penny, 2016/08/02 21:07:43]

Done.

+void DllDetachCrashReportingCleanup() {
+  if (g_crash_reports != nullptr) {
+    g_crash_reports->clear();
+    delete g_crash_reports;
+  }
+}
+
 // Please refer above to more information about why we intercept the

[robertshield, 2016/08/02 02:38:40]

specify to refer to the comment on g_set_unhandled_exception_filter

[penny, 2016/08/02 21:07:43]

Done.

 // SetUnhandledExceptionFilter API.
 void DisableSetUnhandledExceptionFilter() {
-  DWORD patched = g_set_unhandled_exception_filter.PatchFromModule(
-      GetModuleHandle(nullptr), "kernel32.dll", "SetUnhandledExceptionFilter",
-      SetUnhandledExceptionFilterPatch);
-  CHECK(patched == 0);
+  if (g_set_unhandled_exception_filter.Hook(
+          GetModuleHandle(nullptr), "kernel32.dll",
+          "SetUnhandledExceptionFilter",
+          SetUnhandledExceptionFilterPatch) != NO_ERROR) {
+#ifdef _DEBUG
+    assert(false);
+#endif  //_DEBUG
+  }
 }
 
-}  // namespace
-
-void SignalChromeElf() {
-  blacklist::ResetBeacon();
+int GenerateCrashDump(EXCEPTION_POINTERS* exception_pointers) {
+  crashpad::CrashpadClient::DumpWithoutCrash(
+      *(exception_pointers->ContextRecord));
+  return EXCEPTION_CONTINUE_SEARCH;
 }
 
+}  // namespace elf_crash
+
+//------------------------------------------------------------------------------
+// Exported crash APIs for the rest of the process.
+//------------------------------------------------------------------------------
+
 // This helper is invoked by code in chrome.dll to retrieve the crash reports.
-// See CrashUploadListCrashpad. Note that we do not pass an std::vector here,
+// See CrashUploadListCrashpad. Note that we do not pass a std::vector here,
 // because we do not want to allocate/free in different modules. The returned
 // pointer is read-only.
 extern "C" __declspec(dllexport) void GetCrashReportsImpl(
     const crash_reporter::Report** reports,
     size_t* report_count) {
-  crash_reporter::GetReports(g_crash_reports.Pointer());
-  *reports = g_crash_reports.Pointer()->data();
-  *report_count = g_crash_reports.Pointer()->size();
+  crash_reporter::GetReports(g_crash_reports);
+  *reports = g_crash_reports->data();
+  *report_count = g_crash_reports->size();
 }
 
 // This helper is invoked by debugging code in chrome to register the client
@@ -105,21 +143,3 @@ extern "C" __declspec(dllexport) void SetMetricsClientId(
   if (client_id)
     crash_keys::SetMetricsClientIdFromGUID(client_id);
 }
-
-BOOL APIENTRY DllMain(HMODULE module, DWORD reason, LPVOID reserved) {
-  if (reason == DLL_PROCESS_ATTACH) {
-    InitializeCrashReportingForProcess();
-    // CRT on initialization installs an exception filter which calls
-    // TerminateProcess. We need to hook CRT's attempt to set an exception
-    // handler and ignore it.
-    DisableSetUnhandledExceptionFilter();
-
-    install_static::InitializeProcessType();
-
-    __try {
-      blacklist::Initialize(false);  // Don't force, abort if beacon is present.
-    } __except(GenerateCrashDump(GetExceptionInformation())) {
-    }
-  }
-  return TRUE;
-}