|
|
Created:
3 years, 7 months ago by Will Harris Modified:
3 years, 5 months ago 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. |
DescriptionChange 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 #
Messages
Total messages: 47 (20 generated)
Description was changed from ========== Do not export DumpProcessWithoutCrash from chrome_elf. Instead of exporting DumpProcessWithoutCrash from chrome_elf directly pass the correct function to call from crashpad. BUG=683848 ========== to ========== Do not export DumpProcessWithoutCrash from chrome_elf. Instead of exporting DumpProcessWithoutCrash from chrome_elf directly pass the correct function to call from crashpad. BUG=683848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
wfh@chromium.org changed reviewers: + scottmg@chromium.org
very much a WIP first attempt. the aim is to just remove the LoadLibrary but I'm not 100% sure this CL actually works... I'm uploading so it's safe while I'm away on vacation for the next few weeks. 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#n... chrome/app/chrome_main.cc:73: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); I'm not sure it's okay to call this function in chrome_elf.dll from chrome.dll - it seems to work though, but needs investigation. https://codereview.chromium.org/2909623002/diff/1/components/crash/content/ap... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/1/components/crash/content/ap... components/crash/content/app/crashpad_win.cc:162: void DumpProcessWithoutCrash() { not sure why we have two copies of this function... but they do the same thing so ¯\_(ツ)_/¯
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#n... chrome/app/chrome_main.cc:73: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); On 2017/05/26 05:54:38, Will Harris (SYD) wrote: > I'm not sure it's okay to call this function in chrome_elf.dll from chrome.dll - > it seems to work though, but needs investigation. so fwiw current chrome.dll already calls these functions from chrome_elf.dll so I presume adding another one does no harm. GetBlacklistIndex GetInstallDetailsPayload IsBlacklistInitialized SuccessfullyBlocked
https://codereview.chromium.org/2909623002/diff/1/components/crash/content/ap... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/1/components/crash/content/ap... 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_d... will need to be changed too.
https://codereview.chromium.org/2909623002/diff/1/components/crash/content/ap... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/1/components/crash/content/ap... components/crash/content/app/crashpad_win.cc:162: void DumpProcessWithoutCrash() { On 2017/06/13 17:47:55, scottmg wrote: > It looks like > https://cs.chromium.org/chromium/src/chrome/browser/hang_monitor/hang_crash_d... > will need to be changed too. sigh - does hang monitor have any tests...? :S
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/ap... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/1/components/crash/content/ap... components/crash/content/app/crashpad_win.cc:162: void DumpProcessWithoutCrash() { On 2017/06/13 17:47:55, scottmg wrote: > It looks like > https://cs.chromium.org/chromium/src/chrome/browser/hang_monitor/hang_crash_d... > will need to be changed too. Done, the call was removed in another CL.
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.... chrome/app/chrome_main.cc:77: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); Why does this have to be a function that returns a function, can't it just be a function that we reference instead? https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.cc:33: return elf_crash::GetDumpWithoutCrashingFunction(); And chrome_main is calling this one right? https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/crash/crash_... File chrome_elf/crash/crash_helper.h (right): https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/crash/crash_... chrome_elf/crash/crash_helper.h:32: void (*GetDumpWithoutCrashingFunction())(); So what's this one for? Sorry, I'm totally confused with all these exactly-the-same-named functions. https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... components/crash/content/app/crashpad_win.cc:163: CRASHPAD_SIMULATE_CRASH(); indent. ... In fact, we don't need this function any more right because we're not GetProcAddress()ing it any more? Can you just inline CRASHPAD_SIMULATE_CRASH() into the two functions below instead?
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.... chrome/app/chrome_main.cc:77: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); On 2017/06/15 23:08:47, scottmg wrote: > Why does this have to be a function that returns a function, can't it just be a > function that we reference instead? base can't call out of base, which is why we pass the function into base using SetDumpWithoutCrashingFunction. we have to set the crash function in the right copy of base, there's one linked with each executable. This sets the crash function for chrome.exe which is why it's in chromemain. Hope that makes sense. https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.cc:33: return elf_crash::GetDumpWithoutCrashingFunction(); On 2017/06/15 23:08:47, scottmg wrote: > And chrome_main is calling this one right? yes, in chrome_elf via dynamic linkage. https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/crash/crash_... File chrome_elf/crash/crash_helper.h (right): https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/crash/crash_... chrome_elf/crash/crash_helper.h:32: void (*GetDumpWithoutCrashingFunction())(); On 2017/06/15 23:08:47, scottmg wrote: > So what's this one for? Sorry, I'm totally confused with all these > exactly-the-same-named functions. this is the exposed interface from chrome_elf's crash helper... see line 10 "keep all crash-related APIs here". https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... components/crash/content/app/crashpad_win.cc:163: CRASHPAD_SIMULATE_CRASH(); On 2017/06/15 23:08:47, scottmg wrote: > indent. ... In fact, we don't need this function any more right because we're > not GetProcAddress()ing it any more? > > Can you just inline CRASHPAD_SIMULATE_CRASH() into the two functions below > instead? CRASHPAD_SIMULATE_CRASH calls code linked with chrome_elf.dll so can't be called outside chrome_elf module, unless I'm confused.
Sorry, I'm still lost. But good news, you're still (ooo) so I have time to try to figure out what's going on. 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.... chrome/app/chrome_main.cc:77: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); On 2017/06/15 23:37:24, Will Harris (ooo) wrote: > On 2017/06/15 23:08:47, scottmg wrote: > > Why does this have to be a function that returns a function, can't it just be > a > > function that we reference instead? > > base can't call out of base, which is why we pass the function into base using > SetDumpWithoutCrashingFunction. > > we have to set the crash function in the right copy of base, there's one linked > with each executable. This sets the crash function for chrome.exe which is why > it's in chromemain. > > Hope that makes sense. But couldn't it just be base::debug::SetDumpWithoutCrashingFunction(&DumpWithoutCrashingFunction); ? https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/crash/crash_... File chrome_elf/crash/crash_helper.h (right): https://codereview.chromium.org/2909623002/diff/20001/chrome_elf/crash/crash_... chrome_elf/crash/crash_helper.h:32: void (*GetDumpWithoutCrashingFunction())(); On 2017/06/15 23:37:24, Will Harris (ooo) wrote: > On 2017/06/15 23:08:47, scottmg wrote: > > So what's this one for? Sorry, I'm totally confused with all these > > exactly-the-same-named functions. > > this is the exposed interface from chrome_elf's crash helper... see line 10 > "keep all crash-related APIs here". OK, I don't see the point of this, so you should get Penny or Robert to confirm. https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... components/crash/content/app/crashpad.cc:198: return DumpWithoutCrashing; And, then instead of having this return a function, I don't see why it can't just CRASHPAD_SIMULATE_CRASH() right here, if it's the function that's pointed to by base. (Or if we're allowed, then right in chrome_elf/chrome_elf_main.cc) https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... components/crash/content/app/crashpad_win.cc:163: CRASHPAD_SIMULATE_CRASH(); On 2017/06/15 23:37:24, Will Harris (ooo) wrote: > On 2017/06/15 23:08:47, scottmg wrote: > > indent. ... In fact, we don't need this function any more right because we're > > not GetProcAddress()ing it any more? > > > > Can you just inline CRASHPAD_SIMULATE_CRASH() into the two functions below > > instead? > > CRASHPAD_SIMULATE_CRASH calls code linked with chrome_elf.dll so can't be called > outside chrome_elf module, unless I'm confused. I'm not sure what you mean. We're calling it right here? I just mean this local namespace{} DumpProcessWithoutCrash() function isn't necessary because it's only used in two locations, DumpProcessForHungInputThread() and DumpProcessForHungInputNoCrashKeysThread() below. So instead of having a separate function, it can just be put right there.
I think I grok what you're saying, you're saying just pass the crashpad DumpWithoutCrashing function (implemented in crashpad_win.cc) straight into base::debug::SetDumpWithoutCrashingFunction I think that's fine but it breaks all the abstraction layers... if that's okay with penny/robert to stick my sticky hand directly into that function then :shrug:, but I was trying to maintain the abstractions. I can certainly at least see if your suggestion works. We have no adequate tests for this, but I just need to make sure that all copies of base (in chrome_child.dll, chrome.dll, chrome_elf.dll, chrome.exe) end up calling the same function in crashpad_win.cc - I tested that manually by just breakpointing on it before... 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.... chrome/app/chrome_main.cc:77: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); On 2017/06/16 00:26:24, scottmg wrote: > On 2017/06/15 23:37:24, Will Harris (ooo) wrote: > > On 2017/06/15 23:08:47, scottmg wrote: > > > Why does this have to be a function that returns a function, can't it just > be > > a > > > function that we reference instead? > > > > base can't call out of base, which is why we pass the function into base using > > SetDumpWithoutCrashingFunction. > > > > we have to set the crash function in the right copy of base, there's one > linked > > with each executable. This sets the crash function for chrome.exe which is why > > it's in chromemain. > > > > Hope that makes sense. > > But couldn't it just be > > base::debug::SetDumpWithoutCrashingFunction(&DumpWithoutCrashingFunction); > > ? maybe - but &DumpWithoutCrashingFunction would be in chrome_elf wouldn't it, assuming the linker will still like this?
Patchset #3 (id:40001) has been deleted
Done, sorry it took so long. I maintained the abstraction via elf_crash because that's what other functions do e.g. look at SignalInitializeCrashReporting in chrome_elf_main.cc - the main exports are always from chrome_elf_main.cc. PTAL 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.... chrome/app/chrome_main.cc:77: base::debug::SetDumpWithoutCrashingFunction(GetDumpWithoutCrashingFunction()); this is done, and verified that it calls down to the chrome_elf version for both main exe and dll. https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2909623002/diff/20001/components/crash/conten... components/crash/content/app/crashpad_win.cc:163: CRASHPAD_SIMULATE_CRASH(); On 2017/06/16 00:26:24, scottmg wrote: > On 2017/06/15 23:37:24, Will Harris (ooo) wrote: > > On 2017/06/15 23:08:47, scottmg wrote: > > > indent. ... In fact, we don't need this function any more right because > we're > > > not GetProcAddress()ing it any more? > > > > > > Can you just inline CRASHPAD_SIMULATE_CRASH() into the two functions below > > > instead? > > > > CRASHPAD_SIMULATE_CRASH calls code linked with chrome_elf.dll so can't be > called > > outside chrome_elf module, unless I'm confused. > > I'm not sure what you mean. We're calling it right here? > > I just mean this local namespace{} DumpProcessWithoutCrash() function isn't > necessary because it's only used in two locations, > DumpProcessForHungInputThread() and DumpProcessForHungInputNoCrashKeysThread() > below. So instead of having a separate function, it can just be put right there. only one function exists now.
lgtm
Description was changed from ========== Do not export DumpProcessWithoutCrash from chrome_elf. Instead of exporting DumpProcessWithoutCrash from chrome_elf directly pass the correct function to call from crashpad. BUG=683848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Do not export DumpProcessWithoutCrash from chrome_elf. Instead of exporting DumpProcessWithoutCrash from chrome_elf directly pass the correct function to call from crashpad. BUG=683848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
wfh@chromium.org changed reviewers: + robertshield@chromium.org
Robert, please review chrome_elf changes. Thanks!
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.d... chrome_elf/chrome_elf.def:15: DumpWithoutCrashing So we still do export this from elf? The CL description made me think we would not. https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.h (right): https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.h:10: extern "C" void DumpWithoutCrashing(); When included in chrome_main.cc, is this extern resolved to the implementation from components/crash/content/app/crashpad_win.cc? If so, should chrome_main.cc #include components/crash/content/app/crashpad.h instead? https://codereview.chromium.org/2909623002/diff/60001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2909623002/diff/60001/components/crash/conten... components/crash/content/app/crashpad.cc:194: CRASHPAD_SIMULATE_CRASH(); nit: could you move this down to line 308 to keep the ordering consistent with the header?
wfh@chromium.org changed reviewers: + thakis@chromium.org
comments addressed. PTAL thakis -> can you take a look at base/ and chrome/ Thanks. https://codereview.chromium.org/2909623002/diff/60001/base/debug/dump_without... File base/debug/dump_without_crashing.cc (right): https://codereview.chromium.org/2909623002/diff/60001/base/debug/dump_without... base/debug/dump_without_crashing.cc:30: DCHECK(!dump_without_crashing_function_); this actually broke in component builds because they all share the same base.dll - so I added a preprocessor exception for component builds. Surprised this didn't hit on any of the bots. 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.d... chrome_elf/chrome_elf.def:15: DumpWithoutCrashing On 2017/06/22 18:15:40, robertshield wrote: > So we still do export this from elf? The CL description made me think we would > not. this is still needed as it's not explicitly __declspeced in the .h file. https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.h (right): https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.h:10: extern "C" void DumpWithoutCrashing(); On 2017/06/22 18:15:40, robertshield wrote: > When included in chrome_main.cc, is this extern resolved to the implementation > from components/crash/content/app/crashpad_win.cc? If so, should chrome_main.cc > #include components/crash/content/app/crashpad.h instead? no, because that's namespaced crash_reporter::DumpWithoutCrashing. I re-verified in a debug build that the calls work fine when calling dumpwithoutcrashing - they go from base -> chrome_elf!DumpWithoutCrashing -> chrome_elf!elf_crash::DumpWithoutCrashing -> chrome_elf!crash_reporter::DumpWithoutCrashing -> chrome_elf!crashpad::CrashpadClient::DumpWithoutCrash as expected. This works from both chrome.exe and chrome.dll/chrome_child.dll See https://pastebin.com/raw/sGaDsU2Q https://codereview.chromium.org/2909623002/diff/60001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2909623002/diff/60001/components/crash/conten... components/crash/content/app/crashpad.cc:194: CRASHPAD_SIMULATE_CRASH(); On 2017/06/22 18:15:41, robertshield wrote: > nit: could you move this down to line 308 to keep the ordering consistent with > the header? Done.
Description was changed from ========== Do not export DumpProcessWithoutCrash from chrome_elf. Instead of exporting DumpProcessWithoutCrash from chrome_elf directly pass the correct function to call from crashpad. BUG=683848 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== 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 ==========
base/, chrome/ lgtm https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_withou... File base/debug/dump_without_crashing.cc (right): https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_withou... base/debug/dump_without_crashing.cc:30: #if !defined(COMPONENT_BUILD) add comment why this only holds in static builds
Thanks for reviews. https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_withou... File base/debug/dump_without_crashing.cc (right): https://codereview.chromium.org/2909623002/diff/100001/base/debug/dump_withou... base/debug/dump_without_crashing.cc:30: #if !defined(COMPONENT_BUILD) On 2017/06/26 14:36:14, Nico (vacation Jun 30-Jul 11) wrote: > add comment why this only holds in static builds Done.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2909623002/#ps120001 (title: "add comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
woops forgot you, Robert - PTAL :)
LGTM with a renaming / commenting suggestion. https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.h (right): https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.h:10: extern "C" void DumpWithoutCrashing(); On 2017/06/26 12:26:20, Will Harris (UTC TZ) wrote: > On 2017/06/22 18:15:40, robertshield wrote: > > When included in chrome_main.cc, is this extern resolved to the implementation > > from components/crash/content/app/crashpad_win.cc? If so, should > chrome_main.cc > > #include components/crash/content/app/crashpad.h instead? > > no, because that's namespaced crash_reporter::DumpWithoutCrashing. > > I re-verified in a debug build that the calls work fine when calling > dumpwithoutcrashing - they go from > > base -> chrome_elf!DumpWithoutCrashing -> > chrome_elf!elf_crash::DumpWithoutCrashing -> > chrome_elf!crash_reporter::DumpWithoutCrashing -> > chrome_elf!crashpad::CrashpadClient::DumpWithoutCrash > > as expected. This works from both chrome.exe and chrome.dll/chrome_child.dll > > See https://pastebin.com/raw/sGaDsU2Q Hrmm. Ok, I see, I didn't notice the crash_reporter namespace when first reading. That's a little confusing and I'd suggest either renaming one of the DumpWithoutCrashing functions or adding comments in chrome_main.cc at the base::debug::SetDumpWithoutCrashingFunction(&DumpWithoutCrashing); line indicating that this is taking the function from chrome_elf since future hapless code searchers like myself may get confused by this.
all done. I agree this dumpwithoutcrash stuff is a bit confusing so I documented it as much as possible in the .h file. Will commit shortly. Thanks for reviews. https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.h (right): https://codereview.chromium.org/2909623002/diff/60001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.h:10: extern "C" void DumpWithoutCrashing(); On 2017/06/27 14:47:17, robertshield wrote: > On 2017/06/26 12:26:20, Will Harris (UTC TZ) wrote: > > On 2017/06/22 18:15:40, robertshield wrote: > > > When included in chrome_main.cc, is this extern resolved to the > implementation > > > from components/crash/content/app/crashpad_win.cc? If so, should > > chrome_main.cc > > > #include components/crash/content/app/crashpad.h instead? > > > > no, because that's namespaced crash_reporter::DumpWithoutCrashing. > > > > I re-verified in a debug build that the calls work fine when calling > > dumpwithoutcrashing - they go from > > > > base -> chrome_elf!DumpWithoutCrashing -> > > chrome_elf!elf_crash::DumpWithoutCrashing -> > > chrome_elf!crash_reporter::DumpWithoutCrashing -> > > chrome_elf!crashpad::CrashpadClient::DumpWithoutCrash > > > > as expected. This works from both chrome.exe and chrome.dll/chrome_child.dll > > > > See https://pastebin.com/raw/sGaDsU2Q > > Hrmm. Ok, I see, I didn't notice the crash_reporter namespace when first > reading. > > That's a little confusing and I'd suggest either renaming one of the > DumpWithoutCrashing functions or adding comments in chrome_main.cc at the > > base::debug::SetDumpWithoutCrashingFunction(&DumpWithoutCrashing); > > line indicating that this is taking the function from chrome_elf since future > hapless code searchers like myself may get confused by this. Done.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, robertshield@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2909623002/#ps140001 (title: "rename func. add detailed comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, robertshield@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2909623002/#ps160001 (title: "forgot to update def file")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498645992234340, "parent_rev": "3244f639a3911dde9a6f935807e81a6af2f102c8", "commit_rev": "ce8882a5a1253299915174445d136f11ce063dc3"}
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498645992234340, "parent_rev": "18de1d92de6f624d8f6cb3bcf67694706490d490", "commit_rev": "93846275952fa28f83a4ea318e56c4f494bf7005"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/93846275952fa28f83a4ea318e56... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/93846275952fa28f83a4ea318e56... |