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

Issue 2088133002: Switch chrome_elf exception handling from breakpad to crashpad. (Closed)

Created:
4 years, 6 months ago by ananta
Modified:
4 years, 5 months ago
Reviewers:
robertshield, sky, scottmg
CC:
chromium-reviews, caitkp+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Switch chrome_elf exception handling from breakpad to crashpad. Changes in this patch are as below: 1. chrome_elf: Removing breakpad and corresponding exception initialization using breakpad. Registering crashpad as the exception handling mechanism in chrome_elf. We continue to handle exceptions in chrome_elf DllMain and in the blacklist interception code as before, i.e. we use crashpad to grab the dump and pass the exception to the next handler in the chain. I also added support in chrome_elf to not initialize crashpad if it is being loaded in some tests. For e.g. the ELFImportsTest. This is via the exe name check. 2. chrome.exe:- Removing crashpad exception registration. Also we need to retrieve current exception reports information from chrome_elf instead of chrome. 3. Intercept the SetUnhandledExceptionFilter API in chrome_elf via IAT patching on the exe and disallow the call. This is to prevent CRT from overwriting crashpads exception filter. I added a TODO to see if we can use EAT patching or possibly sidestep if we can support 64 bit with it. 4. Added a function RegisterCrashKeysForDebugging to the ChromeCrashReporterClient class on Windows to register crash keys. This is currently invoked from chrome_elf for debugging purposes. 5. Changed the browser lib target to depend on chrome_elf:blacklist instead of chrome_elf. This prevents chrome_elf from being implicitly loaded in processes other than chrome.exe BUG=604923, crashpad:106, 568664 TBR=robertshield Committed: https://crrev.com/34831427124fef3015106cfe6e94d35f3aa9e4f6 Cr-Commit-Position: refs/heads/master@{#403048}

Patch Set 1 #

Patch Set 2 : Remove include #

Total comments: 4

Patch Set 3 : Update commments and attempt to fix dependency failure #

Total comments: 9

Patch Set 4 : Address review comments #

Total comments: 6

Patch Set 5 : git cl format #

Patch Set 6 : Added intercept for the SetUnhandledExceptionFilter API in chrome_elf. This is needed to avoid CRT … #

Patch Set 7 : Use IAT patching for SetUnhandledExceptionFilter #

Patch Set 8 : Update DEPS #

Patch Set 9 : Revert changes to chrome_elf_constants #

Patch Set 10 : Register crash keys only once in the process. #

Total comments: 13

Patch Set 11 : Address review comments #

Patch Set 12 : Rename function to RegisterCrashKeysAndSetMetricsId #

Total comments: 12

Patch Set 13 : Address review comments #

Patch Set 14 : Remove the key registration code from chrome_elf for the code path taken by the child process loggi… #

Total comments: 3

Patch Set 15 : Change more places in chrome to use chrome_elf as the crashpad entry point export conduit. Change c… #

Patch Set 16 : Fix build error #

Total comments: 2

Patch Set 17 : Address review comments #

Patch Set 18 : Fix build error #

Patch Set 19 : Ensure that RegisterCrashKeys when chrome_elf is loaded only happens if we are not in component bui… #

Patch Set 20 : Fix build error #

Patch Set 21 : Fix build error #

