Report cache status for ExtensionDownloader

ExtensionDownloader now reports to its delegate cache status
(hit/miss/outdated version) of the extension. We need it for later to
gather statistics and, in turn, to investigate how effectively cache
works, for example, for force-installed extensions.

Bug: 1015817
Change-Id: I22a0e73d2926f073a231fe13c73879608e2df7e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1868881Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Commit-Queue: Oleg Davydov <burunduk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712086}

extensions/browser/updater/extension_downloader.cc

@@ -859,6 +859,43 @@ void ExtensionDownloader::DetermineUpdates(
   }
 }
 
+base::Optional<base::FilePath> ExtensionDownloader::GetCachedExtension(
+    const ExtensionFetch& fetch_data) {
+  if (!extension_cache_) {
+    delegate_->OnExtensionDownloadCacheStatusRetrieved(
+        fetch_data.id,
+        ExtensionDownloaderDelegate::CacheStatus::CACHE_DISABLED);
+    return base::nullopt;
+  }
+
+  std::string version;
+  if (!extension_cache_->GetExtension(fetch_data.id, fetch_data.package_hash,
+                                      nullptr, &version)) {
+    delegate_->OnExtensionDownloadCacheStatusRetrieved(
+        fetch_data.id, ExtensionDownloaderDelegate::CacheStatus::CACHE_MISS);
+    return base::nullopt;
+  }
+
+  if (version != fetch_data.version) {
+    delegate_->OnExtensionDownloadCacheStatusRetrieved(
+        fetch_data.id,
+        ExtensionDownloaderDelegate::CacheStatus::CACHE_OUTDATED);
+    return base::nullopt;
+  }
+
+  delegate_->OnExtensionDownloadCacheStatusRetrieved(
+      fetch_data.id, ExtensionDownloaderDelegate::CacheStatus::CACHE_HIT);
+
+  base::FilePath crx_path;
+  // Now get .crx file path.
+  // TODO(https://crbug.com/1018271#c2) This has a  side-effect in extension
+  // cache implementation: extension in the cache will be marked as recently
+  // used.
+  extension_cache_->GetExtension(fetch_data.id, fetch_data.package_hash,
+                                 &crx_path, &version);
+  return std::move(crx_path);
+}
+
 // Begins (or queues up) download of an updated extension.
 void ExtensionDownloader::FetchUpdatedExtension(
     std::unique_ptr<ExtensionFetch> fetch_data) {
@@ -892,25 +929,19 @@ void ExtensionDownloader::FetchUpdatedExtension(
         fetch_data->id, ExtensionDownloaderDelegate::Stage::DOWNLOADING_CRX);
     extensions_queue_.active_request()->request_ids.insert(
         fetch_data->request_ids.begin(), fetch_data->request_ids.end());
+    return;
+  }
+  base::Optional<base::FilePath> cached_crx_path =
+      GetCachedExtension(*fetch_data);
+  if (cached_crx_path) {
+    delegate_->OnExtensionDownloadStageChanged(
+        fetch_data->id, ExtensionDownloaderDelegate::Stage::FINISHED);
+    NotifyDelegateDownloadFinished(std::move(fetch_data), true,
+                                   cached_crx_path.value(), false);
   } else {
-    std::string version;
-    if (extension_cache_ &&
-        extension_cache_->GetExtension(fetch_data->id, fetch_data->package_hash,
-                                       NULL, &version) &&
-        version == fetch_data->version) {
-      base::FilePath crx_path;
-      // Now get .crx file path and mark extension as used.
-      extension_cache_->GetExtension(fetch_data->id, fetch_data->package_hash,
-                                     &crx_path, &version);
-      delegate_->OnExtensionDownloadStageChanged(
-          fetch_data->id, ExtensionDownloaderDelegate::Stage::FINISHED);
-      NotifyDelegateDownloadFinished(std::move(fetch_data), true, crx_path,
-                                     false);
-    } else {
-      delegate_->OnExtensionDownloadStageChanged(
-          fetch_data->id, ExtensionDownloaderDelegate::Stage::QUEUED_FOR_CRX);
-      extensions_queue_.ScheduleRequest(std::move(fetch_data));
-    }
+    delegate_->OnExtensionDownloadStageChanged(
+        fetch_data->id, ExtensionDownloaderDelegate::Stage::QUEUED_FOR_CRX);
+    extensions_queue_.ScheduleRequest(std::move(fetch_data));
   }
 }
 

