Merge 61634 - Fix DCHECK assertion failures in DownloadStatusUpdater.

They indicate real problems, including possible NULL
pointer dereferences.

BUG=57577
TEST=none

Review URL: http://codereview.chromium.org/3519005

TBR=phajdan.jr@chromium.org
Review URL: http://codereview.chromium.org/3524015

git-svn-id: svn://svn.chromium.org/chrome/branches/517/src@61650 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 22 insertions, 15 deletions

diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 7989011..73cf9f3 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -63,7 +63,7 @@ DownloadManager::DownloadManager(DownloadStatusUpdater* status_updater)
     : shutdown_needed_(false),
       profile_(NULL),
       file_manager_(NULL),
-      status_updater_(status_updater) {
+      status_updater_(status_updater->AsWeakPtr()) {
   if (status_updater_)
     status_updater_->AddDelegate(this);
 }
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 5cbec75..1517354 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -39,6 +39,7 @@
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "base/time.h"
+#include "base/weak_ptr.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/download/download_status_updater_delegate.h"
 #include "chrome/browser/shell_dialogs.h"
@@ -352,7 +353,7 @@ class DownloadManager
   DownloadFileManager* file_manager_;
 
   // Non-owning pointer for updating the download status.
-  DownloadStatusUpdater* status_updater_;
+  base::WeakPtr<DownloadStatusUpdater> status_updater_;
 
   // The user's last choice for download directory. This is only used when the
   // user wants us to prompt for a save location for each download.
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index e98925e..ca8e89f 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/download/download_status_updater.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/history/download_types.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -22,7 +23,8 @@
 
 class MockDownloadManager : public DownloadManager {
  public:
-  MockDownloadManager() : DownloadManager(NULL) {
+  explicit MockDownloadManager(DownloadStatusUpdater* updater)
+      : DownloadManager(updater) {
   }
 
   // Override some functions.
@@ -34,7 +36,7 @@ class DownloadManagerTest : public testing::Test {
  public:
   DownloadManagerTest()
       : profile_(new TestingProfile()),
-        download_manager_(new MockDownloadManager()),
+        download_manager_(new MockDownloadManager(&download_status_updater_)),
         ui_thread_(ChromeThread::UI, &message_loop_) {
     download_manager_->Init(profile_.get());
   }
@@ -52,6 +54,7 @@ class DownloadManagerTest : public testing::Test {
   }
 
  protected:
+  DownloadStatusUpdater download_status_updater_;
   scoped_ptr<TestingProfile> profile_;
   scoped_refptr<DownloadManager> download_manager_;
   scoped_refptr<DownloadFileManager> file_manager_;
diff --git a/chrome/browser/download/download_status_updater.cc b/chrome/browser/download/download_status_updater.cc
index 6d6b565..35b7adf 100644
--- a/chrome/browser/download/download_status_updater.cc
+++ b/chrome/browser/download/download_status_updater.cc
@@ -12,7 +12,6 @@ DownloadStatusUpdater::DownloadStatusUpdater() {
 }
 
 DownloadStatusUpdater::~DownloadStatusUpdater() {
-  DCHECK(delegates_.empty());
 }
 
 void DownloadStatusUpdater::AddDelegate(
@@ -37,19 +36,16 @@ void DownloadStatusUpdater::Update() {
 bool DownloadStatusUpdater::GetProgress(float* progress) {
   *progress = 0;
 
-  bool progress_known = true;
   int64 received_bytes = 0;
   int64 total_bytes = 0;
   for (DelegateSet::iterator i = delegates_.begin();
        i != delegates_.end(); ++i) {
-    progress_known = progress_known && (*i)->IsDownloadProgressKnown();
+    if (!(*i)->IsDownloadProgressKnown())
+      return false;
     received_bytes += (*i)->GetReceivedDownloadBytes();
     total_bytes += (*i)->GetTotalDownloadBytes();
   }
 
-  if (!progress_known)
-    return false;
-
   if (total_bytes > 0)
     *progress = static_cast<float>(received_bytes) / total_bytes;
   return true;
diff --git a/chrome/browser/download/download_status_updater.h b/chrome/browser/download/download_status_updater.h
index cdfd5b8..c513bc3 100644
--- a/chrome/browser/download/download_status_updater.h
+++ b/chrome/browser/download/download_status_updater.h
@@ -9,11 +9,13 @@
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
+#include "base/weak_ptr.h"
 
 class DownloadStatusUpdaterDelegate;
 
 // Keeps track of download progress for the entire browser.
-class DownloadStatusUpdater {
+class DownloadStatusUpdater
+    : public base::SupportsWeakPtr<DownloadStatusUpdater> {
  public:
   DownloadStatusUpdater();
   ~DownloadStatusUpdater();
diff --git a/chrome/browser/download/download_status_updater_unittest.cc b/chrome/browser/download/download_status_updater_unittest.cc
index c74c0bf..2a37505 100644
--- a/chrome/browser/download/download_status_updater_unittest.cc
+++ b/chrome/browser/download/download_status_updater_unittest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/scoped_ptr.h"
+#include "base/weak_ptr.h"
 #include "chrome/browser/download/download_status_updater.h"
 #include "chrome/browser/download/download_status_updater_delegate.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -12,16 +13,20 @@ namespace {
 class MockDelegate : public DownloadStatusUpdaterDelegate {
  public:
   explicit MockDelegate(DownloadStatusUpdater* updater)
-      : updater_(updater),
+      : updater_(updater->AsWeakPtr()),
         is_download_progress_known_(true),
         in_progress_download_count_(0),
         received_bytes_(0),
         total_bytes_(0) {
-    updater_->AddDelegate(this);
+    EXPECT_TRUE(updater);
+    if (updater_)
+      updater_->AddDelegate(this);
   }
 
   ~MockDelegate() {
-    updater_->RemoveDelegate(this);
+    EXPECT_TRUE(updater_);
+    if (updater_)
+      updater_->RemoveDelegate(this);
   }
 
   // Overriden from DownloadStatusUpdaterDelegate:
@@ -58,7 +63,7 @@ class MockDelegate : public DownloadStatusUpdaterDelegate {
   }
 
  private:
-  DownloadStatusUpdater* updater_;
+  base::WeakPtr<DownloadStatusUpdater> updater_;
 
   bool is_download_progress_known_;
   int64 in_progress_download_count_;