Unified diffs Side-by-side diffs Delta from patch set Stats (+224 lines, -343 lines) Patch
M chrome/app/chrome_crash_reporter_client_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/app/chrome_exe_main_win.cc View 1 2 3 4 5 chunks +0 lines, -24 lines 0 comments Download
M chrome/app/chrome_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chrome_browser_main_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/hang_monitor/hang_crash_dump_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/child_process_logging_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 4 chunks +37 lines, -9 lines 0 comments Download
M chrome/common/chrome_constants.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +1 line, -8 lines 0 comments Download
M chrome/common/chrome_constants.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +1 line, -7 lines 0 comments Download
M chrome/install_static/install_util.cc View 1 2 3 4 5 1 chunk +0 lines, -7 lines 0 comments Download
M chrome_elf/BUILD.gn View 1 2 3 4 5 5 chunks +17 lines, -17 lines 0 comments Download
M chrome_elf/DEPS View 1 2 3 4 5 6 7 1 chunk +4 lines, -1 line 0 comments Download
M chrome_elf/blacklist.gypi View 1 2 3 4 5 2 chunks +19 lines, -1 line 0 comments Download
M chrome_elf/blacklist/blacklist_interceptions.cc View 1 2 3 4 6 1 chunk +1 line, -1 line 0 comments Download
A chrome_elf/blacklist/crashpad_helper.h View 1 2 3 4 1 chunk +15 lines, -0 lines 0 comments Download
A chrome_elf/blacklist/crashpad_helper.cc View 1 2 3 4 5 6 7 8 1 chunk +13 lines, -0 lines 0 comments Download
D chrome_elf/breakpad.h View 1 chunk +0 lines, -34 lines 0 comments Download
D chrome_elf/breakpad.cc View 1 chunk +0 lines, -196 lines 0 comments Download
M chrome_elf/chrome_elf.gyp View 1 2 3 4 5 2 chunks +0 lines, -19 lines 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +98 lines, -3 lines 0 comments Download
M components/crash/content/app/crashpad.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -7 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 46 (15 generated)
ananta
4 years, 6 months ago (2016-06-22 03:32:49 UTC) #2
robertshield
Lg overall, couple of comment requests and there's a dependency failure on one of the ...
4 years, 6 months ago (2016-06-22 04:09:32 UTC) #3
ananta
https://codereview.chromium.org/2088133002/diff/20001/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2088133002/diff/20001/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc#newcode25 chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:25: HMODULE exe_module = GetModuleHandle(L"chrome_elf.dll"); On 2016/06/22 04:09:32, robertshield wrote: ...
4 years, 6 months ago (2016-06-22 19:30:33 UTC) #4
scottmg
Exciting! Does it actually work? :) https://codereview.chromium.org/2088133002/diff/40001/chrome_elf/BUILD.gn File chrome_elf/BUILD.gn (right): https://codereview.chromium.org/2088133002/diff/40001/chrome_elf/BUILD.gn#newcode119 chrome_elf/BUILD.gn:119: "crashpad_helper.cc", I don't ...
4 years, 6 months ago (2016-06-22 19:38:07 UTC) #5
robertshield
https://codereview.chromium.org/2088133002/diff/40001/chrome_elf/crashpad_helper.cc File chrome_elf/crashpad_helper.cc (right): https://codereview.chromium.org/2088133002/diff/40001/chrome_elf/crashpad_helper.cc#newcode9 chrome_elf/crashpad_helper.cc:9: crashpad::CrashpadClient::DumpWithoutCrash( On 2016/06/22 19:38:07, scottmg wrote: > This is ...
4 years, 6 months ago (2016-06-22 19:47:45 UTC) #6
ananta
https://codereview.chromium.org/2088133002/diff/40001/chrome_elf/BUILD.gn File chrome_elf/BUILD.gn (right): https://codereview.chromium.org/2088133002/diff/40001/chrome_elf/BUILD.gn#newcode119 chrome_elf/BUILD.gn:119: "crashpad_helper.cc", On 2016/06/22 19:38:07, scottmg wrote: > I don't ...
4 years, 6 months ago (2016-06-22 21:08:20 UTC) #7
scottmg
lgtm https://codereview.chromium.org/2088133002/diff/60001/chrome/app/chrome_exe_main_win.cc File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2088133002/diff/60001/chrome/app/chrome_exe_main_win.cc#newcode26 chrome/app/chrome_exe_main_win.cc:26: #include "chrome/app/chrome_crash_reporter_client_win.h" Remove some includes here? https://codereview.chromium.org/2088133002/diff/60001/chrome/app/chrome_exe_main_win.cc#newcode34 chrome/app/chrome_exe_main_win.cc:34: ...
4 years, 6 months ago (2016-06-22 21:18:24 UTC) #8
ananta
https://codereview.chromium.org/2088133002/diff/60001/chrome/app/chrome_exe_main_win.cc File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2088133002/diff/60001/chrome/app/chrome_exe_main_win.cc#newcode26 chrome/app/chrome_exe_main_win.cc:26: #include "chrome/app/chrome_crash_reporter_client_win.h" On 2016/06/22 21:18:23, scottmg wrote: > Remove ...
4 years, 6 months ago (2016-06-22 21:25:08 UTC) #9
ananta
+sky for chrome owners stamp
4 years, 6 months ago (2016-06-22 21:29:30 UTC) #11
sky
LGTM
4 years, 6 months ago (2016-06-22 23:04:59 UTC) #12
ananta
Please take a peek at the updated patch guys. crashpad is now functional in chrome ...
4 years, 6 months ago (2016-06-25 02:02:14 UTC) #17
scottmg
(Still trying to make progress on the need to register keys.) https://codereview.chromium.org/2088133002/diff/180001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): ...
4 years, 5 months ago (2016-06-27 19:37:46 UTC) #18
ananta
https://codereview.chromium.org/2088133002/diff/180001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2088133002/diff/180001/chrome/app/chrome_crash_reporter_client_win.cc#newcode72 chrome/app/chrome_crash_reporter_client_win.cc:72: // Crash keys should be registered only once for ...
4 years, 5 months ago (2016-06-27 20:00:27 UTC) #20
scottmg
https://codereview.chromium.org/2088133002/diff/180001/chrome/common/child_process_logging_win.cc File chrome/common/child_process_logging_win.cc (right): https://codereview.chromium.org/2088133002/diff/180001/chrome/common/child_process_logging_win.cc#newcode81 chrome/common/child_process_logging_win.cc:81: crash_keys::RegisterChromeCrashKeys(); I'm confused now, when is the non-elf path ...
4 years, 5 months ago (2016-06-27 20:09:24 UTC) #21
ananta
https://codereview.chromium.org/2088133002/diff/180001/chrome/common/child_process_logging_win.cc File chrome/common/child_process_logging_win.cc (right): https://codereview.chromium.org/2088133002/diff/180001/chrome/common/child_process_logging_win.cc#newcode82 chrome/common/child_process_logging_win.cc:82: if (client_info) On 2016/06/27 20:09:24, scottmg wrote: > On ...
4 years, 5 months ago (2016-06-27 20:36:06 UTC) #22
scottmg
https://codereview.chromium.org/2088133002/diff/220001/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2088133002/diff/220001/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc#newcode26 chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:26: HMODULE elf_module = GetModuleHandle(L"chrome_elf.dll"); Since there's a bunch of ...
4 years, 5 months ago (2016-06-27 20:42:49 UTC) #23
ananta
https://codereview.chromium.org/2088133002/diff/220001/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc File chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc (right): https://codereview.chromium.org/2088133002/diff/220001/chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc#newcode26 chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc:26: HMODULE elf_module = GetModuleHandle(L"chrome_elf.dll"); On 2016/06/27 20:42:48, scottmg wrote: ...
4 years, 5 months ago (2016-06-27 22:52:23 UTC) #24
scottmg
lgtm https://codereview.chromium.org/2088133002/diff/260001/chrome/common/chrome_constants.cc File chrome/common/chrome_constants.cc (left): https://codereview.chromium.org/2088133002/diff/260001/chrome/common/chrome_constants.cc#oldcode202 chrome/common/chrome_constants.cc:202: #if defined(OS_WIN) Thanks for cleaning this up, but ...
4 years, 5 months ago (2016-06-27 23:35:55 UTC) #25
ananta
+sky. Please peek at the chrome changes one more time
4 years, 5 months ago (2016-06-27 23:48:42 UTC) #26
sky
LGTM assuming the following is ok. I do have one question though. Lets say there ...
4 years, 5 months ago (2016-06-28 02:40:45 UTC) #27
ananta
Currently it is all manual testing, and some individual unittests for portions of the code ...
4 years, 5 months ago (2016-06-28 03:15:44 UTC) #28
sky
Ok, thanks for the clarification. On Mon, Jun 27, 2016 at 8:15 PM, <ananta@chromium.org> wrote: ...
4 years, 5 months ago (2016-06-28 16:42:05 UTC) #29
robertshield
lg, one comment about where IAT patching is called from. https://codereview.chromium.org/2088133002/diff/300001/chrome_elf/chrome_elf_main.cc File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2088133002/diff/300001/chrome_elf/chrome_elf_main.cc#newcode117 ...
4 years, 5 months ago (2016-06-29 02:42:55 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2088133002/400001
4 years, 5 months ago (2016-06-30 00:47:06 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/210082)
4 years, 5 months ago (2016-06-30 00:56:56 UTC) #35
ananta
https://codereview.chromium.org/2088133002/diff/300001/chrome_elf/chrome_elf_main.cc File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2088133002/diff/300001/chrome_elf/chrome_elf_main.cc#newcode117 chrome_elf/chrome_elf_main.cc:117: DisableSetUnhandledExceptionFilter(); On 2016/06/29 02:42:54, robertshield wrote: > This will ...
4 years, 5 months ago (2016-06-30 01:00:36 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2088133002/400001
4 years, 5 months ago (2016-06-30 01:02:29 UTC) #40
commit-bot: I haz the power
Committed patchset #21 (id:400001)
4 years, 5 months ago (2016-06-30 01:08:04 UTC) #42
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-06-30 01:08:07 UTC) #43
commit-bot: I haz the power
Patchset 21 (id:??) landed as https://crrev.com/34831427124fef3015106cfe6e94d35f3aa9e4f6 Cr-Commit-Position: refs/heads/master@{#403048}
4 years, 5 months ago (2016-06-30 01:09:54 UTC) #45
Ken Russell (switch to Gerrit)
4 years, 5 months ago (2016-06-30 01:49:23 UTC) #46
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/2108413002/ by kbr@chromium.org.

The reason for reverting is: Broke build:
https://build.chromium.org/p/chromium.win/builders/Win8%20GYP%20%28dbg%29/bui...

Seems likely the component build is broken by this change.
.

Powered by Google App Engine
This is Rietveld 408576698