extensions/browser/updater/extension_downloader.h

@@ -13,8 +13,10 @@
 #include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/version.h"
 #include "extensions/browser/updater/extension_downloader_delegate.h"
 #include "extensions/browser/updater/manifest_fetch_data.h"
@@ -281,6 +283,11 @@ class ExtensionDownloader {
                         std::set<std::string>* no_updates,
                         std::set<std::string>* errors);
 
+  // Checks whether extension is presented in cache. If yes, return path to its
+  // cached CRX, base::nullopt otherwise.
+  base::Optional<base::FilePath> GetCachedExtension(
+      const ExtensionFetch& fetch_data);
+
   // Begins (or queues up) download of an updated extension.
   void FetchUpdatedExtension(std::unique_ptr<ExtensionFetch> fetch_data);
 

extensions/browser/updater/extension_downloader_delegate.cc

@@ -20,12 +20,16 @@ ExtensionDownloaderDelegate::~ExtensionDownloaderDelegate() {
 
 void ExtensionDownloaderDelegate::OnExtensionDownloadStageChanged(
     const ExtensionId& id,
-    ExtensionDownloaderDelegate::Stage stage) {}
+    Stage stage) {}
+
+void ExtensionDownloaderDelegate::OnExtensionDownloadCacheStatusRetrieved(
+    const ExtensionId& id,
+    CacheStatus cache_status) {}
 
 void ExtensionDownloaderDelegate::OnExtensionDownloadFailed(
     const ExtensionId& id,
-    ExtensionDownloaderDelegate::Error error,
-    const ExtensionDownloaderDelegate::PingResult& ping_result,
+    Error error,
+    const PingResult& ping_result,
     const std::set<int>& request_id) {}
 
 void ExtensionDownloaderDelegate::OnExtensionDownloadRetryForTests() {}

extensions/browser/updater/extension_downloader_delegate.h

@@ -43,7 +43,7 @@ class ExtensionDownloaderDelegate {
     CRX_FETCH_FAILED,
   };
 
-  // Passed as an argument OnExtensionDownloadStageChanged() to detail how
+  // Passed as an argument to OnExtensionDownloadStageChanged() to detail how
   // downloading is going on. Typical sequence is: PENDING ->
   // QUEUED_FOR_MANIFEST -> DOWNLOADING_MANIFEST -> PARSING_MANIFEST ->
   // MANIFEST_LOADED -> QUEUED_FOR_CRX -> DOWNLOADING_CRX -> FINISHED. Stages
@@ -95,6 +95,33 @@ class ExtensionDownloaderDelegate {
     kMaxValue = FINISHED,
   };
 
