Fix crash in the icon manager. The decoding could be NULL, which would then get

dereferenced in specific cases. We would have to create the failed icon twice,
so that it would think the icon was already in the cache, when in fact a NULL
value was being cached.

BUG=1259695
Review URL: http://codereview.chromium.org/9672

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@4988 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 12 insertions, 2 deletions

diff --git a/chrome/browser/icon_manager.cc b/chrome/browser/icon_manager.cc
index 8107877..59632e2 100644
--- a/chrome/browser/icon_manager.cc
+++ b/chrome/browser/icon_manager.cc
@@ -70,9 +70,11 @@ bool IconManager::OnSkBitmapLoaded(IconLoader* source, SkBitmap* result) {
     return false;  // Return false to indicate result should be deleted.
   }
 
+  // Cache the bitmap. Watch out: |result| or the cached bitmap may be NULL to
+  // indicate a current or past failure.
   CacheKey key(client_request.file_name, client_request.size);
   IconMap::iterator it = icon_cache_.find(key);
-  if (it != icon_cache_.end()) {
+  if (it != icon_cache_.end() && result && it->second) {
     it->second->swap(*result);
     delete result;
     result = it->second;
diff --git a/chrome/browser/icon_manager.h b/chrome/browser/icon_manager.h
index ad6a4eb..6840ac1 100644
--- a/chrome/browser/icon_manager.h
+++ b/chrome/browser/icon_manager.h
@@ -70,6 +70,8 @@ public:
   // work is done on the file thread, with the callbacks running on the UI
   // thread. The return value is the 'request_id' that will be passed to the
   // client in the callback.
+  //
+  // WATCH OUT: The returned bitmap pointer may be NULL if decoding failed.
   typedef CancelableRequestProvider::Handle Handle;
   typedef Callback2<Handle, SkBitmap*>::Type IconRequestCallback;
 
diff --git a/chrome/browser/views/download_item_view.cc b/chrome/browser/views/download_item_view.cc
index 60a13f5..ab21fe2 100644
--- a/chrome/browser/views/download_item_view.cc
+++ b/chrome/browser/views/download_item_view.cc
@@ -489,6 +489,11 @@ void DownloadItemView::Paint(ChromeCanvas* canvas) {
   SkBitmap* icon = IsDangerousMode() ? warning_icon_ :
       im->LookupIcon(download_->full_path(), IconLoader::SMALL);
 
+  // We count on the fact that the icon manager will cache the icons and if one
+  // is available, it will be cached here. We *don't* want to request the icon
+  // to be loaded here, since this will also get called if the icon can't be
+  // loaded, in which case LookupIcon will always be NULL. The loading will be
+  // triggered only when we think the status might change.
   if (icon) {
     if (!IsDangerousMode()) {
       if (download_->state() == DownloadItem::IN_PROGRESS) {
@@ -739,7 +744,8 @@ void DownloadItemView::OpenDownload() {
 
 void DownloadItemView::OnExtractIconComplete(IconManager::Handle handle,
                                              SkBitmap* icon_bitmap) {
-  GetParent()->SchedulePaint();
+  if (icon_bitmap)
+    GetParent()->SchedulePaint();
 }
 
 void DownloadItemView::LoadIcon() {