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

chrome/browser/icon_loader_win.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/icon_loader.h"
 
 #include <windows.h>
 #include <shellapi.h>
 
 #include "base/message_loop.h"
@@ skipping 13 matching lines @@
     case IconLoader::LARGE:
       size = SHGFI_LARGEICON;
       break;
     default:
       NOTREACHED();
   }
   SHFILEINFO file_info = { 0 };
   if (!SHGetFileInfo(group_.c_str(), FILE_ATTRIBUTE_NORMAL, &file_info,
                      sizeof(SHFILEINFO),
                      SHGFI_ICON | size | SHGFI_USEFILEATTRIBUTES))
     return;

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

Should this also reset |image_| and notify?

 
   ICONINFO icon_info = { 0 };
   BITMAP bitmap_info = { 0 };
 
   BOOL r = ::GetIconInfo(file_info.hIcon, &icon_info);

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

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.

   DCHECK(r);
   r = ::GetObject(icon_info.hbmMask, sizeof(bitmap_info), &bitmap_info);
   DCHECK(r);
 
   gfx::Size icon_size(bitmap_info.bmWidth, bitmap_info.bmHeight);
-  image_.reset(new gfx::Image(
-      IconUtil::CreateSkBitmapFromHICON(file_info.hIcon, icon_size)));
+
+  const SkBitmap* bitmap =
+      IconUtil::CreateSkBitmapFromHICON(file_info.hIcon, icon_size);
+  if (bitmap) {
+    image_.reset(new gfx::Image(bitmap));
+  } else {
+    LOG(ERROR) << "Decoding HICON for group " << group_ << " failed";

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

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.

+    image_.reset();
+  }
 
   target_message_loop_->PostTask(FROM_HERE,
       NewRunnableMethod(this, &IconLoader::NotifyDelegate));
 }