Improve WebstoreInstaller error handling.

chrome/browser/extensions/webstore_installer.cc

 // Copyright (c) 2012 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/extensions/webstore_installer.h"
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/rand_util.h"
 #include "base/stringprintf.h"
 #include "base/string_util.h"
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/download_save_info.h"
 #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 {
 
 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[] = "";
 
 FilePath* g_download_directory_for_tests = NULL;
 
 GURL GetWebstoreInstallURL(
     const std::string& extension_id, const std::string& install_source) {
   CommandLine* cmd_line = CommandLine::ForCurrentProcess();
   if (cmd_line->HasSwitch(switches::kAppsGalleryDownloadURL)) {
     std::string download_url =
@@ skipping 56 matching lines @@
 
 WebstoreInstaller::WebstoreInstaller(Profile* profile,
                                      Delegate* delegate,
                                      NavigationController* controller,
                                      const std::string& id,
                                      int flags)
     : 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));
   AddRef();  // Balanced in ReportSuccess and ReportFailure.
 
   if (!Extension::IdIsValid(id_)) {
     ReportFailure(kInvalidIdError);
     return;
   }
 
   FilePath download_path = DownloadPrefs::FromDownloadManager(
       profile_->GetDownloadManager())->download_path();
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&GetDownloadFilePath, download_path, id_,
                  base::Bind(&WebstoreInstaller::StartDownload,
                             base::Unretained(this))));
 
 }
 
 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 =
           content::Details<const Extension>(details).ptr();
       if (id_ == extension->id())
         ReportSuccess();
       break;
     }
 
     case chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR: {
@@ skipping 13 matching lines @@
 
     default:
       NOTREACHED();
   }
 }
 
 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));
 
   if (file.empty()) {
     ReportFailure(kDownloadDirectoryError);
     return;
   }
 
   // TODO(mihaip): For inline installs, we pretend like the referrer is the
   // gallery, even though this could be an inline install, in order to pass the
   // checks in ExtensionService::IsDownloadFromGallery. We should instead pass
   // the real referrer, track if this is an inline install in the whitelist
   // entry and look that up when checking that this is a valid download.
   GURL referrer = controller_->GetActiveEntry()->GetURL();
   if (flags_ & FLAG_INLINE_INSTALL)
     referrer = GURL(extension_urls::GetWebstoreItemDetailURLPrefix() + id_);
 
   content::DownloadSaveInfo save_info;
   save_info.file_path = file;
 
   // The download url for the given extension is contained in |download_url_|.
   // We will navigate the current tab to this url to start the download. The
   // download system will then pass the crx to the CrxInstaller.
   download_util::RecordDownloadSource(
         download_util::INITIATED_BY_WEBSTORE_INSTALLER);
   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().
 }
 
 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;
+  }
+}