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

ananta

ananta@chromium.org changed reviewers: + grt@chromium.org, robertshield@chromium.org, scottmg@chromium.org

4 years, 6 months ago (2016-06-09 22:43:42 UTC) #1

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)

grt@chromium.org changed reviewers: + fdoray@chromium.org

4 years, 6 months ago (2016-06-10 14:19:59 UTC) #4

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

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

Description was changed from ========== Add chrome_crash_reporter_client_win.cc to the source file list for chrome_elf This ...

4 years, 6 months ago (2016-06-11 03:32:33 UTC) #12

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
==========

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

Description was changed from ========== Add chrome_crash_reporter_client_win.cc to the source file list for chrome_elf This ...

4 years, 6 months ago (2016-06-14 01:29:25 UTC) #14

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
==========

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)

Description was changed from ========== Add chrome_crash_reporter_client_win.cc to the source file list for chrome_elf This ...

4 years, 6 months ago (2016-06-14 01:55:16 UTC) #17

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
==========

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

ananta@chromium.org changed reviewers: + thestig@chromium.org

4 years, 6 months ago (2016-06-15 22:10:26 UTC) #27

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

ananta@chromium.org changed reviewers: + jam@chromium.org

4 years, 6 months ago (2016-06-16 00:29:59 UTC) #34

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

ananta

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, ...

4 years, 6 months ago (2016-06-16 19:30:37 UTC) #38

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

Description was changed from ========== Add chrome_crash_reporter_client_win.cc to the source file list for chrome_elf This ...

4 years, 6 months ago (2016-06-16 22:21:18 UTC) #40

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
==========

commit-bot: I haz the power

Description was changed from ========== Add chrome_crash_reporter_client_win.cc to the source file list for chrome_elf This ...

4 years, 6 months ago (2016-06-16 22:23:07 UTC) #42

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}
==========

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.

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

Description

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 #

Patch Set 10 : Address review comments #

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 #

Patch Set 17 : Address review comments #

Patch Set 18 : Address review comments #

Patch Set 19 : Address review comments #

Patch Set 20 : Address review comments #

Patch Set 21 : Address review comments #

Patch Set 22 : Address base review comments #

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 #

Messages