|
|
DescriptionAdded crashpad support for Windows
This mimics behavior with Mac and Linux, with --enable-crash-reporter
--crash-dumps-dir in headless_shell and chrome --headless
Note that browsertests cannot be enabled as they are since in Windows the crash handler
is forked from the Headless Shell.
BUG=686608
Review-Url: https://codereview.chromium.org/2835913002
Cr-Commit-Position: refs/heads/master@{#472017}
Committed: https://chromium.googlesource.com/chromium/src/+/266c2738a4fb94018caba538e308c60a44cb9646
Patch Set 1 #
Total comments: 2
Patch Set 2 : Avoid adding crashpad dependencies in chrome.dll and chrome_child.dll #Patch Set 3 : Remove unnecessary dependency #Patch Set 4 : fix for windows component build #Patch Set 5 : use CHROME_MULTIPLE_DLL instead #
Total comments: 2
Patch Set 6 : Added comment #
Messages
Total messages: 32 (21 generated)
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dvallet@chromium.org changed reviewers: + eseckler@chromium.org, skyostil@chromium.org
lgtm https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell... headless/app/headless_shell.cc:525: RunChildProcessIfNeeded(argc, argv); I'd have expected a call to base::CommandLine::Init here, too. Codesearch actually also shows one. Maybe this just needs syncing? *confused* :P
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell... headless/app/headless_shell.cc:525: RunChildProcessIfNeeded(argc, argv); On 2017/04/24 09:06:14, Eric Seckler wrote: > I'd have expected a call to base::CommandLine::Init here, too. Codesearch > actually also shows one. Maybe this just needs syncing? *confused* :P That's probably due to this patch: https://codereview.chromium.org/2836823002/ Maybe we could unify this so that the command line is always initialized on all platforms?
On 2017/04/24 19:04:28, Sami wrote: > lgtm > > https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell.cc > File headless/app/headless_shell.cc (right): > > https://codereview.chromium.org/2835913002/diff/1/headless/app/headless_shell... > headless/app/headless_shell.cc:525: RunChildProcessIfNeeded(argc, argv); > On 2017/04/24 09:06:14, Eric Seckler wrote: > > I'd have expected a call to base::CommandLine::Init here, too. Codesearch > > actually also shows one. Maybe this just needs syncing? *confused* :P > > That's probably due to this patch: > > https://codereview.chromium.org/2836823002/ > > Maybe we could unify this so that the command line is always initialized on all > platforms? Ah, didn't see the patch set dependency :) Unification sgtm!
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dvallet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. I had to modify this so that dependencies are not added to chrome.dll and chrome_child.dll Note that HEADLESS_USE_CRASHPAD needs to be used for headless_shell.cc since this is compiled in the headles_shell libs and I only want to add it when linking headless_shell, since chrome --headless already initializes crashpad and adding the dependency increases dll size unnecessarily. headless_content_main_delegate.cc gets linked in the component in component builds, so I check for CHROME_MULTIPLE_DLL instead. This will make it call init in component builds, but has no effect. I'm still looking into cleaning up this a bit but this will do for now. I'm still looking into cleaning up this a bit but this will do.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_c... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_c... headless/lib/headless_content_main_delegate.cc:183: #elif defined(OS_WIN) && !defined(CHROME_MULTIPLE_DLL) nit: add a comment why we don't need to do this for CHROME_MULTIPLE_DLL.
lgtm -- good point about adding a comment.
Thanks for the review! https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_c... File headless/lib/headless_content_main_delegate.cc (right): https://codereview.chromium.org/2835913002/diff/80001/headless/lib/headless_c... headless/lib/headless_content_main_delegate.cc:183: #elif defined(OS_WIN) && !defined(CHROME_MULTIPLE_DLL) On 2017/05/15 at 08:47:02, Eric Seckler wrote: > nit: add a comment why we don't need to do this for CHROME_MULTIPLE_DLL. Done
The CQ bit was checked by dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2835913002/#ps100001 (title: "Added comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by dvallet@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494903336212290, "parent_rev": "f6706c46e3ee46e567028602861da551164c1772", "commit_rev": "266c2738a4fb94018caba538e308c60a44cb9646"}
Message was sent while issue was closed.
Description was changed from ========== Added crashpad support for Windows This mimics behavior with Mac and Linux, with --enable-crash-reporter --crash-dumps-dir in headless_shell and chrome --headless Note that browsertests cannot be enabled as they are since in Windows the crash handler is forked from the Headless Shell. BUG=686608 ========== to ========== Added crashpad support for Windows This mimics behavior with Mac and Linux, with --enable-crash-reporter --crash-dumps-dir in headless_shell and chrome --headless Note that browsertests cannot be enabled as they are since in Windows the crash handler is forked from the Headless Shell. BUG=686608 Review-Url: https://codereview.chromium.org/2835913002 Cr-Commit-Position: refs/heads/master@{#472017} Committed: https://chromium.googlesource.com/chromium/src/+/266c2738a4fb94018caba538e308... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/266c2738a4fb94018caba538e308... |