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

Issue 2053953002: Add chrome_crash_reporter_client_win.cc to the source file list for chrome_elf (Closed)

Created:
4 years, 6 months ago by ananta
Modified:
4 years, 6 months ago
CC:
chromium-reviews, darin-cc_chromium.org, kalyank, sadrul, 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

Add chrome_crash_reporter_client_win.cc to the source file list for chrome_elf This is in preparation for switching the exception handling in chrome_elf from breakpad to crashpad. I will do that in a subsequent patch. This patch contains the following changes. 1. base Changes here are to restrict the usage of FilePath and file_util from the portions of the base which are referenced by crashpad and ChromeCrashReporterClient. These in turn end up bringing in dependencies on message_loop which won't work in chrome_elf. Fixed logging.cc to not use file path and file util and instead use Windows API's to achieve the same result. Fixed platform_thread_win.cc to avoid using windows_version.h as that pulls in object watcher and other badness. 2. crash_pad_win.cc. Remove usage of PathService and avoid using startup_metric_utils. That pulls in registry.h which in turn pulls in object watcher. 3. ChromeCrashReporterClient (Windows). Removed usage of chrome::RegisterChromeCrashKeys and instead locally defined a smaller version of that function. I added a TODO at the top here to remove these functions and the RegisterKeys function from the CrashClient interface, whenever scottmg's patch which avoids pre registration of crash keys lands. I also added a static function InitializeCrashReportingForProcess to this class which does the work for registering the process with crashpad. 4. install_static. Added some constants to this library and a new function GetSwitchValueFromCommandLine to retrieve the value of a switch from the command line. This is to avoid depending on shell32 via base command line. Added a unittest for this function. 5. base\message_loop\message_pump_win.cc This file gets pulled in by users of platform_thread_win.cc which is used indirectly by lots of base code. This line tracked_objects::ThreadData::InitializeThreadContext(name); brings in the deps via TRACE_EVENT. I changed the deps to function pointers with CHECKs for user32 being loaded and the function pointers being valid to catch these in the wild. BUG=604923 TEST=InstallStaticTest.GetSwitchValueFromCommandLineTest Committed: https://crrev.com/f265187bf390845c6d353fa7c4a9a72575fd86dd Cr-Commit-Position: refs/heads/master@{#400278}

Patch Set 1 #

Patch Set 2 : git cl format #

Patch Set 3 : Revert changes to logging.h #

Patch Set 4 : Fix gn redness (Attempt 1) #

Patch Set 5 : Fix gn redness (Attempt 2) #

Patch Set 6 : Fix gn redness (Attempt 3) #

Patch Set 7 : Add content:result_codes to the deps of chrome_elf to fix gn redness #

Patch Set 8 : Remove usage of PathService #

Patch Set 9 : Fix kasko annotations #

Total comments: 23

Patch Set 10 : Address review comments #

Total comments: 1

Patch Set 11 : Avoid depending on user32 from message_pump_win.cc #

Patch Set 12 : Fix windows redness #

Patch Set 13 : Add a number of dlls to the chrome_elf delay load list for gn. These are pulled in by the third_par… #

Patch Set 14 : Add user32 to the list of imports in chrome_elf in build.gn. This dependency comes in via the app_n… #

Patch Set 15 : Fix build error with GetCommandLineW #

Patch Set 16 : Add dbghelp.dll to the list of delay loads #

Total comments: 12

Patch Set 17 : Address review comments #

Total comments: 8

Patch Set 18 : Address review comments #

Total comments: 7

Patch Set 19 : Address review comments #

Total comments: 2

Patch Set 20 : Address review comments #

Total comments: 12

Patch Set 21 : Address review comments #

Total comments: 11

Patch Set 22 : Address base review comments #

Total comments: 4

Patch Set 23 : Fix presubmit #

Patch Set 24 : Flip the arguments of EXPECT_EQ in the install_static unittest #

