In icon_loader_win.cc, if CreateSkBitmapFromHICON fails, don't pass NULL to gfx::Image and crash.

In icon_loader_win.cc, if CreateSkBitmapFromHICON fails, don't pass NULL to gfx::Image and crash.

BUG=92982
TEST=none


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

[Robert Sesek, 2011-08-26 18:09:39]

Everyone who has touched this file in the past has left the project. I think you
maybe reviewed changes here at one point?

[Peter Kasting, 2011-08-26 18:12:56]

Can we add a test for this by constructing a corrupt icon file?

http://codereview.chromium.org/7753009/diff/1/chrome/browser/icon_loader_win.cc
File chrome/browser/icon_loader_win.cc (right):

http://codereview.chromium.org/7753009/diff/1/chrome/browser/icon_loader_win....
chrome/browser/icon_loader_win.cc:50: // Don't create a gfx::Image and crash.
The delegate won't be notified.
Should we instead set |image_| to a non-NULL but zero-size or blank image and
notify the delegate anyway?  Arguably that's more consistent behavior.

[Robert Sesek, 2011-08-26 19:38:28]

On 2011/08/26 18:12:56, Peter Kasting wrote:
> Can we add a test for this by constructing a corrupt icon file?

If you can provide a corrupt icon file, I think I can try. I don't have access
to a Windows box, though.

http://codereview.chromium.org/7753009/diff/1/chrome/browser/icon_loader_win.cc
File chrome/browser/icon_loader_win.cc (right):

http://codereview.chromium.org/7753009/diff/1/chrome/browser/icon_loader_win....
chrome/browser/icon_loader_win.cc:50: // Don't create a gfx::Image and crash.
The delegate won't be notified.
On 2011/08/26 18:12:56, Peter Kasting wrote:
> Should we instead set |image_| to a non-NULL but zero-size or blank image and
> notify the delegate anyway?  Arguably that's more consistent behavior.

No, we should actually notify with a NULL pointer, as described in the header.
Changed to do just that.

[Peter Kasting, 2011-08-26 19:52:56]

On 2011/08/26 19:38:28, rsesek wrote:
> On 2011/08/26 18:12:56, Peter Kasting wrote:
> > Can we add a test for this by constructing a corrupt icon file?
> 
> If you can provide a corrupt icon file, I think I can try.

We probably want to test several cases: a zero-length source file, a non-icon
file (e.g. "Hello, world" renamed from .txt to .ico), and a .cur file, which is
like an .ico enough to make it through most of the checks here but not all.  I
think one of these may be found at
http://www.streetbiker.ru/forum/styles/abbcode/zoomin.cur .

http://codereview.chromium.org/7753009/diff/4001/chrome/browser/icon_loader_w...
File chrome/browser/icon_loader_win.cc (right):

http://codereview.chromium.org/7753009/diff/4001/chrome/browser/icon_loader_w...
chrome/browser/icon_loader_win.cc:34: return;
Should this also reset |image_| and notify?

http://codereview.chromium.org/7753009/diff/4001/chrome/browser/icon_loader_w...
chrome/browser/icon_loader_win.cc:39: BOOL r = ::GetIconInfo(file_info.hIcon,
&icon_info);
I think we should also look at, rather than DCHECK, these next two functions'
return values.  If the file is corrupt, they could reasonably fail.

http://codereview.chromium.org/7753009/diff/4001/chrome/browser/icon_loader_w...
chrome/browser/icon_loader_win.cc:51: LOG(ERROR) << "Decoding HICON for group "
<< group_ << " failed";
Nit: TBH, I'm not sure how much use this error log statement is.  Maybe
considering the comments above, the right thing here is to do "image_.reset();"
atop this function, and then do the reset() call above in some nested case where
we've passed all our conditions.

[Peter Kasting, 2012-06-13 19:27:41]

This seems to have died.  Robert, can you take another look?

[Peter Kasting, 2012-09-05 21:25:38]

On 2012/06/13 19:27:41, Peter Kasting wrote:
> This seems to have died.  Robert, can you take another look?

Ping

[Robert Sesek, 2012-09-05 22:11:22]

On 2012/09/05 21:25:38, Peter Kasting wrote:
> On 2012/06/13 19:27:41, Peter Kasting wrote:
> > This seems to have died.  Robert, can you take another look?
> 
> Ping

I took another look but didn't update this CL.

I tried your suggests for tests and Windows was able to create valid icons for
each. So I can fix the crash but not provide tests.

[Peter Kasting, 2012-09-05 22:13:58]

Crash fix w/o tests > no crash fix and no tests

[Robert Sesek, 2012-09-05 23:43:03]

PTAL

[Peter Kasting, 2012-09-06 00:07:42]

LGTM

[commit-bot: I haz the power, 2012-09-06 13:13:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/7753009/13003

[commit-bot: I haz the power, 2012-09-06 13:13:54]

Presubmit check for 7753009-13003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[Robert Sesek, 2012-09-06 15:26:45]

+thakis for the ONEROUS approval

[Nico, 2012-09-06 15:41:46]

lgtm

[commit-bot: I haz the power, 2012-09-06 15:42:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/7753009/13003

[commit-bot: I haz the power, 2012-09-06 18:02:36]

Try job failure for 7753009-13003 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-06 18:03:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/7753009/13003

[commit-bot: I haz the power, 2012-09-06 21:40:14]

Try job failure for 7753009-13003 (previous was lost) (retry) on win_rel for
step "sync_integration_tests".
It's a second try, previously, steps "sync_integration_tests,
interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-07 15:43:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/7753009/13003

[commit-bot: I haz the power, 2012-09-07 18:43:13]

Change committed as 155434