Harden NativeThemeWin::PaintIndirect()

Harden NativeThemeWin::PaintIndirect()

Speculative fix for crash observed in m54 beta.

Before https://codereview.chromium.org/2182083002, any sort of setup
error in PaintIndirect() would create an empty, unbacked SkCanvas;
we'd run the entire painting and pixel-copying code using it, turning
all the leaf operations into no-ops.

The previous change to PaintIndirect() used new APIs which are
pointer-based and would create NULL pointers in case of error;
although we didn't expect setup errors, we're seeing crashes that
suggest they are occurring in the field (possibly due to GDI handle
exhaustion or running out of memory).

This CL adds early exits if any of the preliminary setup steps has an
error. Calling SkSurface::getCanvas() is defined as *always* returning
some sort of canvas, so we leave it as a DCHECK(); the crash was
happening because we were halting initilization before having a valid
SkSurface to make that call against.

R=fmalita@chromium.org,pkasting@chromium.org
BUG=648790
Committed: https://crrev.com/7dcd512c8e7705ab266aa5593465afa4b78c4999
Cr-Commit-Position: refs/heads/master@{#420324}

[tomhudson, 2016-09-21 20:16:01]

Description was changed from

==========
Harden NativeThemeWin::PaintIndirect()

Speculative fix for crash observed in m54 beta.

Before https://codereview.chromium.org/2182083002, any sort of setup
error in PaintIndirect() would create an empty, unbacked SkCanvas;
we'd run the entire painting and pixel-copying code using it, turning
all the leaf operations into no-ops.

The previous change to PaintIndirect() used new APIs which are
pointer-based and would create NULL pointers in case of error.

This CL adds early exits if any of the preliminary setup steps has an
error. Calling SkSurface::getCanvas() is defined as *always* returning
some sort of canvas, so we leave it as a DCHECK(); the crash was
happening because we were halting initilization before having a valid
SkSurface to make that call against.

R=fmalita@chromium.org,pkasting@chromium.org
BUG=648790
==========

to

==========
Harden NativeThemeWin::PaintIndirect()

Speculative fix for crash observed in m54 beta.

Before https://codereview.chromium.org/2182083002, any sort of setup
error in PaintIndirect() would create an empty, unbacked SkCanvas;
we'd run the entire painting and pixel-copying code using it, turning
all the leaf operations into no-ops.

The previous change to PaintIndirect() used new APIs which are
pointer-based and would create NULL pointers in case of error;
although we didn't expect setup errors, we're seeing crashes that
suggest they are occurring in the field (possibly due to GDI handle
exhaustion or running out of memory).

This CL adds early exits if any of the preliminary setup steps has an
error. Calling SkSurface::getCanvas() is defined as *always* returning
some sort of canvas, so we leave it as a DCHECK(); the crash was
happening because we were halting initilization before having a valid
SkSurface to make that call against.

R=fmalita@chromium.org,pkasting@chromium.org
BUG=648790
==========

[tomhudson, 2016-09-21 20:46:26]

Florin, Peter, PTAL; I tried to sketch the history and the suspected etiology of
the bug in the CL description.

[Peter Kasting, 2016-09-21 20:50:44]

LGTM

https://codereview.chromium.org/2355423002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2355423002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:671: return;
Nit: It's probably worth a comment or two on these explaining what sort of thing
can cause failure, especially if those things are different (e.g. GDI handle
exhaustion for one but only OOM for the other) and/or if those things wouldn't
cause Chrome crashes anyway (e.g. mention why OOM would not have killed the
browser already, if that's something that could cause this).

[tomhudson, 2016-09-21 21:19:20]

https://codereview.chromium.org/2355423002/diff/20001/ui/native_theme/native_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2355423002/diff/20001/ui/native_theme/native_...
ui/native_theme/native_theme_win.cc:671: return;
On 2016/09/21 20:50:44, Peter Kasting wrote:
> Nit: It's probably worth a comment or two on these explaining what sort of
thing
> can cause failure.

Done, although we don't know as much as we'd like to. Since Microsoft doesn't
document much of what can cause the really low-level calls to fail, this mostly
boils down to "Will be null if lower-level Windows calls fail."

Somewhat frighteningly, even the first version of this code was expected to fail
if we didn't have access to GDI - presumably due to sandboxing.
(BitmapPlatformDevice::Create() could take a Windows shared section, but this
function always passed in NULL.)

[f(malita), 2016-09-22 12:56:04]

lgtm

[tomhudson, 2016-09-22 13:19:03]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2016-09-22 13:19:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2355423002/#ps40001
(title: "first pass at Peter's requested comments")

[commit-bot: I haz the power, 2016-09-22 13:19:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 14:04:17]

Description was changed from

==========
Harden NativeThemeWin::PaintIndirect()

Speculative fix for crash observed in m54 beta.

Before https://codereview.chromium.org/2182083002, any sort of setup
error in PaintIndirect() would create an empty, unbacked SkCanvas;
we'd run the entire painting and pixel-copying code using it, turning
all the leaf operations into no-ops.

The previous change to PaintIndirect() used new APIs which are
pointer-based and would create NULL pointers in case of error;
although we didn't expect setup errors, we're seeing crashes that
suggest they are occurring in the field (possibly due to GDI handle
exhaustion or running out of memory).

This CL adds early exits if any of the preliminary setup steps has an
error. Calling SkSurface::getCanvas() is defined as *always* returning
some sort of canvas, so we leave it as a DCHECK(); the crash was
happening because we were halting initilization before having a valid
SkSurface to make that call against.

R=fmalita@chromium.org,pkasting@chromium.org
BUG=648790
==========

to

==========
Harden NativeThemeWin::PaintIndirect()

Speculative fix for crash observed in m54 beta.

Before https://codereview.chromium.org/2182083002, any sort of setup
error in PaintIndirect() would create an empty, unbacked SkCanvas;
we'd run the entire painting and pixel-copying code using it, turning
all the leaf operations into no-ops.

The previous change to PaintIndirect() used new APIs which are
pointer-based and would create NULL pointers in case of error;
although we didn't expect setup errors, we're seeing crashes that
suggest they are occurring in the field (possibly due to GDI handle
exhaustion or running out of memory).

This CL adds early exits if any of the preliminary setup steps has an
error. Calling SkSurface::getCanvas() is defined as *always* returning
some sort of canvas, so we leave it as a DCHECK(); the crash was
happening because we were halting initilization before having a valid
SkSurface to make that call against.

R=fmalita@chromium.org,pkasting@chromium.org
BUG=648790
==========

[commit-bot: I haz the power, 2016-09-22 14:04:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-22 14:05:38]

Description was changed from

==========
Harden NativeThemeWin::PaintIndirect()

Speculative fix for crash observed in m54 beta.

Before https://codereview.chromium.org/2182083002, any sort of setup
error in PaintIndirect() would create an empty, unbacked SkCanvas;
we'd run the entire painting and pixel-copying code using it, turning
all the leaf operations into no-ops.

The previous change to PaintIndirect() used new APIs which are
pointer-based and would create NULL pointers in case of error;
although we didn't expect setup errors, we're seeing crashes that
suggest they are occurring in the field (possibly due to GDI handle
exhaustion or running out of memory).

This CL adds early exits if any of the preliminary setup steps has an
error. Calling SkSurface::getCanvas() is defined as *always* returning
some sort of canvas, so we leave it as a DCHECK(); the crash was
happening because we were halting initilization before having a valid
SkSurface to make that call against.

R=fmalita@chromium.org,pkasting@chromium.org
BUG=648790
==========

to

==========
Harden NativeThemeWin::PaintIndirect()

Speculative fix for crash observed in m54 beta.

Before https://codereview.chromium.org/2182083002, any sort of setup
error in PaintIndirect() would create an empty, unbacked SkCanvas;
we'd run the entire painting and pixel-copying code using it, turning
all the leaf operations into no-ops.

The previous change to PaintIndirect() used new APIs which are
pointer-based and would create NULL pointers in case of error;
although we didn't expect setup errors, we're seeing crashes that
suggest they are occurring in the field (possibly due to GDI handle
exhaustion or running out of memory).

This CL adds early exits if any of the preliminary setup steps has an
error. Calling SkSurface::getCanvas() is defined as *always* returning
some sort of canvas, so we leave it as a DCHECK(); the crash was
happening because we were halting initilization before having a valid
SkSurface to make that call against.

R=fmalita@chromium.org,pkasting@chromium.org
BUG=648790

Committed: https://crrev.com/7dcd512c8e7705ab266aa5593465afa4b78c4999
Cr-Commit-Position: refs/heads/master@{#420324}
==========

[commit-bot: I haz the power, 2016-09-22 14:05:39]

Patchset 3 (id:??) landed as
https://crrev.com/7dcd512c8e7705ab266aa5593465afa4b78c4999
Cr-Commit-Position: refs/heads/master@{#420324}