Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 6 months ago by ananta Modified:
4 years, 6 months ago Reviewers:
Lei Zhang, fdoray, jam, scottmg, robertshield, grt (UTC plus 2) 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. |
DescriptionAdd 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 #
Created: 4 years, 6 months ago
Depends on Patchset: Messages
Total messages: 45 (11 generated)
ananta@chromium.org changed reviewers: + grt@chromium.org, robertshield@chromium.org, scottmg@chromium.org
scottmg: please review everything. robertsheild: please review changes to chrome_elf grt: please review install_static and the whole patch if you like :)
Looking at the windows release builder redness. chrome_elf builds fine locally for me on 32 and 64 bit builds. https://codereview.chromium.org/2053953002/diff/150001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/150001/base/files/file_path.c... base/files/file_path.cc:698: ::GetProcAddress(::GetModuleHandle(L"user32.dll"), "CharUpperW")); Will change this to use a static function pointer.
grt@chromium.org changed reviewers: + fdoray@chromium.org
quick scan: looks good from the nitpicky point of view. i'm a bit concerned that the approach seems fragile. is any of this covered by unittests? +fdoray to approve the preread options change. https://codereview.chromium.org/2053953002/diff/150001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/2053953002/diff/150001/base/logging.cc#newcod... base/logging.cc:294: if (len == 0 || len > MAX_PATH) nit: MAX_PATH -> arraysize(system_buffer) https://codereview.chromium.org/2053953002/diff/150001/base/logging.cc#newcod... base/logging.cc:299: if ((*g_log_file_name)[g_log_file_name->length() - 1] != L'\\') ? if (g_log_file_name->back() != L'\\') https://codereview.chromium.org/2053953002/diff/150001/base/threading/platfor... File base/threading/platform_thread_win.cc (left): https://codereview.chromium.org/2053953002/diff/150001/base/threading/platfor... base/threading/platform_thread_win.cc:103: if (stack_size > 0 && base::win::GetVersion() >= base::win::VERSION_XP) { yay! https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:31: const char kActiveURL[] = "url-chunk"; nit: constexpr for compile-time constants https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:77: base::debug::CrashKey fixed_keys[] = { can/should this be constexpr? this will avoid pushing all this stuff onto the stack https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:175: return base::debug::InitCrashKeys(&keys.at(0), keys.size(), kChunkMaxLength); use &keys[0] since we don't handle exceptions and don't need the range checking that at() provides. https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:194: base::CommandLine::Init(0, nullptr); remove https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... components/crash/content/app/crashpad.cc:371: ::GetModuleFileName(NULL, exe_file, arraysize(exe_file)); CHECK? https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... components/crash/content/app/crashpad.cc:371: ::GetModuleFileName(NULL, exe_file, arraysize(exe_file)); nullptr everywhere https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (left): https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... components/crash/content/app/crashpad_win.cc:104: if (startup_metric_utils::GetPreReadOptions().use_prefetch_argument) { fdoray: PTAL
Pre-read change LGTM https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (left): https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... components/crash/content/app/crashpad_win.cc:104: if (startup_metric_utils::GetPreReadOptions().use_prefetch_argument) { On 2016/06/10 14:20:01, grt (slow) wrote: > fdoray: PTAL The pre-read experiment is completed. Removing this code is on my TODO list so lgtm!
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.... chrome_elf/chrome_elf.gyp:40: '../chrome/app/chrome_crash_reporter_client_win.cc', The GN config adds an explicit dependency on "//chrome/common/chrome_result_codes.h", should we not do that here too?
grt, there is a chrome_elf_unittest which checks the import table of chrome_elf to see if there are any dependencies outside the ones we allow, i.e. msvcrt, kernel32 and advapi32. That is the only test we have at the moment. https://codereview.chromium.org/2053953002/diff/150001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/2053953002/diff/150001/base/logging.cc#newcod... base/logging.cc:294: if (len == 0 || len > MAX_PATH) On 2016/06/10 14:20:00, grt (slow) wrote: > nit: MAX_PATH -> arraysize(system_buffer) Done. https://codereview.chromium.org/2053953002/diff/150001/base/logging.cc#newcod... base/logging.cc:299: if ((*g_log_file_name)[g_log_file_name->length() - 1] != L'\\') On 2016/06/10 14:20:00, grt (slow) wrote: > ? if (g_log_file_name->back() != L'\\') Done. https://codereview.chromium.org/2053953002/diff/150001/base/threading/platfor... File base/threading/platform_thread_win.cc (left): https://codereview.chromium.org/2053953002/diff/150001/base/threading/platfor... base/threading/platform_thread_win.cc:103: if (stack_size > 0 && base::win::GetVersion() >= base::win::VERSION_XP) { On 2016/06/10 14:20:00, grt (slow) wrote: > yay! Yeah. Harmless line of code which pulled in user32 :) https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:31: const char kActiveURL[] = "url-chunk"; On 2016/06/10 14:20:01, grt (slow) wrote: > nit: constexpr for compile-time constants Done. https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:77: base::debug::CrashKey fixed_keys[] = { On 2016/06/10 14:20:01, grt (slow) wrote: > can/should this be constexpr? this will avoid pushing all this stuff onto the > stack Done. I am hoping to remove this code soon. Once scottmg's patch lands that is. https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:175: return base::debug::InitCrashKeys(&keys.at(0), keys.size(), kChunkMaxLength); On 2016/06/10 14:20:01, grt (slow) wrote: > use &keys[0] since we don't handle exceptions and don't need the range checking > that at() provides. Done. https://codereview.chromium.org/2053953002/diff/150001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:194: base::CommandLine::Init(0, nullptr); On 2016/06/10 14:20:01, grt (slow) wrote: > remove done. 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.... chrome_elf/chrome_elf.gyp:40: '../chrome/app/chrome_crash_reporter_client_win.cc', On 2016/06/10 18:09:56, robertshield wrote: > The GN config adds an explicit dependency on > "//chrome/common/chrome_result_codes.h", should we not do that here too? The config adds a dependency on content/public/common:result_codes. That is fine. The chrome_result_codes.h is part of a lib which has other items. I don't want to pull in additional dependencies which could cause problems. https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2053953002/diff/150001/components/crash/conte... components/crash/content/app/crashpad.cc:371: ::GetModuleFileName(NULL, exe_file, arraysize(exe_file)); On 2016/06/10 14:20:01, grt (slow) wrote: > CHECK? Done.
lgtm. do we have a test that induces a crash and verifies that the crash keys appear properly in the dump? https://codereview.chromium.org/2053953002/diff/170001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2053953002/diff/170001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:77: constexpr base::debug::CrashKey fixed_keys[] = { oops: this should become kFixedKeys, too. apologies
Changes in chrome_elf lgtm
Is there anywhere it'd make sense to add a presubmit check to try to help people not start pulling in user32 by accident? https://codereview.chromium.org/2053953002/diff/150001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/150001/base/files/file_path.c... base/files/file_path.cc:702: // http://blogs.msdn.com/michkap/archive/2005/10/17/481600.aspx The linked blog post http://web.archive.org/web/20100223192005/http://blogs.msdn.com/michkap/archi... mentions "LCMapString with the LCMAP_UPPERCASE flag (and without the LCMAP_LINGUISTIC_CASING flag!)" as being an option also, and that's in kernel32.
Description was changed from ========== 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. BUG=604923 ========== to ========== 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. 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 ==========
On 2016/06/10 19:36:19, scottmg wrote: > Is there anywhere it'd make sense to add a presubmit check to try to help people > not start pulling in user32 by accident? > > https://codereview.chromium.org/2053953002/diff/150001/base/files/file_path.cc > File base/files/file_path.cc (right): > > https://codereview.chromium.org/2053953002/diff/150001/base/files/file_path.c... > base/files/file_path.cc:702: // > http://blogs.msdn.com/michkap/archive/2005/10/17/481600.aspx > The linked blog post > > http://web.archive.org/web/20100223192005/http://blogs.msdn.com/michkap/archi... > > mentions "LCMapString with the LCMAP_UPPERCASE flag (and without the > LCMAP_LINGUISTIC_CASING flag!)" as being an option also, and that's in kernel32. It would be tricky to add a presubmit check as we don't know beforehand what in base would be indirectly pulled in.
Description was changed from ========== 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. 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 ========== to ========== 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 ==========
guys, please take a peek at the patch again. This has grown a touch due to redness with the gn builds pulling in user32.dll via components\crash. These have been addressed in this patch. grt, please go over install_static again and over the whole patch if you have time. scottmg, please look over the whole patch. robertshield, please look at the changes at chrome_elf. These look scary I agree. It should work fine if I am understanding the code right.
I plan to update the ELFImportsTest.ChromeElfSanityCheck test to check for delay loads as well. At the moment this is a touch tricky because the list of delay loads is different between gn and gyp.
Description was changed from ========== 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 ========== to ========== 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 ==========
what are benefits of manually resolving functions vs delay loading (which can be done deterministically with error handling; see https://chromiumcodereview.appspot.com/11299332)? https://codereview.chromium.org/2053953002/diff/290001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/290001/base/files/file_path.c... base/files/file_path.cc:696: using CharUpperFunction = wchar_t*(WINAPI*)(wchar_t * str); nit: "wchar_t* str" https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... base/message_loop/message_pump_win.cc:83: static bool init = false; nit: the process will die if any of these fails, so there's no need for the extra bit of state. how about simply: if (g_kill_timer) return; https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... File base/message_loop/message_pump_win.h (right): https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... base/message_loop/message_pump_win.h:63: LONG work_state_; since you're touching this and the ctor, please switch to the new style: = READY https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... base/message_loop/message_pump_win.h:66: RunState* state_; = nullptr https://codereview.chromium.org/2053953002/diff/290001/base/process/launch_wi... File base/process/launch_win.cc (left): https://codereview.chromium.org/2053953002/diff/290001/base/process/launch_wi... base/process/launch_win.cc:220: if (base::win::GetVersion() < base::win::VERSION_VISTA) { awesome! please update the comments in launch.h for handles_to_inherit and inherit_handles; there's no longer a need for them to say "on Vista and higher". https://codereview.chromium.org/2053953002/diff/290001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2053953002/diff/290001/chrome/install_static/... chrome/install_static/install_util.cc:917: switch_token += switch_name; wdyt of searching for "--switch=" here? if switch_offset != npos, then switch_offset + switch_token.size() is the start of the value. is there a case that this misses?
As it stands delay load of user32.dll may work for chrome_elf. However once we get rid of the offending target in components\crash which brings in user32 via breakpad, then the current approach would allow us to catch user32 dependencies sneaking into base which would hurt chrome_elf. https://codereview.chromium.org/2053953002/diff/290001/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2053953002/diff/290001/base/files/file_path.c... base/files/file_path.cc:696: using CharUpperFunction = wchar_t*(WINAPI*)(wchar_t * str); On 2016/06/14 02:20:49, grt (slow) wrote: > nit: "wchar_t* str" Done. Thought git cl format would fix it. https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... base/message_loop/message_pump_win.cc:83: static bool init = false; On 2016/06/14 02:20:49, grt (slow) wrote: > nit: the process will die if any of these fails, so there's no need for the > extra bit of state. how about simply: > > if (g_kill_timer) > return; Done. https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... File base/message_loop/message_pump_win.h (right): https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... base/message_loop/message_pump_win.h:63: LONG work_state_; On 2016/06/14 02:20:49, grt (slow) wrote: > since you're touching this and the ctor, please switch to the new style: > = READY Done. https://codereview.chromium.org/2053953002/diff/290001/base/message_loop/mess... base/message_loop/message_pump_win.h:66: RunState* state_; On 2016/06/14 02:20:49, grt (slow) wrote: > = nullptr Done. https://codereview.chromium.org/2053953002/diff/290001/base/process/launch_wi... File base/process/launch_win.cc (left): https://codereview.chromium.org/2053953002/diff/290001/base/process/launch_wi... base/process/launch_win.cc:220: if (base::win::GetVersion() < base::win::VERSION_VISTA) { On 2016/06/14 02:20:49, grt (slow) wrote: > awesome! please update the comments in launch.h for handles_to_inherit and > inherit_handles; there's no longer a need for them to say "on Vista and higher". Done. https://codereview.chromium.org/2053953002/diff/290001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2053953002/diff/290001/chrome/install_static/... chrome/install_static/install_util.cc:917: switch_token += switch_name; On 2016/06/14 02:20:49, grt (slow) wrote: > wdyt of searching for "--switch=" here? if switch_offset != npos, then > switch_offset + switch_token.size() is the start of the value. is there a case > that this misses? Yeah. That makes it much simpler. Thanks. done
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.c... base/files/file_path.cc:697: static CharUpperFunction char_upper_api = reinterpret_cast<CharUpperFunction>( Instead of CharUpperFunction, decltype(::CharUpperW)*. (Please try the alternate kernel32 function in a followup too.) https://codereview.chromium.org/2053953002/diff/310001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/310001/base/message_loop/mess... base/message_loop/message_pump_win.cc:63: TranslateMessageFunction g_translate_message = nullptr; Use decltype here too. I think I'd do decltype(::TranslateMessage)* g_translate_message; here, and then use a macro something like #define GET_USER32_FUNCTION(name) reinterpret_cast<decltype(name)*>(::GetProcAddress(GetModuleHandle("user32.dll"), #name)) to initialize them. Or maybe have the macro call a function if you want to have the CHECK() in there. https://codereview.chromium.org/2053953002/diff/310001/base/strings/string_nu... File base/strings/string_number_conversions.cc (left): https://codereview.chromium.org/2053953002/diff/310001/base/strings/string_nu... base/strings/string_number_conversions.cc:141: nit; probably no need to delete these blank lines. https://codereview.chromium.org/2053953002/diff/310001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2053953002/diff/310001/chrome/install_static/... chrome/install_static/install_util.cc:906: const std::string& switch_name) { I guess this will fail in the same way as Google Desktop, right? Good ol' --ignored=" --type=renderer "?
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.c... base/files/file_path.cc:697: static CharUpperFunction char_upper_api = reinterpret_cast<CharUpperFunction>( On 2016/06/14 15:58:48, scottmg wrote: > Instead of CharUpperFunction, decltype(::CharUpperW)*. (Please try the alternate > kernel32 function in a followup too.) Done. https://codereview.chromium.org/2053953002/diff/310001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/310001/base/message_loop/mess... base/message_loop/message_pump_win.cc:63: TranslateMessageFunction g_translate_message = nullptr; On 2016/06/14 15:58:48, scottmg wrote: > Use decltype here too. > > I think I'd do > > decltype(::TranslateMessage)* g_translate_message; > > here, and then use a macro something like > > #define GET_USER32_FUNCTION(name) > reinterpret_cast<decltype(name)*>(::GetProcAddress(GetModuleHandle("user32.dll"), > #name)) > > to initialize them. Or maybe have the macro call a function if you want to have > the CHECK() in there. Done. https://codereview.chromium.org/2053953002/diff/310001/base/strings/string_nu... File base/strings/string_number_conversions.cc (left): https://codereview.chromium.org/2053953002/diff/310001/base/strings/string_nu... base/strings/string_number_conversions.cc:141: On 2016/06/14 15:58:48, scottmg wrote: > nit; probably no need to delete these blank lines. Done. https://codereview.chromium.org/2053953002/diff/310001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2053953002/diff/310001/chrome/install_static/... chrome/install_static/install_util.cc:906: const std::string& switch_name) { On 2016/06/14 15:58:48, scottmg wrote: > I guess this will fail in the same way as Google Desktop, right? Good ol' > --ignored=" --type=renderer "? Added a check for that with an assert
All very frightening! We definitely need that improved test to detect user32. lgtm https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... base/message_loop/message_pump_win.cc:51: #define GET_USER32_API(name) \ git cl format https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... base/message_loop/message_pump_win.cc:61: HMODULE user32_dll = ::GetModuleHandle(L"user32.dll"); Don't need this any more. https://codereview.chromium.org/2053953002/diff/330001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2053953002/diff/330001/chrome/install_static/... chrome/install_static/install_util.cc:911: // --ignored=" --type=renderer ", which we used for Google desktop. nit; desktop -> Desktop
https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... base/message_loop/message_pump_win.cc:61: HMODULE user32_dll = ::GetModuleHandle(L"user32.dll"); On 2016/06/14 19:50:36, scottmg wrote: > Don't need this any more. Done. https://codereview.chromium.org/2053953002/diff/330001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2053953002/diff/330001/chrome/install_static/... chrome/install_static/install_util.cc:911: // --ignored=" --type=renderer ", which we used for Google desktop. On 2016/06/14 19:50:36, scottmg wrote: > nit; desktop -> Desktop Done.
https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... base/message_loop/message_pump_win.cc:53: ::GetModuleHandle(L"user32.dll"), #name)) is this going to get the loader lock twice for each macro expansion? https://codereview.chromium.org/2053953002/diff/350001/chrome_elf/BUILD.gn File chrome_elf/BUILD.gn (right): https://codereview.chromium.org/2053953002/diff/350001/chrome_elf/BUILD.gn#ne... chrome_elf/BUILD.gn:53: "/DELAYLOAD:dbghelp.dll", is the idea here that these are all delayloads that should never be loaded in practice? if so, wdyt of adding a __pfnDliNotifyHook2 that CHECKs in case of an unexpected load?
https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/330001/base/message_loop/mess... base/message_loop/message_pump_win.cc:53: ::GetModuleHandle(L"user32.dll"), #name)) On 2016/06/14 20:07:13, grt (slow) wrote: > is this going to get the loader lock twice for each macro expansion? Yeah. Changed to take the module handle. https://codereview.chromium.org/2053953002/diff/350001/chrome_elf/BUILD.gn File chrome_elf/BUILD.gn (right): https://codereview.chromium.org/2053953002/diff/350001/chrome_elf/BUILD.gn#ne... chrome_elf/BUILD.gn:53: "/DELAYLOAD:dbghelp.dll", On 2016/06/14 20:07:13, grt (slow) wrote: > is the idea here that these are all delayloads that should never be loaded in > practice? if so, wdyt of adding a __pfnDliNotifyHook2 that CHECKs in case of an > unexpected load? The expectation is that these dlls were load after chrome_elf, i.e a load of chrome_elf should not cause these dlls to load via import dependencies. The dll we are concerned about is user32 which loads global hooks, etc. I think once we initialize crashpad in chrome_elf, we can add a check for user32 and crash there. Will do that in a later patch set.
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.c... base/files/file_path.cc:696: static decltype(::CharUpperW)* char_upper_api = nit: static decltype(::CharUpperW)* const char_upper_api = https://codereview.chromium.org/2053953002/diff/370001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/logging.cc#newcod... base/logging.cc:293: DWORD len = ::GetCurrentDirectory(MAX_PATH, system_buffer); nit: MAX_PATH -> arraysize(system_buffer) https://codereview.chromium.org/2053953002/diff/370001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/message_loop/mess... base/message_loop/message_pump_win.cc:31: // The following define pointer to user32 API's for the API's which are used nit: "pointers to" https://codereview.chromium.org/2053953002/diff/370001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/threading/platfor... base/threading/platform_thread_win.cc:104: } else { else clause no longer needed https://codereview.chromium.org/2053953002/diff/370001/chrome/install_static/... File chrome/install_static/install_util_unittest.cc (right): https://codereview.chromium.org/2053953002/diff/370001/chrome/install_static/... chrome/install_static/install_util_unittest.cc:137: GetSwitchValueFromCommandLine("c:\\temp\bleh.exe --type=bar", "type"); \b -> \\b here and below https://codereview.chromium.org/2053953002/diff/370001/chrome/install_static/... chrome/install_static/install_util_unittest.cc:172: } please add tests for: - nothing after =: "c:\\temp\\bleh.exe --type=" - only whitespace after =: "c:\\temp\\bleh.exe --type= " and any other permutations you can think of
ananta@chromium.org changed reviewers: + thestig@chromium.org
+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.c... base/files/file_path.cc:696: static decltype(::CharUpperW)* char_upper_api = On 2016/06/15 15:49:21, grt (slow) wrote: > nit: > static decltype(::CharUpperW)* const char_upper_api = Done. https://codereview.chromium.org/2053953002/diff/370001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/logging.cc#newcod... base/logging.cc:293: DWORD len = ::GetCurrentDirectory(MAX_PATH, system_buffer); On 2016/06/15 15:49:21, grt (slow) wrote: > nit: MAX_PATH -> arraysize(system_buffer) Done. https://codereview.chromium.org/2053953002/diff/370001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/message_loop/mess... base/message_loop/message_pump_win.cc:31: // The following define pointer to user32 API's for the API's which are used On 2016/06/15 15:49:21, grt (slow) wrote: > nit: "pointers to" Done. https://codereview.chromium.org/2053953002/diff/370001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2053953002/diff/370001/base/threading/platfor... base/threading/platform_thread_win.cc:104: } else { On 2016/06/15 15:49:21, grt (slow) wrote: > else clause no longer needed Done. https://codereview.chromium.org/2053953002/diff/370001/chrome/install_static/... File chrome/install_static/install_util_unittest.cc (right): https://codereview.chromium.org/2053953002/diff/370001/chrome/install_static/... chrome/install_static/install_util_unittest.cc:137: GetSwitchValueFromCommandLine("c:\\temp\bleh.exe --type=bar", "type"); On 2016/06/15 15:49:21, grt (slow) wrote: > \b -> \\b here and below Done. https://codereview.chromium.org/2053953002/diff/370001/chrome/install_static/... chrome/install_static/install_util_unittest.cc:172: } On 2016/06/15 15:49:21, grt (slow) wrote: > please add tests for: > - nothing after =: "c:\\temp\\bleh.exe --type=" > - only whitespace after =: "c:\\temp\\bleh.exe --type= " > and any other permutations you can think of Done.
I did a test with LoadLibrary of chrome_elf.dll with the changes in this patch. It does not load anything other than version.dll which is in the allowed list. I was planning to add a CHECK in DllMain for chrome_elf once we switch to crashpad for handling the case where user32 loads in crashpad initialization. That should help us catch these errors when they land on the bots. Longer term, having a smaller base target which only depends on kernel32, perhaps advapi32 and version.dll would perhaps be the way to go. Most of the problems I was having in this patch stem from logging. It may be a good idea to fix logging by possibly using some form of dependency injection or the like to reduce the amount of code which is pulled in via logging in consumers like chrome_elf or the windows sandbox.
https://codereview.chromium.org/2053953002/diff/390001/base/debug/stack_trace... File base/debug/stack_trace_win.cc (right): https://codereview.chromium.org/2053953002/diff/390001/base/debug/stack_trace... base/debug/stack_trace_win.cc:9: #include <memory> nit: C++ header goes in the section below. 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#newcod... base/logging.cc:293: DWORD len = ::GetCurrentDirectory(arraysize(system_buffer), Is the rest of the CL going to keep someone from looking at this, and thinking, "hey, I will replace this with base::GetCurrentDirectory()" ? https://codereview.chromium.org/2053953002/diff/390001/base/logging.cc#newcod... base/logging.cc:300: if (g_log_file_name->back() != L'\\') Does it hurt if this check goes away and we just always append? i.e. end up with c:\path\to\\debug.log. https://codereview.chromium.org/2053953002/diff/390001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/390001/base/message_loop/mess... base/message_loop/message_pump_win.cc:319: DWORD result = g_msg_wait_for_multiple_objects_ex(0, NULL, delay, s/NULL/nullptr/ here and below since we are modifying the code already? Assuming it builds with nullptrs. https://codereview.chromium.org/2053953002/diff/390001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/2053953002/diff/390001/base/process/launch.h#... base/process/launch.h:75: // handles must be inheritable). This is only supported on Vista and higher. Another Vista references. https://codereview.chromium.org/2053953002/diff/390001/base/trace_event/winhe... File base/trace_event/winheap_dump_provider_win.cc (right): https://codereview.chromium.org/2053953002/diff/390001/base/trace_event/winhe... base/trace_event/winheap_dump_provider_win.cc:58: // undefined.". This is a problem on Windows XP where some system DLLs are Since we are removing pre-Win7 bits, can this go away too?
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#newcod... base/logging.cc:293: DWORD len = ::GetCurrentDirectory(arraysize(system_buffer), On 2016/06/15 22:50:45, Lei Zhang wrote: > Is the rest of the CL going to keep someone from looking at this, and thinking, > "hey, I will replace this with base::GetCurrentDirectory()" ? It won't. We are looking into addressing this by splitting base possibly into multiple targets. On Windows for e.g. a smaller base which depends on kernel32, advapi32 and version.dll and the larger base which depends on the smaller base and then other things. That is not part of this patch though. Added a comment in the code explaining this. https://codereview.chromium.org/2053953002/diff/390001/base/logging.cc#newcod... base/logging.cc:300: if (g_log_file_name->back() != L'\\') On 2016/06/15 22:50:45, Lei Zhang wrote: > Does it hurt if this check goes away and we just always append? i.e. end up with > c:\path\to\\debug.log. That would work. I think it is better to avoid the double slashes in the path https://codereview.chromium.org/2053953002/diff/390001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/2053953002/diff/390001/base/message_loop/mess... base/message_loop/message_pump_win.cc:319: DWORD result = g_msg_wait_for_multiple_objects_ex(0, NULL, delay, On 2016/06/15 22:50:45, Lei Zhang wrote: > s/NULL/nullptr/ here and below since we are modifying the code already? Assuming > it builds with nullptrs. Done. https://codereview.chromium.org/2053953002/diff/390001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/2053953002/diff/390001/base/process/launch.h#... base/process/launch.h:75: // handles must be inheritable). This is only supported on Vista and higher. On 2016/06/15 22:50:45, Lei Zhang wrote: > Another Vista references. Thanks. Removed https://codereview.chromium.org/2053953002/diff/390001/base/trace_event/winhe... File base/trace_event/winheap_dump_provider_win.cc (right): https://codereview.chromium.org/2053953002/diff/390001/base/trace_event/winhe... base/trace_event/winheap_dump_provider_win.cc:58: // undefined.". This is a problem on Windows XP where some system DLLs are On 2016/06/15 22:50:45, Lei Zhang wrote: > Since we are removing pre-Win7 bits, can this go away too? Thanks. Done
lgtm https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.h (right): https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.h:18: #endif would be nice to have a blank line after https://codereview.chromium.org/2053953002/diff/410001/chrome/install_static/... File chrome/install_static/install_util_unittest.cc (right): https://codereview.chromium.org/2053953002/diff/410001/chrome/install_static/... chrome/install_static/install_util_unittest.cc:138: EXPECT_EQ(value, "bar"); The arguments are flipped around. As is, when this fails, gtest will print out "expected: wrong value, actual: bar" which is rather confusing. Ditto below.
https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.h (right): https://codereview.chromium.org/2053953002/diff/410001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.h:18: #endif On 2016/06/15 23:20:46, Lei Zhang wrote: > would be nice to have a blank line after Done. https://codereview.chromium.org/2053953002/diff/410001/chrome/install_static/... File chrome/install_static/install_util_unittest.cc (right): https://codereview.chromium.org/2053953002/diff/410001/chrome/install_static/... chrome/install_static/install_util_unittest.cc:138: EXPECT_EQ(value, "bar"); On 2016/06/15 23:20:46, Lei Zhang wrote: > The arguments are flipped around. As is, when this fails, gtest will print out > "expected: wrong value, actual: bar" which is rather confusing. > > Ditto below. Done.
ananta@chromium.org changed reviewers: + jam@chromium.org
+jam for content owners
On 2016/06/16 00:30:01, ananta wrote: > +jam for content owners lgtm
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, robertshield@chromium.org, scottmg@chromium.org, grt@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2053953002/#ps470001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053953002/470001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:470001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/f265187bf390845c6d353fa7c4a9a72575fd86dd Cr-Commit-Position: refs/heads/master@{#400278}
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:470001) has been created in https://codereview.chromium.org/2083463003/ by 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.
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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
