Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(59)

Issue 2428703002: Signal chrome_elf to initialize crash reporting, rather than doing it in DllMain (Closed)

Created:
2 years, 10 months ago by scottmg
Modified:
2 years, 10 months ago
Reviewers:
penny, Lei Zhang, ananta
CC:
chromium-reviews, pennymac+watch_chromium.org, caitkp+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Signal chrome_elf to initialize crash reporting, rather than doing it in DllMain Currently, initialization crash reporting indirectly calls CreateProcess() from chrome_elf's DllMain(), which causes deadlock in some configurations (showing up in M54). Instead, signal to chrome_elf that it should initialize crash reporting later in WinMain(). This means we lose a little coverage that we were hoping to gain from the move to chrome_elf, but a localized fix is required for merge to stable. Follow up bug is https://crbug.com/656800. BUG=655788, 656800 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng TEST=enforce certificate rules per https://bugs.chromium.org/p/chromium/issues/detail?id=655788#c57 and then try to launch chrome. Committed: https://crrev.com/f3a5670dd8d42b045d31625dde4a2561b471138f Cr-Commit-Position: refs/heads/master@{#425840}

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -5 lines) Patch
M chrome/app/chrome_exe_main_win.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf.def View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome_elf/chrome_elf_main.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 1 chunk +11 lines, -5 lines 0 comments Download

Messages

Total messages: 36 (18 generated)
Will Harris
On 2016/10/17 20:57:53, scottmg wrote: > Description was changed from > > ========== > Signal ...
2 years, 10 months ago (2016-10-17 21:07:45 UTC) #2
Will Harris
Seems like this should work to me. Can you add a manual TEST= line for ...
2 years, 10 months ago (2016-10-17 21:07:57 UTC) #3
ananta
lgtm
2 years, 10 months ago (2016-10-17 21:09:13 UTC) #6
scottmg
On 2016/10/17 21:07:57, Will Harris wrote: > Seems like this should work to me. Can ...
2 years, 10 months ago (2016-10-17 21:10:05 UTC) #10
scottmg
pennymac: chrome_elf/ OWNERS thestig: for small change in chrome_exe_main_win.cc (sorry for going to chrome/OWNERS, my ...
2 years, 10 months ago (2016-10-17 21:21:14 UTC) #13
penny
On 2016/10/17 21:21:14, scottmg wrote: > pennymac: chrome_elf/ OWNERS > thestig: for small change in ...
2 years, 10 months ago (2016-10-17 21:32:41 UTC) #17
penny
On 2016/10/17 21:32:41, penny wrote: > On 2016/10/17 21:21:14, scottmg wrote: > > pennymac: chrome_elf/ ...
2 years, 10 months ago (2016-10-17 21:36:40 UTC) #18
scottmg
On 2016/10/17 21:32:41, penny wrote: > On 2016/10/17 21:21:14, scottmg wrote: > > pennymac: chrome_elf/ ...
2 years, 10 months ago (2016-10-17 21:37:43 UTC) #19
penny
On 2016/10/17 21:37:43, scottmg wrote: > On 2016/10/17 21:32:41, penny wrote: > > On 2016/10/17 ...
2 years, 10 months ago (2016-10-17 21:46:56 UTC) #22
penny
On 2016/10/17 21:46:56, penny wrote: > On 2016/10/17 21:37:43, scottmg wrote: > > On 2016/10/17 ...
2 years, 10 months ago (2016-10-17 22:46:56 UTC) #24
Lei Zhang
lgtm
2 years, 10 months ago (2016-10-17 22:58:12 UTC) #25
scottmg
On 2016/10/17 22:46:56, penny wrote: > On 2016/10/17 21:46:56, penny wrote: > > On 2016/10/17 ...
2 years, 10 months ago (2016-10-17 23:14:39 UTC) #27
scottmg
On 2016/10/17 23:14:39, scottmg wrote: > On 2016/10/17 22:46:56, penny wrote: > > On 2016/10/17 ...
2 years, 10 months ago (2016-10-17 23:27:46 UTC) #28
scottmg
On 2016/10/17 23:27:46, scottmg wrote: > On 2016/10/17 23:14:39, scottmg wrote: > > On 2016/10/17 ...
2 years, 10 months ago (2016-10-17 23:44:09 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2428703002/40001
2 years, 10 months ago (2016-10-17 23:45:12 UTC) #32
scottmg
It looks like some the test binaries are going to fail on win10_chromium_x64_rel_ng for a ...
2 years, 10 months ago (2016-10-18 00:55:04 UTC) #33
scottmg
On 2016/10/18 00:55:04, scottmg wrote: > It looks like some the test binaries are going ...
2 years, 10 months ago (2016-10-18 00:56:47 UTC) #34
commit-bot: I haz the power
2 years, 10 months ago (2016-10-18 01:05:31 UTC) #36
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/f3a5670dd8d42b045d31625dde4a2561b471138f
Cr-Commit-Position: refs/heads/master@{#425840}

Powered by Google App Engine
This is Rietveld 408576698