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;

[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 {
 
 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[] = "";
 
 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 57 matching lines @@
 WebstoreInstaller::WebstoreInstaller(Profile* profile,
                                      Delegate* delegate,
                                      NavigationController* controller,
                                      const std::string& id,
                                      int flags)
     : profile_(profile),
       delegate_(delegate),
       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);
 
   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() {
+  // 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));
   AddRef();  // Balanced in ReportSuccess and ReportFailure.
 
   if (!Extension::IdIsValid(id_)) {
     ReportFailure(kInvalidIdError);
     return;
   }
 
@@ skipping 37 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;
+  }
+
+  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));
 
   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);
 
   Release();  // Balanced in Start().
 }
 
 void WebstoreInstaller::ReportSuccess() {
   if (delegate_)
     delegate_->OnExtensionInstallSuccess(id_);
 
   Release();  // Balanced in Start().
 }