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..8188fc305663910cc8a6dfa8a13354e9cd617163 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;
 using content::NavigationController;
 
 namespace {
@@ -43,7 +47,10 @@ 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 kInstallCanceledError[] = "Install canceled";
+const char kDownloadInterruptedError[] = "Download interrupted";
+const char kInvalidDownloadError[] = "Download was not a CRX";
 const char kInlineInstallSource[] = "inline";
 const char kDefaultInstallSource[] = "";
 
@@ -120,17 +127,22 @@ WebstoreInstaller::WebstoreInstaller(Profile* profile,
       delegate_(delegate),
       controller_(controller),
       id_(id),
+      download_item_(NULL),
       flags_(flags) {
   download_url_ = GetWebstoreInstallURL(id, flags & FLAG_INLINE_INSTALL ?
       kInlineInstallSource : kDefaultInstallSource);
 
+  registrar_.Add(this, chrome::NOTIFICATION_CRX_INSTALLER_DONE,
+                 content::NotificationService::AllSources());
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
                  content::Source<Profile>(profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
                  content::Source<CrxInstaller>(NULL));
 }
 
-WebstoreInstaller::~WebstoreInstaller() {}
+WebstoreInstaller::~WebstoreInstaller() {
+  RemoveObservers();
+}
 
 void WebstoreInstaller::Start() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -155,6 +167,16 @@ void WebstoreInstaller::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
   switch (type) {
+    case chrome::NOTIFICATION_CRX_INSTALLER_DONE: {
+      const Extension* extension =
+          content::Details<const Extension>(details).ptr();
+      const CrxInstaller* installer =
+          content::Source<CrxInstaller>(source).ptr();
+      if (extension == NULL && installer->expected_id() == id_)

[Mihai Parparita -not on Chrome, 2012/03/26 18:46:49]

You may also want to compare installer->profile() and profile_, to make sure
that you're getting the notification from the install that you started (in
theory the same extension could be installed at nearly the same time in multiple
profiles).

[jstritar, 2012/03/26 19:53:36]

Done. Also switched this to compare the CrxInstaller's download URL to the
DownloadItem's URL, since CrxInstaller::expected_id() is generally not set.

+        ReportFailure(kInstallCanceledError);
+      break;
+    }
+
     case chrome::NOTIFICATION_EXTENSION_INSTALLED: {
       CHECK(profile_->IsSameProfile(content::Source<Profile>(source).ptr()));
       const Extension* extension =
@@ -188,6 +210,46 @@ 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;
+  }
+
+  CHECK(id.IsValid());
+
+  content::DownloadManager* download_manager = profile_->GetDownloadManager();
+  download_item_ = download_manager->GetActiveDownloadItem(id.local());
+  download_item_->AddObserver(this);
+}
+
+void WebstoreInstaller::OnDownloadUpdated(DownloadItem* download) {
+  CHECK_EQ(download_item_, download);
+
+  switch (download->GetState()) {
+    case DownloadItem::CANCELLED:
+      ReportFailure(kDownloadCanceledError);
+      break;
+    case DownloadItem::INTERRUPTED:
+      ReportFailure(kDownloadInterruptedError);
+      break;
+    case DownloadItem::REMOVING:

[Mihai Parparita -not on Chrome, 2012/03/26 18:46:49]

Based on what Randy said, it seems cleaner/safer to null out the download_item_
pointer when you get the REMOVING state, and only then. Otherwise, you're
relying on catching all of the other error states, and if a new error enum value
is added, you won't be clearing the download_item_ pointer.

If in the REMOVING state, you call download_item_->RemoveObserver, and null out
the pointer, then you can get rid of RemoveObservers altogether.

[jstritar, 2012/03/26 19:53:36]

Done.  I also removed the observer in the destructor since WebstoreInstaller can
complete before the DownloadItem goes into the REMOVING state.

+      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.
+      break;
+  }
+}
+
+void WebstoreInstaller::OnDownloadOpened(DownloadItem* download) {
+  CHECK_EQ(download_item_, download);
+}
+
 void WebstoreInstaller::StartDownload(const FilePath& file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -216,13 +278,14 @@ 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) {
   if (delegate_)
     delegate_->OnExtensionInstallFailure(id_, error);
 
+  RemoveObservers();

[Mihai Parparita -not on Chrome, 2012/03/26 18:46:49]

This may be obsolete because of the comment above, but this call shouldn't be
necessary, the Release() call below will result in this instance being deleted,
which invokes the destructor, which invokes RemoveObservers.

[jstritar, 2012/03/26 19:53:36]

Done. I also NULL out the delegat to make sure we don't call it twice if they
hold a reference (e.g. Release wouldn't destruct installer).

   Release();  // Balanced in Start().
 }
 
@@ -230,5 +293,14 @@ void WebstoreInstaller::ReportSuccess() {
   if (delegate_)
     delegate_->OnExtensionInstallSuccess(id_);
 
+  RemoveObservers();

[Mihai Parparita -not on Chrome, 2012/03/26 18:46:49]

Ditto.

[jstritar, 2012/03/26 19:53:36]

Done.

   Release();  // Balanced in Start().
 }
+
+void WebstoreInstaller::RemoveObservers() {
+  registrar_.RemoveAll();
+  if (download_item_) {
+    download_item_->RemoveObserver(this);
+    download_item_ = NULL;
+  }
+}