Audit calls to DumpWithoutCrashing |
|||||||||||||||||
Issue descriptionThere are a great many calls to base::debug::DumpWithoutCrashing(). https://cs.chromium.org/search/?q=DumpWithoutCrashing+package:%5Echromium$&p=1&type=cs I don't think most of them are a good idea. If it's important enough to hang the process for a few seconds while dumping, it's important enough to add a CHECK, temporarily. Many of the crbug tags associated with these calls are old unmonitored/uncommented bugs which makes me relatively certain there's no one looking at the associated crash dumps either. At best they're useless dead code. At worst we are hitting them occasionally causing hangs for no good reason, and possibly throttling legitimate crashes that follow them due to crash upload throttling. Other possible proposals include: - replacing DumpWithoutCrashing() with DumpWithoutCrashingOnDevAndCanaryOnly() - adding a presubmit that requests a review from someone on stability team for new calls - making it a noop on certain channels But auditing and removing is probably the first step.
,
Feb 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8d2142efacf01639e95d5d26b789f846ac62c0ad commit 8d2142efacf01639e95d5d26b789f846ac62c0ad Author: scottmg <scottmg@chromium.org> Date: Wed Feb 22 16:00:39 2017 Remove DumpWithoutCrashing() from extension_prefs Bug appears to have been resolved, trying rationalize all calls to DumpWithoutCrashing() and remove any unnecessary ones. R=rdevlin.cronin@chromium.org BUG=616149 ,694688 Review-Url: https://codereview.chromium.org/2703393004 Cr-Commit-Position: refs/heads/master@{#452069} [modify] https://crrev.com/8d2142efacf01639e95d5d26b789f846ac62c0ad/chrome/app/chrome_crash_reporter_client_win.cc [modify] https://crrev.com/8d2142efacf01639e95d5d26b789f846ac62c0ad/chrome/common/crash_keys.cc [modify] https://crrev.com/8d2142efacf01639e95d5d26b789f846ac62c0ad/chromecast/crash/cast_crash_keys.cc [modify] https://crrev.com/8d2142efacf01639e95d5d26b789f846ac62c0ad/extensions/browser/extension_prefs.cc
,
Feb 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0f78a6b4ca7b46cc6047ecea9d09987ed0a16e70 commit 0f78a6b4ca7b46cc6047ecea9d09987ed0a16e70 Author: scottmg <scottmg@chromium.org> Date: Wed Feb 22 20:04:37 2017 Remove DumpWithoutCrashing() from privet_url_fetcher Part of trying to rationalize all calls to DumpWithoutCrashing(). It looks like this one is no longer necessary as the related bug is marked fixed. BUG=513505,694688 Review-Url: https://codereview.chromium.org/2709633007 Cr-Commit-Position: refs/heads/master@{#452175} [modify] https://crrev.com/0f78a6b4ca7b46cc6047ecea9d09987ed0a16e70/chrome/browser/printing/cloud_print/privet_url_fetcher.cc
,
Feb 22 2017
,
Feb 22 2017
,
Feb 22 2017
,
Feb 22 2017
,
Feb 22 2017
,
Feb 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb4e0caa63778779d1fadfe2b31e418e6502f8c0 commit cb4e0caa63778779d1fadfe2b31e418e6502f8c0 Author: scottmg <scottmg@chromium.org> Date: Wed Feb 22 20:42:56 2017 Remove DumpWithoutCrashing() and keys from audio_manager Reports are no longer being gathered and there's nothing further we can learn at this point. Part of trying to rationalize all calls to DumpWithoutCrashing(). R=dalecurtis@chromium.org BUG= 422522 , 694688 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2705293003 Cr-Commit-Position: refs/heads/master@{#452188} [modify] https://crrev.com/cb4e0caa63778779d1fadfe2b31e418e6502f8c0/chrome/app/chrome_crash_reporter_client_win.cc [modify] https://crrev.com/cb4e0caa63778779d1fadfe2b31e418e6502f8c0/chrome/common/crash_keys.cc [modify] https://crrev.com/cb4e0caa63778779d1fadfe2b31e418e6502f8c0/media/audio/audio_manager.cc [modify] https://crrev.com/cb4e0caa63778779d1fadfe2b31e418e6502f8c0/media/audio/audio_manager.h
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/669ebe761301d962d5c0540bb7d8e2cfe836a81e commit 669ebe761301d962d5c0540bb7d8e2cfe836a81e Author: scottmg <scottmg@chromium.org> Date: Thu Feb 23 00:36:19 2017 Remove DumpWithoutCrashing() from RFHI::SetNavigationHandle Semi-related bug is marked fixed. Part of trying to rationalize all calls to DumpWithoutCrashing(). R=clamy@chromium.org BUG= 621856 ,694688 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2708043003 Cr-Commit-Position: refs/heads/master@{#452309} [modify] https://crrev.com/669ebe761301d962d5c0540bb7d8e2cfe836a81e/content/browser/frame_host/render_frame_host_impl.cc
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bdfe69d301b8826665a213a553647e26f3b198c1 commit bdfe69d301b8826665a213a553647e26f3b198c1 Author: scottmg <scottmg@chromium.org> Date: Thu Feb 23 23:53:26 2017 Remove DumpWithoutCrashing() from sync/engine/commit_util This looks to be out of date code (related bug marked Fixed in 2015). Part of trying to rationalize all calls to DumpWithoutCrashing() as they can obscure real crashes due to upload throttling. R=stanisc@chromium.org BUG= 332371 , 694688 Review-Url: https://codereview.chromium.org/2712013002 Cr-Commit-Position: refs/heads/master@{#452678} [modify] https://crrev.com/bdfe69d301b8826665a213a553647e26f3b198c1/components/sync/engine_impl/commit_util.cc
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02612b1e0615618c3350f29f7f54fc836bd25e64 commit 02612b1e0615618c3350f29f7f54fc836bd25e64 Author: scottmg <scottmg@chromium.org> Date: Fri Feb 24 18:09:28 2017 Remove DumpWithoutCrashing() from broker_process_dispatcher There seems to be very few instances of these dumps on the crash server, but if no one is looking at them, I'd like to remove calls to it. DumpWithoutCrashing() can hide legitimate crashes due to crash throttling. R=yzshen@chromium.org BUG= 610600 ,694688 Review-Url: https://codereview.chromium.org/2717473003 Cr-Commit-Position: refs/heads/master@{#452871} [modify] https://crrev.com/02612b1e0615618c3350f29f7f54fc836bd25e64/content/ppapi_plugin/broker_process_dispatcher.cc
,
Feb 24 2017
,
Feb 25 2017
scottmg: You should be able to revert https://crrev.com/ff618a8c4b0c79d87f0f367e76c828696ae950e3 to cleanly remove everything related to bug 519022, which helps unblock your work here.
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9efb69199efad988c5088925d9c992ceeb92de59 commit 9efb69199efad988c5088925d9c992ceeb92de59 Author: scottmg <scottmg@chromium.org> Date: Mon Feb 27 19:36:22 2017 Remove DumpWithoutCrashing() from navigation_handle_impl More tidy up of DumpWithoutCrashing() calls. Per https://goto.google.com/uoflm, this doesn't seem to have been hit post M56, so let me know if you'd like to remove the whole `if` instead. R=clamy@chromium.org BUG=653344,694688 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2714823002 Cr-Commit-Position: refs/heads/master@{#453298} [modify] https://crrev.com/9efb69199efad988c5088925d9c992ceeb92de59/content/browser/frame_host/navigation_handle_impl.cc
,
Feb 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0cade2e02fe208771c5e2da0f5611be37987e131 commit 0cade2e02fe208771c5e2da0f5611be37987e131 Author: creis <creis@chromium.org> Date: Tue Feb 28 06:37:47 2017 Remove DumpWithoutCrashing calls for empty PageState. There were no reports from these cases, so they aren't on the right track. This effectively reverts https://crrev.com/408521. BUG= 568703 ,694688 TEST=No behavior change. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2713743002 Cr-Commit-Position: refs/heads/master@{#453527} [modify] https://crrev.com/0cade2e02fe208771c5e2da0f5611be37987e131/content/browser/frame_host/navigation_controller_impl.cc [modify] https://crrev.com/0cade2e02fe208771c5e2da0f5611be37987e131/content/browser/frame_host/navigation_entry_impl.cc [modify] https://crrev.com/0cade2e02fe208771c5e2da0f5611be37987e131/content/browser/web_contents/web_contents_impl.cc
,
Feb 28 2017
,
May 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0f27852ca4088b309083e746c3eaab29fb0b4415 commit 0f27852ca4088b309083e746c3eaab29fb0b4415 Author: yzshen <yzshen@chromium.org> Date: Mon May 01 17:10:22 2017 Network service: fix perf issue caused by unnecessary DumpWithoutCrashing. And also add a trace event that matches the non-network service path. BUG=598073,694688 Review-Url: https://codereview.chromium.org/2846393002 Cr-Commit-Position: refs/heads/master@{#468337} [modify] https://crrev.com/0f27852ca4088b309083e746c3eaab29fb0b4415/content/browser/loader/navigation_url_loader_network_service.cc [modify] https://crrev.com/0f27852ca4088b309083e746c3eaab29fb0b4415/content/child/web_url_loader_impl.cc
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/086d9f85f043be0c711d2b493ed7317c36b2accc commit 086d9f85f043be0c711d2b493ed7317c36b2accc Author: Yuri Wiitala <miu@chromium.org> Date: Thu Nov 09 19:55:14 2017 Clean-up: Revert 'zero-encode-details' crash key. Looks like the "dump without crashing" has never triggered. Thus, the issue seems to be long-gone. Bug: 519022, 694688 Change-Id: I253efa3438930d708c6c1e1f9554b532ecbdc9b9 Reviewed-on: https://chromium-review.googlesource.com/760017 Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: Yuri Wiitala <miu@chromium.org> Cr-Commit-Position: refs/heads/master@{#515251} [modify] https://crrev.com/086d9f85f043be0c711d2b493ed7317c36b2accc/chrome/common/crash_keys.cc [modify] https://crrev.com/086d9f85f043be0c711d2b493ed7317c36b2accc/chrome/common/crash_keys.h [modify] https://crrev.com/086d9f85f043be0c711d2b493ed7317c36b2accc/media/cast/net/cast_transport_impl.cc [modify] https://crrev.com/086d9f85f043be0c711d2b493ed7317c36b2accc/media/cast/sender/external_video_encoder.cc [modify] https://crrev.com/086d9f85f043be0c711d2b493ed7317c36b2accc/media/cast/sender/vp8_encoder.cc [modify] https://crrev.com/086d9f85f043be0c711d2b493ed7317c36b2accc/media/cast/sender/vp8_encoder.h
,
Nov 12
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by w...@chromium.org
, Feb 22 2017