Add histograms to understand why so many CRX installs fail in the field.

* Report path length to extensions dir.
* Report cause and install source in histograms.
* Add histogram to show why writing an update to a file might fail.
* Don't create installer for crx that can't be read.

BUG=81687
TEST=Manual: look at about:histogram

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@84617 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 73 insertions, 9 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index 64eef79..a29ae11 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -784,6 +784,7 @@ void AutomationProvider::InstallExtension(const FilePath& crx_path,
 
     // Pass NULL for a silent install with no UI.
     scoped_refptr<CrxInstaller> installer(service->MakeCrxInstaller(NULL));
+    installer->set_install_cause(extension_misc::INSTALL_CAUSE_AUTOMATION);
     installer->InstallCrx(crx_path);
   } else {
     AutomationMsg_InstallExtension::WriteReplyParams(
@@ -816,6 +817,7 @@ void AutomationProvider::InstallExtensionAndGetHandle(
     ExtensionInstallUI* client =
         (with_ui ? new ExtensionInstallUI(profile_) : NULL);
     scoped_refptr<CrxInstaller> installer(service->MakeCrxInstaller(client));
+    installer->set_install_cause(extension_misc::INSTALL_CAUSE_AUTOMATION);
     installer->InstallCrx(crx_path);
   } else {
     AutomationMsg_InstallExtensionAndGetHandle::WriteReplyParams(
diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc
index 72a0b92..720d03a 100644
--- a/chrome/browser/download/download_util.cc
+++ b/chrome/browser/download/download_util.cc
@@ -341,8 +341,9 @@ void OpenChromeExtension(Profile* profile,
   installer->set_apps_require_extension_mime_type(true);
   installer->set_original_url(download_item.url());
   installer->set_is_gallery_install(is_gallery_download);
-  installer->InstallCrx(download_item.full_path());
   installer->set_allow_silent_install(is_gallery_download);
+  installer->set_install_cause(extension_misc::INSTALL_CAUSE_USER_DOWNLOAD);
+  installer->InstallCrx(download_item.full_path());
 }
 
 void RecordDownloadCount(DownloadCountTypes type) {
diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index 66aadd4..e7d0f94 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -117,7 +117,8 @@ CrxInstaller::CrxInstaller(base::WeakPtr<ExtensionService> frontend_weak,
       frontend_weak_(frontend_weak),
       client_(client),
       apps_require_extension_mime_type_(false),
-      allow_silent_install_(false) {
+      allow_silent_install_(false),
+      install_cause_(extension_misc::INSTALL_CAUSE_UNSET) {
 }
 
 CrxInstaller::~CrxInstaller() {
@@ -297,6 +298,15 @@ bool CrxInstaller::AllowInstall(const Extension* extension,
 
 void CrxInstaller::OnUnpackFailure(const std::string& error_message) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackFailureInstallSource",
+                            install_source(), Extension::NUM_LOCATIONS);
+
+
+  UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackFailureInstallCause",
+                            install_cause(),
+                            extension_misc::NUM_INSTALL_CAUSES);
+
   ReportFailureFromFileThread(error_message);
 }
 
@@ -305,6 +315,14 @@ void CrxInstaller::OnUnpackSuccess(const FilePath& temp_dir,
                                    const Extension* extension) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
+  UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackSuccessInstallSource",
+                            install_source(), Extension::NUM_LOCATIONS);
+
+
+  UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackSuccessInstallCause",
+                            install_cause(),
+                            extension_misc::NUM_INSTALL_CAUSES);
+
   // Note: We take ownership of |extension| and |temp_dir|.
   extension_ = extension;
   temp_dir_ = temp_dir;
diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h
index 4294fa2..1759517 100644
--- a/chrome/browser/extensions/crx_installer.h
+++ b/chrome/browser/extensions/crx_installer.h
@@ -47,7 +47,6 @@ class CrxInstaller
     : public SandboxedExtensionUnpackerClient,
       public ExtensionInstallUI::Delegate {
  public:
-
   // This is pretty lame, but given the difficulty of connecting a particular
   // ExtensionFunction to a resulting download in the download manager, it's
   // currently necessary. This is the |id| of an extension to be installed
@@ -135,6 +134,14 @@ class CrxInstaller
     original_mime_type_ = original_mime_type;
   }
 
+  extension_misc::CrxInstallCause install_cause() const {
+    return install_cause_;
+  }
+
+  void set_install_cause(extension_misc::CrxInstallCause install_cause) {
+    install_cause_ = install_cause;
+  }
+
  private:
   ~CrxInstaller();
 
@@ -256,6 +263,10 @@ class CrxInstaller
   // Ignorred unless |require_extension_mime_type_| is true.
   std::string original_mime_type_;
 
+  // What caused this install?  Used only for histograms that report
+  // on failure rates, broken down by the cause of the install.
+  extension_misc::CrxInstallCause install_cause_;
+
   DISALLOW_COPY_AND_ASSIGN(CrxInstaller);
 };
 
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index fca68a1..603772c 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -488,6 +488,10 @@ ExtensionService::ExtensionService(Profile* profile,
   omnibox_icon_manager_.set_monochrome(true);
   omnibox_icon_manager_.set_padding(gfx::Insets(0, kOmniboxIconPaddingLeft,
                                                 0, kOmniboxIconPaddingRight));
+
+  // How long is the path to the Extensions directory?
+  UMA_HISTOGRAM_CUSTOM_COUNTS("Extensions.ExtensionRootPathLength",
+                              install_directory_.value().length(), 0, 500, 100);
 }
 
 const ExtensionList* ExtensionService::extensions() const {
@@ -610,6 +614,7 @@ void ExtensionService::UpdateExtension(const std::string& id,
     installer->set_install_source(extension->location());
   installer->set_delete_source(true);
   installer->set_original_url(download_url);
+  installer->set_install_cause(extension_misc::INSTALL_CAUSE_UPDATE);
   installer->InstallCrx(extension_path);
 }
 
@@ -2018,7 +2023,8 @@ void ExtensionService::OnExternalExtensionFileFound(
   scoped_refptr<CrxInstaller> installer(MakeCrxInstaller(NULL));
   installer->set_install_source(location);
   installer->set_expected_id(id);
-  installer->set_expected_version(*version),
+  installer->set_expected_version(*version);
+  installer->set_install_cause(extension_misc::INSTALL_CAUSE_EXTERNAL_FILE);
   installer->InstallCrx(path);
 }
 
diff --git a/chrome/browser/extensions/extension_updater.cc b/chrome/browser/extensions/extension_updater.cc
index a340755..f3c9e34 100644
--- a/chrome/browser/extensions/extension_updater.cc
+++ b/chrome/browser/extensions/extension_updater.cc
@@ -10,6 +10,7 @@
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/file_util.h"
+#include "base/memory/scoped_handle.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/stl_util-inl.h"
@@ -55,6 +56,8 @@ using prefs::kNextExtensionsUpdateCheck;
 // Update AppID for extension blacklist.
 const char* ExtensionUpdater::kBlacklistAppID = "com.google.crx.blacklist";
 
+namespace {
+
 // Wait at least 5 minutes after browser startup before we do any checks. If you
 // change this value, make sure to update comments where it is used.
 const int kStartupWaitSeconds = 60 * 5;
@@ -67,6 +70,16 @@ static const int kMaxUpdateFrequencySeconds = 60 * 60 * 24 * 7;  // 7 days
 // request. We want to stay under 2K because of proxies, etc.
 static const int kExtensionsManifestMaxURLSize = 2000;
 
+enum FileWriteResult {
+  SUCCESS = 0,
+  CANT_CREATE_TEMP_CRX,
+  CANT_WRITE_CRX_DATA,
+  CANT_READ_CRX_FILE,
+  NUM_FILE_WRITE_RESULTS
+};
+
+}  // namespace
+
 ManifestFetchData::ManifestFetchData(const GURL& update_url)
     : base_url_(update_url),
       full_url_(update_url) {
@@ -381,21 +394,35 @@ class ExtensionUpdaterFileHandler
     // Make sure we're running in the right thread.
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-    bool failed = false;
+    FileWriteResult file_write_result = SUCCESS;
     FilePath path;
     if (!file_util::CreateTemporaryFile(&path)) {
       LOG(WARNING) << "Failed to create temporary file path";
-      failed = true;
+      file_write_result = CANT_CREATE_TEMP_CRX;
     } else if (file_util::WriteFile(path, data.c_str(), data.length()) !=
         static_cast<int>(data.length())) {
       // TODO(asargent) - It would be nice to back off updating altogether if
       // the disk is full. (http://crbug.com/12763).
       LOG(ERROR) << "Failed to write temporary file";
       file_util::Delete(path, false);
-      failed = true;
+      file_write_result = CANT_WRITE_CRX_DATA;
+    } else {
+      // We are seeing a high failure rate unpacking extensions, where
+      // the crx file can not be read. See if the file we wrote is readable.
+      // See crbug.com/81687 .
+      ScopedStdioHandle file(file_util::OpenFile(path, "rb"));
+      if (!file.get()) {
+        LOG(ERROR) << "Can't read CRX file written for update at path "
+                   << path.value().c_str();
+        file_util::Delete(path, false);
+        file_write_result = CANT_READ_CRX_FILE;
+      }
     }
 
-    if (failed) {
+    UMA_HISTOGRAM_ENUMERATION("Extensions.UpdaterWriteCrx", file_write_result,
+                              NUM_FILE_WRITE_RESULTS);
+
+    if (file_write_result != SUCCESS) {
       if (!BrowserThread::PostTask(
               BrowserThread::UI, FROM_HERE,
               NewRunnableMethod(
diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.cc b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
index f449b6a..8e31ddf 100644
--- a/chrome/browser/extensions/sandboxed_extension_unpacker.cc
+++ b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
@@ -472,7 +472,6 @@ bool SandboxedExtensionUnpacker::ValidateSignature() {
 
 void SandboxedExtensionUnpacker::ReportFailure(FailureReason reason,
                                                const std::string& error) {
-  UMA_HISTOGRAM_COUNTS("Extensions.SandboxUnpackFailure", 1);
   UMA_HISTOGRAM_ENUMERATION("Extensions.SandboxUnpackFailureReason",
                             reason, NUM_FAILURE_REASONS);
   UMA_HISTOGRAM_TIMES("Extensions.SandboxUnpackFailureTime",