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

Issue 2909623002: Change DumpProcessWithoutCrash to use load-time dynamic linking (Closed)

Created:
3 years, 7 months ago by Will Harris
Modified:
3 years, 5 months ago
Reviewers:
Nico, robertshield, scottmg
CC:
chromium-reviews, sadrul, jam, pennymac+watch_chromium.org, darin-cc_chromium.org, caitkp+watch_chromium.org, kalyank
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Change DumpProcessWithoutCrash to use load-time dynamic linking. Instead of exporting DumpProcessWithoutCrash from chrome_elf directly pass the correct function to call from crashpad, and use load-time dynamic linking. This avoids an explicit call to LoadLibrary early in Chrome startup which can cause issues with certain third party code. See also bug 674541 where this was done for code in install_static. BUG=683848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2909623002 Cr-Commit-Position: refs/heads/master@{#482949} Committed: https://chromium.googlesource.com/chromium/src/+/93846275952fa28f83a4ea318e56c4f494bf7005

Patch Set 1 #

Total comments: 6

Patch Set 2 : rebase #

Total comments: 15

Patch Set 3 : change to direct call #

Total comments: 9

Patch Set 4 : rebase #

Patch Set 5 : code review comments #

Total comments: 2

Patch Set 6 : add comment #

Patch Set 7 : rename func. add detailed comment. #

Patch Set 8 : forgot to update def file #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -30 lines) Patch
M base/debug/dump_without_crashing.h View 1 2 3 4 5 6 1 chunk +8 lines, -2 lines 0 comments Download
M base/debug/dump_without_crashing.cc View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/app/chrome_main.cc View 1 2 3 4 5 6 2 chunks +4 lines, -9 lines 0 comments Download
M chrome_elf/chrome_elf.def View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome_elf/chrome_elf_main.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chrome_elf/crash/crash_helper.h View 1 2 1 chunk +5 lines, -1 line 0 comments Download
M chrome_elf/crash/crash_helper.cc View 1 2 2 chunks +5 lines, -1 line 0 comments Download
M components/crash/content/app/crashpad.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M components/crash/content/app/crashpad.cc View 1 2 3 4 2 chunks +4 lines, -4 lines 0 comments Download
M components/crash/content/app/crashpad_win.cc View 1 2 3 chunks +2 lines, -13 lines 0 comments Download

Messages

Total messages: 47 (20 generated)
Will Harris
very much a WIP first attempt. the aim is to just remove the LoadLibrary but ...
3 years, 7 months ago (2017-05-26 05:54:38 UTC) #3
Will Harris
https://codereview.chromium.org/2909623002/diff/1/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2909623002/diff/1/chrome/app/chrome_main.cc#newcode73 chrome/app/chrome_main.cc:73: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); On 2017/05/26 05:54:38, Will Harris (SYD) wrote: > ...
3 years, 7 months ago (2017-05-26 06:03:35 UTC) #4
scottmg
https://codereview.chromium.org/2909623002/diff/1/components/crash/content/app/crashpad_win.cc File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/1/components/crash/content/app/crashpad_win.cc#newcode162 components/crash/content/app/crashpad_win.cc:162: void DumpProcessWithoutCrash() { It looks like https://cs.chromium.org/chromium/src/chrome/browser/hang_monitor/hang_crash_dump_win.cc?rcl=bfe6ca7a755f69fe8a8bcc6986f1edd5cd0860aa&l=29 will need ...
3 years, 6 months ago (2017-06-13 17:47:55 UTC) #5
Will Harris
https://codereview.chromium.org/2909623002/diff/1/components/crash/content/app/crashpad_win.cc File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/1/components/crash/content/app/crashpad_win.cc#newcode162 components/crash/content/app/crashpad_win.cc:162: void DumpProcessWithoutCrash() { On 2017/06/13 17:47:55, scottmg wrote: > ...
3 years, 6 months ago (2017-06-13 19:03:59 UTC) #6
Will Harris
rebased with crrev.com/2939843002 and I think that's the final user of exported DumpProcessWithoutCrash. PTAL...? https://codereview.chromium.org/2909623002/diff/1/components/crash/content/app/crashpad_win.cc ...
3 years, 6 months ago (2017-06-15 22:54:38 UTC) #7
scottmg
https://codereview.chromium.org/2909623002/diff/20001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2909623002/diff/20001/chrome/app/chrome_main.cc#newcode77 chrome/app/chrome_main.cc:77: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); Why does this have to be a function ...
3 years, 6 months ago (2017-06-15 23:08:47 UTC) #8
Will Harris
okay I admit this is a bit confusing. https://codereview.chromium.org/2909623002/diff/20001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2909623002/diff/20001/chrome/app/chrome_main.cc#newcode77 chrome/app/chrome_main.cc:77: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); ...
3 years, 6 months ago (2017-06-15 23:37:25 UTC) #9
scottmg
Sorry, I'm still lost. But good news, you're still (ooo) so I have time to ...
3 years, 6 months ago (2017-06-16 00:26:24 UTC) #10
Will Harris
I think I grok what you're saying, you're saying just pass the crashpad DumpWithoutCrashing function ...
3 years, 6 months ago (2017-06-16 00:37:49 UTC) #11
Will Harris
Done, sorry it took so long. I maintained the abstraction via elf_crash because that's what ...
3 years, 6 months ago (2017-06-21 14:31:26 UTC) #13
scottmg
lgtm
3 years, 6 months ago (2017-06-21 15:07:58 UTC) #14
Will Harris
Robert, please review chrome_elf changes. Thanks!
3 years, 6 months ago (2017-06-22 17:56:50 UTC) #17
robertshield
https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf.def File chrome_elf/chrome_elf.def (right): https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf.def#newcode15 chrome_elf/chrome_elf.def:15: DumpWithoutCrashing So we still do export this from elf? ...
3 years, 6 months ago (2017-06-22 18:15:41 UTC) #18
Will Harris
comments addressed. PTAL thakis -> can you take a look at base/ and chrome/ Thanks. ...
3 years, 5 months ago (2017-06-26 12:26:20 UTC) #20
Nico
base/, chrome/ lgtm https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_without_crashing.cc File base/debug/dump_without_crashing.cc (right): https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_without_crashing.cc#newcode30 base/debug/dump_without_crashing.cc:30: #if !defined(COMPONENT_BUILD) add comment why this ...
3 years, 5 months ago (2017-06-26 14:36:14 UTC) #22
Will Harris
Thanks for reviews. https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_without_crashing.cc File base/debug/dump_without_crashing.cc (right): https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_without_crashing.cc#newcode30 base/debug/dump_without_crashing.cc:30: #if !defined(COMPONENT_BUILD) On 2017/06/26 14:36:14, Nico ...
3 years, 5 months ago (2017-06-26 16:27:04 UTC) #23
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/2909623002/120001
3 years, 5 months ago (2017-06-26 16:27:19 UTC) #26
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/473971)
3 years, 5 months ago (2017-06-26 16:34:00 UTC) #28
Will Harris
woops forgot you, Robert - PTAL :)
3 years, 5 months ago (2017-06-26 18:03:21 UTC) #29
robertshield
LGTM with a renaming / commenting suggestion. https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_main.h File chrome_elf/chrome_elf_main.h (right): https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_main.h#newcode10 chrome_elf/chrome_elf_main.h:10: extern "C" ...
3 years, 5 months ago (2017-06-27 14:47:17 UTC) #30
Will Harris
all done. I agree this dumpwithoutcrash stuff is a bit confusing so I documented it ...
3 years, 5 months ago (2017-06-27 16:13:27 UTC) #31
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/2909623002/140001
3 years, 5 months ago (2017-06-27 16:19:43 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/477185)
3 years, 5 months ago (2017-06-27 16:51:56 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/2909623002/140001
3 years, 5 months ago (2017-06-28 09:45:54 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/477933)
3 years, 5 months ago (2017-06-28 10:28:05 UTC) #40
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/2909623002/160001
3 years, 5 months ago (2017-06-28 10:33:25 UTC) #43
commit-bot: I haz the power
3 years, 5 months ago (2017-06-28 11:51:23 UTC) #47
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/93846275952fa28f83a4ea318e56...

Powered by Google App Engine
This is Rietveld 408576698