Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(40)

Issue 2475863004: Make Crashpad start asynchronous, and move back to chrome_elf (Closed)

Created:
2 years, 5 months ago by scottmg
Modified:
2 years, 4 months ago
CC:
chromium-reviews, sadrul, jam, pennymac+watch_chromium.org, darin-cc_chromium.org, caitkp+watch_chromium.org, kalyank
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make Crashpad start asynchronous, and move back to chrome_elf Crashpad initialization has been reworked to support an asynchronous mode where StartHandler() only calls CreateThread() and does not synchronize with that thread, making it safe to use in DllMain(). So, we can now move it back into chrome_elf to catch earlier crashes. We block much later now in browser startup, before a renderer (i.e. another client that would connect to the handler) will be started. This should not be strictly necessary, but is slightly more conservative as a first pass. This allows for error reporting, and prevents log spew from each renderer that starts up and tries to connect to a nonexistent handler. Also added is a UMA timer to see how long we're blocking startup for by joining with the background thread. This includes an effective revert of https://chromium.googlesource.com/chromium/src/+/f3a5670dd8d42b045d31625dde4a2561b471138f . BUG=655788, 656800, 565063 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/be0cfa14d77c72864a74d2bcc7910b0b31b7eecb Cr-Commit-Position: refs/heads/master@{#439188}

Patch Set 1 #

Patch Set 2 : . #

Total comments: 6

Patch Set 3 : fixes #

Patch Set 4 : rebase #

Patch Set 5 : rebase again #

Patch Set 6 : more stripping for elf #

Patch Set 7 : rebase part 3 #

Total comments: 6

Patch Set 8 : timeout #

Total comments: 2

Patch Set 9 : comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -17 lines) Patch
M chrome/app/chrome_exe_main_win.cc View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 2 chunks +13 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf.def View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M chrome_elf/chrome_elf_main.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 2 3 4 2 chunks +4 lines, -11 lines 0 comments Download
M components/crash/content/app/crashpad.h View 1 chunk +4 lines, -0 lines 0 comments Download
M components/crash/content/app/crashpad.cc View 1 1 chunk +7 lines, -0 lines 0 comments Download
M components/crash/content/app/crashpad_win.cc View 1 2 3 4 5 6 7 8 2 chunks +19 lines, -3 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 100 (71 generated)
scottmg
mark: general review pennymac/ananta: chrome_elf/fyi
2 years, 5 months ago (2016-11-04 21:38:59 UTC) #4
Mark Mentovai
LGTM otherwise https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_main_win.cc File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_main_win.cc#newcode1 chrome/app/chrome_exe_main_win.cc:1: // Copyright (c) 2011 The Chromium Authors. ...
2 years, 5 months ago (2016-11-04 21:47:56 UTC) #5
scottmg
https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_main_win.cc File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2475863004/diff/20001/chrome/app/chrome_exe_main_win.cc#newcode1 chrome/app/chrome_exe_main_win.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights ...
2 years, 5 months ago (2016-11-04 22:13:42 UTC) #9
scottmg
+thestig: chrome/ +asvitkine: histograms.xml
2 years, 5 months ago (2016-11-04 22:14:01 UTC) #10
Alexei Svitkine (slow)
lgtm
2 years, 5 months ago (2016-11-04 22:14:50 UTC) #11
Mark Mentovai
Works for me. LGTM.
2 years, 5 months ago (2016-11-04 22:17:35 UTC) #12
Lei Zhang
lgtm
2 years, 5 months ago (2016-11-04 22:23:53 UTC) #13
scottmg
After shaving a few yaks deep, this should finally be able to land, so will ...
2 years, 4 months ago (2016-12-05 19:17:31 UTC) #44
Mark Mentovai
LGTM
2 years, 4 months ago (2016-12-05 20:03:01 UTC) #45
scottmg
On 2016/12/05 19:17:31, scottmg wrote: > After shaving a few yaks deep, this should finally ...
2 years, 4 months ago (2016-12-06 18:31:32 UTC) #57
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/2475863004/220001
2 years, 4 months ago (2016-12-08 23:36:11 UTC) #65
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/177529)
2 years, 4 months ago (2016-12-08 23:44:52 UTC) #67
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/2475863004/220001
2 years, 4 months ago (2016-12-08 23:50:21 UTC) #69
commit-bot: I haz the power
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/135641)
2 years, 4 months ago (2016-12-08 23:54:13 UTC) #71
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/2475863004/220001
2 years, 4 months ago (2016-12-09 00:18:49 UTC) #73
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/322128)
2 years, 4 months ago (2016-12-09 01:05:46 UTC) #75
scottmg
+robertshield for chrome_elf/OWNERS
2 years, 4 months ago (2016-12-09 01:31:04 UTC) #77
robertshield
https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc#newcode1419 chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf, "BlockUntilHandlerStartedImpl")); This isn't in elf's .def file. Should ...
2 years, 4 months ago (2016-12-09 01:56:44 UTC) #78
scottmg
https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc#newcode1419 chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf, "BlockUntilHandlerStartedImpl")); On 2016/12/09 01:56:44, robertshield_slow_reviews wrote: > This ...
2 years, 4 months ago (2016-12-09 20:23:15 UTC) #79
robertshield
https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc#newcode1419 chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf, "BlockUntilHandlerStartedImpl")); On 2016/12/09 20:23:14, scottmg wrote: > On ...
2 years, 4 months ago (2016-12-09 21:11:47 UTC) #80
scottmg
https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc#newcode1419 chrome/browser/chrome_browser_main.cc:1419: GetProcAddress(chrome_elf, "BlockUntilHandlerStartedImpl")); On 2016/12/09 21:11:47, robertshield_slow_reviews wrote: > On ...
2 years, 4 months ago (2016-12-13 18:46:59 UTC) #83
scottmg
On 2016/12/13 18:46:59, scottmg wrote: > https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/2475863004/diff/220001/chrome/browser/chrome_browser_main.cc#newcode1419 > ...
2 years, 4 months ago (2016-12-14 17:53:33 UTC) #86
robertshield
LGTM, w/ one comment update suggestion https://codereview.chromium.org/2475863004/diff/240001/components/crash/content/app/crashpad_win.cc File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2475863004/diff/240001/components/crash/content/app/crashpad_win.cc#newcode126 components/crash/content/app/crashpad_win.cc:126: // converted to ...
2 years, 4 months ago (2016-12-15 20:06:30 UTC) #87
scottmg
https://codereview.chromium.org/2475863004/diff/240001/components/crash/content/app/crashpad_win.cc File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2475863004/diff/240001/components/crash/content/app/crashpad_win.cc#newcode126 components/crash/content/app/crashpad_win.cc:126: // converted to non-fatal, calls to BlockUntilHandlerStarted() will have ...
2 years, 4 months ago (2016-12-16 17:16:51 UTC) #88
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/2475863004/260001
2 years, 4 months ago (2016-12-16 17:18:27 UTC) #91
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/87732)
2 years, 4 months ago (2016-12-16 19:55:02 UTC) #93
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/2475863004/260001
2 years, 4 months ago (2016-12-16 20:02:20 UTC) #95
commit-bot: I haz the power
Committed patchset #9 (id:260001)
2 years, 4 months ago (2016-12-16 21:01:39 UTC) #98
commit-bot: I haz the power
2 years, 4 months ago (2016-12-16 21:06:39 UTC) #100
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/be0cfa14d77c72864a74d2bcc7910b0b31b7eecb
Cr-Commit-Position: refs/heads/master@{#439188}

Powered by Google App Engine
This is Rietveld 408576698