Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2452933002:
Collect information on the dump when GetDC fails. (Closed)
DescriptionCollect information on the dump when GetDC fails.
We already have the code that collects GDI usage information on the
dump, so we can reuse it to handle any unrecoverable GDI failures.
BUG=661461
Committed: https://crrev.com/80e1d0d45fe77ad8ad61a9bff41e08a8bd147f9a
Cr-Commit-Position: refs/heads/master@{#429248}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Provide only one function declaration #
Total comments: 1
Patch Set 3 : Remove excessive check for |header|. #
Messages
Total messages: 30 (12 generated)
anpol@yandex-team.ru changed reviewers: + pkasting@chromium.org, thestig@chromium.org
thestig@chromium.org changed reviewers: + siggi@chromium.org
+siggi
LGTM https://codereview.chromium.org/2452933002/diff/1/base/debug/gdi_debug_util_w... File base/debug/gdi_debug_util_win.h (right): https://codereview.chromium.org/2452933002/diff/1/base/debug/gdi_debug_util_w... base/debug/gdi_debug_util_win.h:16: // |base::debug::Alias| so we can find what is causing the GDI failures. Nit: No || on classnames (just variable names) https://codereview.chromium.org/2452933002/diff/1/base/debug/gdi_debug_util_w... base/debug/gdi_debug_util_win.h:22: HANDLE shared_section); Nit: I suggest just one function declaration, to make it clearer these really do the same thing, and simplify your implementation: // Crashes the process, using base::debug::Alias to leave valuable debugging // information in the crash dump. Pass values for |header| and |shared_section| // in the event of a bitmap allocation failure, to gather information about // those as well. void BASE_EXPORT CollectGDIUsageAndDie(BITMAPINFOHEADER* header = nullptr, HANDLE shared_section = nullptr);
Maybe worth calling base::win::IsUser32AndGdi32Available and recording that in a base::debug::Alias - since these DC calls will fail "by design" if the code is executing under win32k lockdown (which is all renderers, ppapi code)
Message was sent while issue was closed.
All comments have been addressed.
Message was sent while issue was closed.
Description was changed from ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG= ========== to ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG= ==========
PTAL
Still LGTM https://codereview.chromium.org/2452933002/diff/20001/base/debug/gdi_debug_ut... File base/debug/gdi_debug_util_win.cc (right): https://codereview.chromium.org/2452933002/diff/20001/base/debug/gdi_debug_ut... base/debug/gdi_debug_util_win.cc:109: if (header && std::abs(height) * width > 100) { Nit: No need for |header| check now; if it fails, the width/height will also be zero.
is there a BUG= id for this, and did you consider the suggestion in #6?
On 2016/11/01 17:09:35, Will Harris wrote: > is there a BUG= id for this, and did you consider the suggestion in #6? It's implemented.
ah doh. I skimmed the latest CL but missed the additional line. great!
lgtm
Description was changed from ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG= ========== to ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG=661461 ==========
The CQ bit was checked by anpol@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2452933002/#ps40001 (title: "Remove excessive check for |header|.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
anpol@yandex-team.ru changed reviewers: + tomhudson@chromium.org
Tom Hudson, please, review skia/ext/skia_utils_win.cc
tomhudson@google.com changed reviewers: + tomhudson@google.com
lgtm
The CQ bit was checked by tomhudson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG=661461 ========== to ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG=661461 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG=661461 ========== to ========== Collect information on the dump when GetDC fails. We already have the code that collects GDI usage information on the dump, so we can reuse it to handle any unrecoverable GDI failures. BUG=661461 Committed: https://crrev.com/80e1d0d45fe77ad8ad61a9bff41e08a8bd147f9a Cr-Commit-Position: refs/heads/master@{#429248} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/80e1d0d45fe77ad8ad61a9bff41e08a8bd147f9a Cr-Commit-Position: refs/heads/master@{#429248} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