Patch Set 25 : Address review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+514 lines, -107 lines) Patch
M base/debug/stack_trace_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +2 lines, -3 lines 0 comments Download
M base/files/file_path.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +6 lines, -2 lines 0 comments Download
M base/logging.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +15 lines, -6 lines 0 comments Download
M base/message_loop/message_pump_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +3 lines, -3 lines 0 comments Download
M base/message_loop/message_pump_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 21 chunks +119 lines, -34 lines 0 comments Download
M base/process/launch.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +2 lines, -2 lines 0 comments Download
M base/process/launch_win.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +0 lines, -12 lines 0 comments Download
M base/strings/string_number_conversions.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -2 lines 0 comments Download
M base/strings/string_util.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -1 line 0 comments Download
M base/threading/platform_thread_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +1 line, -4 lines 0 comments Download
M base/trace_event/winheap_dump_provider_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +1 line, -8 lines 0 comments Download
M chrome/app/chrome_crash_reporter_client_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/app/chrome_crash_reporter_client_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +187 lines, -4 lines 0 comments Download
M chrome/install_static/install_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/install_static/install_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +51 lines, -0 lines 0 comments Download
M chrome/install_static/install_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +64 lines, -0 lines 0 comments Download
M chrome_elf/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +21 lines, -1 line 0 comments Download
M chrome_elf/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M chrome_elf/chrome_elf.gyp View 2 chunks +3 lines, -0 lines 0 comments Download
M components/crash/content/app/crashpad.cc View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -3 lines 0 comments Download
M components/crash/content/app/crashpad_win.cc View 1 2 3 4 5 6 7 8 9 10 11 7 chunks +17 lines, -22 lines 0 comments Download
M content/browser/browser_main_runner.cc View 1 chunk +1 line, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 45 (11 generated)
ananta
scottmg: please review everything. robertsheild: please review changes to chrome_elf grt: please review install_static and ...
4 years, 6 months ago (2016-06-09 22:43:43 UTC) #2
ananta
Looking at the windows release builder redness. chrome_elf builds fine locally for me on 32 ...
4 years, 6 months ago (2016-06-10 05:54:57 UTC) #3
grt (UTC plus 2)
quick scan: looks good from the nitpicky point of view. i'm a bit concerned that ...
4 years, 6 months ago (2016-06-10 14:20:01 UTC) #5
fdoray
Pre-read change LGTM https://codereview.chromium.org/2053953002/diff/150001/components/crash/content/app/crashpad_win.cc File components/crash/content/app/crashpad_win.cc (left): https://codereview.chromium.org/2053953002/diff/150001/components/crash/content/app/crashpad_win.cc#oldcode104 components/crash/content/app/crashpad_win.cc:104: if (startup_metric_utils::GetPreReadOptions().use_prefetch_argument) { On 2016/06/10 14:20:01, ...
4 years, 6 months ago (2016-06-10 17:37:42 UTC) #6
robertshield
https://codereview.chromium.org/2053953002/diff/150001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/2053953002/diff/150001/chrome_elf/chrome_elf.gyp#newcode40 chrome_elf/chrome_elf.gyp:40: '../chrome/app/chrome_crash_reporter_client_win.cc', The GN config adds an explicit dependency on ...
4 years, 6 months ago (2016-06-10 18:09:56 UTC) #7
ananta
grt, there is a chrome_elf_unittest which checks the import table of chrome_elf to see if ...
4 years, 6 months ago (2016-06-10 18:29:57 UTC) #8
grt (UTC plus 2)
lgtm. do we have a test that induces a crash and verifies that the crash ...
4 years, 6 months ago (2016-06-10 18:52:34 UTC) #9
robertshield
Changes in chrome_elf lgtm
4 years, 6 months ago (2016-06-10 19:06:09 UTC) #10
scottmg
Is there anywhere it'd make sense to add a presubmit check to try to help ...
4 years, 6 months ago (2016-06-10 19:36:19 UTC) #11
ananta
On 2016/06/10 19:36:19, scottmg wrote: > Is there anywhere it'd make sense to add a ...
4 years, 6 months ago (2016-06-11 03:38:02 UTC) #13
ananta
guys, please take a peek at the patch again. This has grown a touch due ...
4 years, 6 months ago (2016-06-14 01:34:41 UTC) #15
ananta
I plan to update the ELFImportsTest.ChromeElfSanityCheck test to check for delay loads as well. At ...
4 years, 6 months ago (2016-06-14 01:35:56 UTC) #16
grt (UTC plus 2)
what are benefits of manually resolving functions vs delay loading (which can be done deterministically ...
4 years, 6 months ago (2016-06-14 02:20:49 UTC) #18
ananta
As it stands delay load of user32.dll may work for chrome_elf. However once we get ...
4 years, 6 months ago (2016-06-14 03:24:05 UTC) #19
scottmg
https://codereview.chromium.org/2053953002/diff/310001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/310001/base/files/file_path.cc#newcode697 base/files/file_path.cc:697: static CharUpperFunction char_upper_api = reinterpret_cast<CharUpperFunction>( Instead of CharUpperFunction, decltype(::CharUpperW)*. ...
4 years, 6 months ago (2016-06-14 15:58:48 UTC) #20
ananta
https://codereview.chromium.org/2053953002/diff/310001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/310001/base/files/file_path.cc#newcode697 base/files/file_path.cc:697: static CharUpperFunction char_upper_api = reinterpret_cast<CharUpperFunction>( On 2016/06/14 15:58:48, scottmg ...
4 years, 6 months ago (2016-06-14 19:13:15 UTC) #21
scottmg
All very frightening! We definitely need that improved test to detect user32. lgtm https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/message_pump_win.cc File ...
4 years, 6 months ago (2016-06-14 19:50:36 UTC) #22
ananta
https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/message_pump_win.cc File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/message_pump_win.cc#newcode61 base/message_loop/message_pump_win.cc:61: HMODULE user32_dll = ::GetModuleHandle(L"user32.dll"); On 2016/06/14 19:50:36, scottmg wrote: ...
4 years, 6 months ago (2016-06-14 19:58:54 UTC) #23
grt (UTC plus 2)
https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/message_pump_win.cc File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/message_pump_win.cc#newcode53 base/message_loop/message_pump_win.cc:53: ::GetModuleHandle(L"user32.dll"), #name)) is this going to get the loader ...
4 years, 6 months ago (2016-06-14 20:07:13 UTC) #24
ananta
https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/message_pump_win.cc File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/message_pump_win.cc#newcode53 base/message_loop/message_pump_win.cc:53: ::GetModuleHandle(L"user32.dll"), #name)) On 2016/06/14 20:07:13, grt (slow) wrote: > ...
4 years, 6 months ago (2016-06-14 21:27:41 UTC) #25
grt (UTC plus 2)
lgtm w/ nits and test requests below. https://codereview.chromium.org/2053953002/diff/370001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/files/file_path.cc#newcode696 base/files/file_path.cc:696: static decltype(::CharUpperW)* ...
4 years, 6 months ago (2016-06-15 15:49:21 UTC) #26
ananta
+thestig for base owners https://codereview.chromium.org/2053953002/diff/370001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/files/file_path.cc#newcode696 base/files/file_path.cc:696: static decltype(::CharUpperW)* char_upper_api = On ...
4 years, 6 months ago (2016-06-15 22:10:28 UTC) #28
ananta
I did a test with LoadLibrary of chrome_elf.dll with the changes in this patch. It ...
4 years, 6 months ago (2016-06-15 22:40:03 UTC) #29
Lei Zhang
https://codereview.chromium.org/2053953002/diff/390001/base/debug/stack_trace_win.cc File base/debug/stack_trace_win.cc (right): https://codereview.chromium.org/2053953002/diff/390001/base/debug/stack_trace_win.cc#newcode9 base/debug/stack_trace_win.cc:9: #include <memory> nit: C++ header goes in the section ...
4 years, 6 months ago (2016-06-15 22:50:45 UTC) #30
ananta
https://codereview.chromium.org/2053953002/diff/390001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/2053953002/diff/390001/base/logging.cc#newcode293 base/logging.cc:293: DWORD len = ::GetCurrentDirectory(arraysize(system_buffer), On 2016/06/15 22:50:45, Lei Zhang ...
4 years, 6 months ago (2016-06-15 23:09:53 UTC) #31
Lei Zhang
lgtm https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_crash_reporter_client_win.h File chrome/app/chrome_crash_reporter_client_win.h (right): https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_crash_reporter_client_win.h#newcode18 chrome/app/chrome_crash_reporter_client_win.h:18: #endif would be nice to have a blank ...
4 years, 6 months ago (2016-06-15 23:20:47 UTC) #32
ananta
https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_crash_reporter_client_win.h File chrome/app/chrome_crash_reporter_client_win.h (right): https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_crash_reporter_client_win.h#newcode18 chrome/app/chrome_crash_reporter_client_win.h:18: #endif On 2016/06/15 23:20:46, Lei Zhang wrote: > would ...
4 years, 6 months ago (2016-06-16 00:29:30 UTC) #33
ananta
+jam for content owners
4 years, 6 months ago (2016-06-16 00:30:01 UTC) #35
jam
On 2016/06/16 00:30:01, ananta wrote: > +jam for content owners lgtm
4 years, 6 months ago (2016-06-16 16:19:11 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053953002/470001
4 years, 6 months ago (2016-06-16 19:31:08 UTC) #39
commit-bot: I haz the power
Committed patchset #25 (id:470001)
4 years, 6 months ago (2016-06-16 22:21:19 UTC) #41
commit-bot: I haz the power
Patchset 25 (id:??) landed as https://crrev.com/f265187bf390845c6d353fa7c4a9a72575fd86dd Cr-Commit-Position: refs/heads/master@{#400278}
4 years, 6 months ago (2016-06-16 22:23:09 UTC) #43
benwells
A revert of this CL (patchset #25 id:470001) has been created in https://codereview.chromium.org/2083463003/ by benwells@chromium.org. ...
4 years, 6 months ago (2016-06-20 10:19:14 UTC) #44
benwells
4 years, 6 months ago (2016-06-20 10:34:49 UTC) #45
Message was sent while issue was closed.
On 2016/06/20 10:19:14, benwells wrote:
> A revert of this CL (patchset #25 id:470001) has been created in
> https://codereview.chromium.org/2083463003/ by mailto:benwells@chromium.org.
> 
> The reason for reverting is: It looks like this patch introduced test failures
> on DrMemory.
> 
> First failing build:
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...
> 
> Sample error:
> ELFImportsTest.ChromeElfSanityCheck:
> c:\b\build\slave\drm-cr\build\src\chrome_elf\elf_imports_unittest.cc(107):
> error: Value of: match
> Actual: false
> Expected: true
> Illegal import in chrome_elf.dll: base.dll.

Revert didn't apply, have created http://crbug.com/621460 to fix this.

Powered by Google App Engine
This is Rietveld 408576698