Improve WebstoreInstaller error handling.

chrome/browser/extensions/webstore_installer.cc

Index: chrome/browser/extensions/webstore_installer.cc
diff --git a/chrome/browser/extensions/webstore_installer.cc b/chrome/browser/extensions/webstore_installer.cc
index 0bafe1c1fd3ee2cbba1f852db8d61cef1c5182f6..7f96d22d74f747ecac57e8690e1b896cd5f3a62c 100644
--- a/chrome/browser/extensions/webstore_installer.cc
+++ b/chrome/browser/extensions/webstore_installer.cc
@@ -14,6 +14,7 @@
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/extensions/crx_installer.h"
@@ -31,11 +32,14 @@
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
 
 using content::BrowserThread;
+using content::DownloadId;
+using content::DownloadItem;

[Randy Smith (Not in Mondays), 2012/03/23 20:57:19]

I thought the style guide generally preferred actually qualifying the name at
point of use?  (Not a request to change if you feel strongly, just raising the
issue.)

[jstritar, 2012/03/23 21:52:53]

I think this is okay in .cc files, but we're not supposed to pull in whole
namespaces like:

using namespace content;

 using content::NavigationController;
 
 namespace {
@@ -43,7 +47,9 @@ namespace {
 const char kInvalidIdError[] = "Invalid id";
 const char kNoBrowserError[] = "No browser found";
 const char kDownloadDirectoryError[] = "Could not create download directory";
-
+const char kDownloadCanceledError[] = "Download canceled";
+const char kDownloadInterruptedError[] = "Download interrupted";
+const char kInvalidDownloadError[] = "Download was not a CRX";
 const char kInlineInstallSource[] = "inline";
 const char kDefaultInstallSource[] = "";
 
@@ -121,6 +127,10 @@ WebstoreInstaller::WebstoreInstaller(Profile* profile,
       controller_(controller),
       id_(id),
       flags_(flags) {
+  // The caller should have already prompted to accept the permissions.
+  CHECK(CrxInstaller::GetWhitelistEntry(id) ||
+        CrxInstaller::IsIdWhitelisted(id));
+
   download_url_ = GetWebstoreInstallURL(id, flags & FLAG_INLINE_INSTALL ?
       kInlineInstallSource : kDefaultInstallSource);
 
@@ -130,7 +140,17 @@ WebstoreInstaller::WebstoreInstaller(Profile* profile,
                  content::Source<CrxInstaller>(NULL));
 }
 
-WebstoreInstaller::~WebstoreInstaller() {}
+WebstoreInstaller::~WebstoreInstaller() {
+  // Stop observing the DownloadItem, if applicable.
+  if (!download_id_.IsValid())
+    return;
+
+  content::DownloadManager* download_manager = profile_->GetDownloadManager();
+  DownloadItem* item = download_manager->GetActiveDownloadItem(
+      download_id_.local());
+  if (item)
+      item->RemoveObserver(this);

[Randy Smith (Not in Mondays), 2012/03/23 20:57:19]

This scares me.  I'm not certain as to the lifetime of the WebstoreInstaller,
but if the download item completes but is not deleted before the
WebStoreInstaller destructor is called, I think the DownloadItem will still be
around but GetActiveDownloadItem() won't return it, so we won't remove ourselves
as an observer, so our observation interfaces may later be called (i.e.
use-after-free).  

The canonical model is to keep a copy of a pointer to the DownloadItem around,
and be sensitive with an update to the DownloadItem state to REMOVING, which'll
always occur before the DownloadItem is destroyed.

[jstritar, 2012/03/23 21:52:53]

Ah, I see. A GetDownloadItem method that returned the download regardless of
what state it's in would be useful. 

I updated the WebstoreInstaller to hold a pointer to the DownloadItem and use
that in the destructor. (We already remove the observer when we get these
notifications -- CANCELLED, INTERRUPTED, REMOVING, COMPLETE.)

[Randy Smith (Not in Mondays), 2012/03/23 22:18:29]

You are completely right.  Mea culpa, and I hope to fix that soon.

+}
 
 void WebstoreInstaller::Start() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -188,6 +208,50 @@ void WebstoreInstaller::SetDownloadDirectoryForTests(FilePath* directory) {
   g_download_directory_for_tests = directory;
 }
 
+void WebstoreInstaller::OnDownloadStarted(DownloadId id, net::Error error) {
+  if (error != net::OK) {
+    ReportFailure(net::ErrorToString(error));
+    return;
+  }
+
+  download_id_ = id;
+  CHECK(id.IsValid());
+
+  content::DownloadManager* download_manager = profile_->GetDownloadManager();
+  DownloadItem* item = download_manager->GetActiveDownloadItem(id.local());
+  if (item)

[Randy Smith (Not in Mondays), 2012/03/23 20:57:19]

YOu should be able to make this a DCHECK; the DI should be available from
GetActiveDownloadItem at this point.  (And if it's not, that's probably a
problem for you, so we should find out soon :-}.)

[jstritar, 2012/03/23 21:52:53]

Done. I removed the if but didn't add a CHECK since we'll crash on the next line
anyways.

+      item->AddObserver(this);
+}
+
+void WebstoreInstaller::OnDownloadUpdated(DownloadItem* download) {
+  CHECK_EQ(download_id_, download->GetGlobalId());
+
+  switch (download->GetState()) {
+    case DownloadItem::CANCELLED:
+      ReportFailure(kDownloadCanceledError);
+      break;
+    case DownloadItem::INTERRUPTED:
+      ReportFailure(kDownloadInterruptedError);
+      break;
+    case DownloadItem::REMOVING:
+      break;
+    case DownloadItem::COMPLETE:
+      // Wait for other notifications if the download is really an extension.
+      if (!ChromeDownloadManagerDelegate::IsExtensionDownload(download))
+        ReportFailure(kInvalidDownloadError);
+      break;
+    default:
+      // Continue listening if the download is not in one of the above states.
+      return;
+  }
+
+  download->RemoveObserver(this);
+}
+
+void WebstoreInstaller::OnDownloadOpened(DownloadItem* download) {
+  CHECK_EQ(download_id_, download->GetGlobalId());
+}
+
 void WebstoreInstaller::StartDownload(const FilePath& file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -216,7 +280,7 @@ void WebstoreInstaller::StartDownload(const FilePath& file) {
   profile_->GetDownloadManager()->DownloadUrl(
       download_url_, referrer, "",
       false, -1, save_info, controller_->GetWebContents(),
-      content::DownloadManager::OnStartedCallback());
+      base::Bind(&WebstoreInstaller::OnDownloadStarted, this));
 }
 
 void WebstoreInstaller::ReportFailure(const std::string& error) {