+  // Passes as an argument to OnExtensionDownloadCacheStatusRetrieved to inform
+  // delegate about cache status.
+  // Note: enum used for UMA. Do NOT reorder or remove entries. Don't forget to
+  // update enums.xml (name: ExtensionInstallationCacheStatus) when adding new
+  // entries.
+  enum class CacheStatus {
+    // No information about cache status. This is never reported by
+    // ExtensionDownloader, but may be used later in statistics.
+    CACHE_UNKNOWN = 0,
+
+    // There is no cache at all.
+    CACHE_DISABLED = 1,
+
+    // Extension was not found in cache.
+    CACHE_MISS = 2,
+
+    // There is an extension in cache, but its version is not as expected.
+    CACHE_OUTDATED = 3,
+
+    // Cache entry is good and will be used.
+    CACHE_HIT = 4,
+
+    // Magic constant used by the histogram macros.
+    // Always update it to the max value.
+    kMaxValue = CACHE_HIT,
+  };
+
   // Passed as an argument to the completion callbacks to signal whether
   // the extension update sent a ping.
   struct PingResult {
@@ -131,6 +158,13 @@ class ExtensionDownloaderDelegate {
   virtual void OnExtensionDownloadStageChanged(const ExtensionId& id,
                                                Stage stage);
 
+  // Invoked once during downloading, after fetching and parsing update
+  // manifest, |cache_status| contains information about what have we found in
+  // local cache about the extension.
+  virtual void OnExtensionDownloadCacheStatusRetrieved(
+      const ExtensionId& id,
+      CacheStatus cache_status);
+
   // Invoked if the extension couldn't be downloaded. |error| contains the
   // failure reason.
   virtual void OnExtensionDownloadFailed(const ExtensionId& id,

extensions/browser/updater/extension_downloader_test_helper.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <set>
+#include <string>
 
 #include "extensions/browser/updater/extension_downloader.h"
 #include "extensions/browser/updater/extension_downloader_delegate.h"
@@ -25,9 +26,11 @@ class MockExtensionDownloaderDelegate : public ExtensionDownloaderDelegate {
 
   MOCK_METHOD4(
       OnExtensionDownloadFailed,
-      void(const std::string&, Error, const PingResult&, const std::set<int>&));
+      void(const ExtensionId&, Error, const PingResult&, const std::set<int>&));
   MOCK_METHOD2(OnExtensionDownloadStageChanged,
-               void(const std::string&, Stage));
+               void(const ExtensionId&, Stage));
+  MOCK_METHOD2(OnExtensionDownloadCacheStatusRetrieved,
+               void(const ExtensionId&, CacheStatus));
   MOCK_METHOD7(OnExtensionDownloadFinished,
                void(const extensions::CRXFileInfo&,
                     bool,
@@ -38,11 +41,11 @@ class MockExtensionDownloaderDelegate : public ExtensionDownloaderDelegate {
                     const InstallCallback&));
   MOCK_METHOD0(OnExtensionDownloadRetryForTests, void());
   MOCK_METHOD2(GetPingDataForExtension,
-               bool(const std::string&, ManifestFetchData::PingData*));
-  MOCK_METHOD1(GetUpdateUrlData, std::string(const std::string&));
-  MOCK_METHOD1(IsExtensionPending, bool(const std::string&));
+               bool(const ExtensionId&, ManifestFetchData::PingData*));
+  MOCK_METHOD1(GetUpdateUrlData, std::string(const ExtensionId&));
+  MOCK_METHOD1(IsExtensionPending, bool(const ExtensionId&));
   MOCK_METHOD2(GetExtensionExistingVersion,
-               bool(const std::string&, std::string*));
+               bool(const ExtensionId&, std::string*));
 
   void Wait();
 

extensions/browser/updater/extension_downloader_unittest.cc

@@ -8,6 +8,7 @@
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/extensions_test.h"
+#include "extensions/browser/updater/extension_cache_fake.h"
 #include "extensions/browser/updater/extension_downloader_test_helper.h"
 #include "extensions/common/extension.h"
 
@@ -52,6 +53,32 @@ class ExtensionDownloaderTest : public ExtensionsTest {
                                 std::unique_ptr<ManifestFetchData> fetch) {
     helper->downloader().StartUpdateCheck(std::move(fetch));
   }
+
+  std::string CreateUpdateManifest(const std::string& extension_id,
+                                   const std::string& extension_version) {
+    return "<?xml version='1.0' encoding='UTF-8'?>"
+           "<gupdate xmlns='http://www.google.com/update2/response'"
+           "                protocol='2.0'>"
+           " <app appid='" +
+           extension_id +
+           "'>"
+           "  <updatecheck codebase='http://example.com/extension_1.2.3.4.crx'"
+           "               version='" +
+           extension_version +
+           "' prodversionmin='1.1' />"
+           " </app>"
+           "</gupdate>";
+  }
+
+  void SetHelperHandlers(ExtensionDownloaderTestHelper* helper,
+                         const GURL& fetch_url,
+                         const std::string& manifest) {
+    helper->test_url_loader_factory().AddResponse(fetch_url.spec(), manifest,
+                                                  net::HTTP_OK);
+    GURL kCrxUrl("http://example.com/extension_1.2.3.4.crx");
+    helper->test_url_loader_factory().AddResponse(kCrxUrl.spec(), "no data",
+                                                  net::HTTP_OK);
+  }
 };
 
 // Several tests checking that OnExtensionDownloadStageChanged callback is
