Re-introduction of release mode checks to determine the cause of CreateDIBSection failures.

Re-introduction of release mode checks to determine the cause of CreateDIBSection failures.

The crash is happening outside of the expected conditions in which CreateDIBSection would fail (large bitmap size, GDI and virtual memory pressure).  To track the cause, this CL attempts to capture the last system error on bitmap allocation failure.

See previous checks here: http://codereview.chromium.org/8341090

BUG=101934
TEST=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109461

[Jeff Timanus, 2011-11-10 00:45:09]

Hi Guys, 

Please take a look.  Did you already consider this approach when previously
trying to track down the cause of the allocation failures?

As an aside, the last error reported when opening the crash dump indicated in
the bug is NO_ERROR, so this approach may not work at all.  It is possible that
the dump packaging code in the crash handler is re-setting the last error, which
is what is reported in the dump.

Is it worth trying this in the canary, and seeing what error values we get?

Jeff

[vandebo (ex-Chrome), 2011-11-10 00:52:13]

On 2011/11/10 00:45:09, Jeff Timanus wrote:
> Hi Guys, 
> 
> Please take a look.  Did you already consider this approach when previously
> trying to track down the cause of the allocation failures?
> 
> As an aside, the last error reported when opening the crash dump indicated in
> the bug is NO_ERROR, so this approach may not work at all.  It is possible
that
> the dump packaging code in the crash handler is re-setting the last error,
which
> is what is reported in the dump.
> 
> Is it worth trying this in the canary, and seeing what error values we get?
> 
> Jeff

Certainly worth a try.  LGTM with one suggestion.

http://codereview.chromium.org/8509030/diff/4/skia/ext/bitmap_platform_device...
File skia/ext/bitmap_platform_device_win.cc (right):

http://codereview.chromium.org/8509030/diff/4/skia/ext/bitmap_platform_device...
skia/ext/bitmap_platform_device_win.cc:48: DWORD last_error = GetLastError();
Wouldn't it be better to call this at the start of this function?

[groby-ooo-7-16, 2011-11-10 01:18:43]

Tried that (except for GetLastEror/FormatErrorMessage in
http://codereview.chromium.org/8341090/

Be aware that putting that in will gain you an immediate spot as TopCrasher in
Canary - there are lots of other paths that  call BitmapPlatformDevice::create
and handle NULL returns. (I believe via plugin paths. See crash reports for
17.0.928.0)

On 2011/11/10 00:52:13, vandebo wrote:
> On 2011/11/10 00:45:09, Jeff Timanus wrote:
> > Hi Guys, 
> > 
> > Please take a look.  Did you already consider this approach when previously
> > trying to track down the cause of the allocation failures?
> > 
> > As an aside, the last error reported when opening the crash dump indicated
in
> > the bug is NO_ERROR, so this approach may not work at all.  It is possible
> that
> > the dump packaging code in the crash handler is re-setting the last error,
> which
> > is what is reported in the dump.
> > 
> > Is it worth trying this in the canary, and seeing what error values we get?
> > 
> > Jeff
> 
> Certainly worth a try.  LGTM with one suggestion.
> 
>
http://codereview.chromium.org/8509030/diff/4/skia/ext/bitmap_platform_device...
> File skia/ext/bitmap_platform_device_win.cc (right):
> 
>
http://codereview.chromium.org/8509030/diff/4/skia/ext/bitmap_platform_device...
> skia/ext/bitmap_platform_device_win.cc:48: DWORD last_error = GetLastError();
> Wouldn't it be better to call this at the start of this function?

[vandebo (ex-Chrome), 2011-11-10 01:21:46]

On 2011/11/10 01:18:43, groby wrote:
> Tried that (except for GetLastEror/FormatErrorMessage in
> http://codereview.chromium.org/8341090/

Note that this change only crashes if last error != NO_ERROR, so there may not
be as much collateral damage.

[Jeff Timanus, 2011-11-10 15:14:46]

On 2011/11/10 00:52:13, vandebo wrote:
> On 2011/11/10 00:45:09, Jeff Timanus wrote:
> > Hi Guys, 
> > 
> > Please take a look.  Did you already consider this approach when previously
> > trying to track down the cause of the allocation failures?
> > 
> > As an aside, the last error reported when opening the crash dump indicated
in
> > the bug is NO_ERROR, so this approach may not work at all.  It is possible
> that
> > the dump packaging code in the crash handler is re-setting the last error,
> which
> > is what is reported in the dump.
> > 
> > Is it worth trying this in the canary, and seeing what error values we get?
> > 
> > Jeff
> 
> Certainly worth a try.  LGTM with one suggestion.
> 
>
http://codereview.chromium.org/8509030/diff/4/skia/ext/bitmap_platform_device...
> File skia/ext/bitmap_platform_device_win.cc (right):
> 
>
http://codereview.chromium.org/8509030/diff/4/skia/ext/bitmap_platform_device...
> skia/ext/bitmap_platform_device_win.cc:48: DWORD last_error = GetLastError();
> Wouldn't it be better to call this at the start of this function?

Yes, I think it's necessary to get the last error at the start of the function. 
The reason that the crash dumps report no error is potentially because the gdi
and memory space query functions are resetting the error.

Done.

[commit-bot: I haz the power, 2011-11-10 17:21:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/8509030/6001

[commit-bot: I haz the power, 2011-11-10 18:31:21]

Change committed as 109461