@@ -60,28 +87,16 @@ TEST_F(ExtensionDownloaderTest, TestStageChanges) {
   ExtensionDownloaderTestHelper helper;
 
   std::unique_ptr<ManifestFetchData> fetch(CreateTestAppFetchData());
-  GURL fetch_url = fetch->full_url();
-
-  const std::string kManifest =
-      "<?xml version='1.0' encoding='UTF-8'?>"
-      "<gupdate xmlns='http://www.google.com/update2/response'"
-      "                protocol='2.0'>"
-      " <app appid='" +
-      std::string(kTestExtensionId) +
-      "'>"
-      "  <updatecheck codebase='http://example.com/extension_1.2.3.4.crx'"
-      "               version='1.1' prodversionmin='1.1' />"
-      " </app>"
-      "</gupdate>";
-  helper.test_url_loader_factory().AddResponse(fetch_url.spec(), kManifest,
-                                               net::HTTP_OK);
-  GURL kCrxUrl("http://example.com/extension_1.2.3.4.crx");
-  helper.test_url_loader_factory().AddResponse(kCrxUrl.spec(), "no data",
-                                               net::HTTP_OK);
+  const std::string manifest = CreateUpdateManifest(kTestExtensionId, "1.1");
+  SetHelperHandlers(&helper, fetch->full_url(), manifest);
 
   MockExtensionDownloaderDelegate& delegate = helper.delegate();
   EXPECT_CALL(delegate, IsExtensionPending(kTestExtensionId))
       .WillOnce(Return(true));
+  EXPECT_CALL(delegate,
+              OnExtensionDownloadCacheStatusRetrieved(
+                  kTestExtensionId,
+                  ExtensionDownloaderDelegate::CacheStatus::CACHE_DISABLED));
   Sequence sequence;
   EXPECT_CALL(delegate,
               OnExtensionDownloadStageChanged(
@@ -128,24 +143,8 @@ TEST_F(ExtensionDownloaderTest, TestStageChangesNoUpdates) {
   ExtensionDownloaderTestHelper helper;
 
   std::unique_ptr<ManifestFetchData> fetch(CreateTestAppFetchData());
-  GURL fetch_url = fetch->full_url();
-
-  const std::string kManifest =
-      "<?xml version='1.0' encoding='UTF-8'?>"
-      "<gupdate xmlns='http://www.google.com/update2/response'"
-      "                protocol='2.0'>"
-      " <app appid='" +
-      std::string(kTestExtensionId) +
-      "'>"
-      "  <updatecheck codebase='http://example.com/extension_1.2.3.4.crx'"
-      "               version='1.1' prodversionmin='1.1' />"
-      " </app>"
-      "</gupdate>";
-  helper.test_url_loader_factory().AddResponse(fetch_url.spec(), kManifest,
-                                               net::HTTP_OK);
-  GURL kCrxUrl("http://example.com/extension_1.2.3.4.crx");
-  helper.test_url_loader_factory().AddResponse(kCrxUrl.spec(), "no data",
-                                               net::HTTP_OK);
+  const std::string manifest = CreateUpdateManifest(kTestExtensionId, "1.1");
+  SetHelperHandlers(&helper, fetch->full_url(), manifest);
 
   MockExtensionDownloaderDelegate& delegate = helper.delegate();
   EXPECT_CALL(delegate, IsExtensionPending(kTestExtensionId))
@@ -308,4 +307,98 @@ TEST_F(ExtensionDownloaderTest, TestNoUpdatesManifestReports) {
   testing::Mock::VerifyAndClearExpectations(&delegate);
 }
 
+// Test that cache status callback is called correctly if there is no such
+// extension in cache.
+TEST_F(ExtensionDownloaderTest, TestCacheStatusMiss) {
+  ExtensionDownloaderTestHelper helper;
+
+  std::unique_ptr<ManifestFetchData> fetch(CreateTestAppFetchData());
+  const std::string manifest = CreateUpdateManifest(kTestExtensionId, "1.1");
+  SetHelperHandlers(&helper, fetch->full_url(), manifest);
+
+  // Create cache and provide it to downloader.
+  auto test_extension_cache = std::make_unique<ExtensionCacheFake>();
+  helper.downloader().StartAllPending(test_extension_cache.get());
+
+  MockExtensionDownloaderDelegate& delegate = helper.delegate();
+  EXPECT_CALL(delegate, IsExtensionPending(kTestExtensionId))
+      .WillOnce(Return(true));
+  EXPECT_CALL(delegate,
+              OnExtensionDownloadCacheStatusRetrieved(
+                  kTestExtensionId,
+                  ExtensionDownloaderDelegate::CacheStatus::CACHE_MISS));
+
+  AddFetchDataToDownloader(&helper, std::move(fetch));
+
+  content::RunAllTasksUntilIdle();
+
+  testing::Mock::VerifyAndClearExpectations(&delegate);
+}
+
+// Test that cache status callback is called correctly if there is an extension
+// archive in cache, but only an old version.
+TEST_F(ExtensionDownloaderTest, TestCacheStatusOutdated) {
+  ExtensionDownloaderTestHelper helper;
+
+  std::unique_ptr<ManifestFetchData> fetch(CreateTestAppFetchData());
+  const std::string manifest = CreateUpdateManifest(kTestExtensionId, "1.1");
+  SetHelperHandlers(&helper, fetch->full_url(), manifest);
+
+  // Create cache and provide it to downloader.
+  auto test_extension_cache = std::make_unique<ExtensionCacheFake>();
+  test_extension_cache->AllowCaching(kTestExtensionId);
+  test_extension_cache->PutExtension(
+      kTestExtensionId, "" /* expected hash, ignored by ExtensionCacheFake */,
+      base::FilePath(), "1.0", base::DoNothing());
+  helper.downloader().StartAllPending(test_extension_cache.get());
+
+  MockExtensionDownloaderDelegate& delegate = helper.delegate();
+  EXPECT_CALL(delegate, IsExtensionPending(kTestExtensionId))
+      .WillOnce(Return(true));
+  EXPECT_CALL(delegate,
+              OnExtensionDownloadCacheStatusRetrieved(
+                  kTestExtensionId,
+                  ExtensionDownloaderDelegate::CacheStatus::CACHE_OUTDATED));
+
+  AddFetchDataToDownloader(&helper, std::move(fetch));
+
+  content::RunAllTasksUntilIdle();
+
+  testing::Mock::VerifyAndClearExpectations(&delegate);
+}
+
+// Test that cache status callback is called correctly if there is an extension
+// archive in cache.
+TEST_F(ExtensionDownloaderTest, TestCacheStatusHit) {
+  ExtensionDownloaderTestHelper helper;
+
+  std::unique_ptr<ManifestFetchData> fetch(CreateTestAppFetchData());
+  const std::string manifest = CreateUpdateManifest(kTestExtensionId, "1.1");
+  SetHelperHandlers(&helper, fetch->full_url(), manifest);
+
+  // Create cache and provide it to downloader.
+  auto test_extension_cache = std::make_unique<ExtensionCacheFake>();
+  test_extension_cache->AllowCaching(kTestExtensionId);
+  test_extension_cache->PutExtension(
+      kTestExtensionId, "" /* expected hash, ignored by ExtensionCacheFake */,
+      base::FilePath(FILE_PATH_LITERAL(
+          "file_not_found.crx")) /* We don't need to actually read file. */,
+      "1.1", base::DoNothing());
+  helper.downloader().StartAllPending(test_extension_cache.get());
+
+  MockExtensionDownloaderDelegate& delegate = helper.delegate();
+  EXPECT_CALL(delegate, IsExtensionPending(kTestExtensionId))
+      .WillOnce(Return(true));
+  EXPECT_CALL(delegate,
+              OnExtensionDownloadCacheStatusRetrieved(
+                  kTestExtensionId,
+                  ExtensionDownloaderDelegate::CacheStatus::CACHE_HIT));
+
+  AddFetchDataToDownloader(&helper, std::move(fetch));
+
+  content::RunAllTasksUntilIdle();
+
+  testing::Mock::VerifyAndClearExpectations(&delegate);
+}
+
 }  // namespace extensions