Implement ContentHash caching.

Add caching for ContentHash instances for a particular version of
an extension. Since ContentHash is required for all extension
resource requests when content verification applies, this should
speed up ContentVerifyJob.

Note that we should expect gains in
"ExtensionContentVerifyJob.TimeSpentUS" UMA.

Bug: 796395
Test: No visible behavior changes expected.
Change-Id: I4d03bcd95f00215628eb9b3f143e15e7bd2c84d5
Reviewed-on: https://chromium-review.googlesource.com/c/1266604
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605875}

extensions/browser/content_hash_fetcher_unittest.cc

@@ -62,7 +62,7 @@ class ContentHashWaiter {
   }
 
  private:
-  void CreatedCallback(const scoped_refptr<ContentHash>& content_hash,
+  void CreatedCallback(scoped_refptr<ContentHash> content_hash,
                        bool was_cancelled) {
     if (!reply_task_runner_->RunsTasksInCurrentSequence()) {
       reply_task_runner_->PostTask(

extensions/browser/content_verifier.cc

@@ -114,11 +114,34 @@ std::unique_ptr<ContentVerifierIOData::ExtensionData> CreateIOData(
 
 }  // namespace
 
+struct ContentVerifier::CacheKey {
+  CacheKey(const ExtensionId& extension_id,
+           const base::Version& version,
+           bool needs_force_missing_computed_hashes_creation)
+      : extension_id(extension_id),
+        version(version),
+        needs_force_missing_computed_hashes_creation(
+            needs_force_missing_computed_hashes_creation) {}
+
+  bool operator<(const CacheKey& other) const {
+    return std::tie(extension_id, version,
+                    needs_force_missing_computed_hashes_creation) <
+           std::tie(other.extension_id, other.version,
+                    other.needs_force_missing_computed_hashes_creation);
+  }
+
+  ExtensionId extension_id;
+  base::Version version;
+  // TODO(lazyboy): This shouldn't be necessary as key. For the common
+  // case, we'd only want to cache successful ContentHash instances regardless
+  // of whether force creation was requested.
+  bool needs_force_missing_computed_hashes_creation = false;
+};
+
 // A class to retrieve ContentHash for ContentVerifier.
 //
 // All public calls originate and terminate on IO, making it suitable for
 // ContentVerifier to cache ContentHash instances easily.
-// TODO(lazyboy): Implement caching.
 //
 // This class makes sure we do not have more than one ContentHash request in
 // flight for a particular version of an extension. If a call to retrieve an
@@ -245,7 +268,7 @@ class ContentVerifier::HashHelper {
   using IsCancelledCallback = base::RepeatingCallback<bool(void)>;
 
   static void ForwardToIO(ContentHash::CreatedCallback callback,
-                          const scoped_refptr<ContentHash>& content_hash,
+                          scoped_refptr<ContentHash> content_hash,
                           bool was_cancelled) {
     // If the request was cancelled, then we don't have a corresponding entry
     // for the request in |callback_infos_| anymore.
@@ -278,7 +301,7 @@ class ContentVerifier::HashHelper {
 
   void DidReadHash(const CallbackKey& key,
                    const scoped_refptr<IsCancelledChecker>& checker,
-                   const scoped_refptr<ContentHash>& content_hash,
+                   scoped_refptr<ContentHash> content_hash,
                    bool was_cancelled) {
     DCHECK(checker);
     if (was_cancelled ||
@@ -318,7 +341,7 @@ class ContentVerifier::HashHelper {
 
   void CompleteDidReadHash(const CallbackKey& key,
                            const scoped_refptr<IsCancelledChecker>& checker,
-                           const scoped_refptr<ContentHash>& content_hash,
+                           scoped_refptr<ContentHash> content_hash,
                            bool was_cancelled) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     DCHECK(checker);
@@ -456,6 +479,17 @@ void ContentVerifier::GetContentHash(
     return;
   }
 
+  CacheKey cache_key(extension_id, extension_version,
+                     force_missing_computed_hashes_creation);
+  auto cache_iter = cache_.find(cache_key);
+  if (cache_iter != cache_.end()) {
+    // Currently, we expect |callback| to be called asynchronously.
+    base::PostTaskWithTraits(
+        FROM_HERE, {content::BrowserThread::IO},
+        base::BindOnce(std::move(callback), cache_iter->second));
+    return;
+  }
+
   ContentHash::ExtensionKey extension_key(extension_id, extension_root,
                                           extension_version,
                                           delegate_->GetPublicKey());
@@ -465,7 +499,9 @@ void ContentVerifier::GetContentHash(
   // non-nullptr instance of HashHelper.
   GetOrCreateHashHelper()->GetContentHash(
       extension_key, std::move(fetch_params),
-      force_missing_computed_hashes_creation, std::move(callback));
+      force_missing_computed_hashes_creation,
+      base::BindOnce(&ContentVerifier::DidGetContentHash, this, cache_key,
+                     std::move(callback)));
 }
 
 void ContentVerifier::VerifyFailed(const ExtensionId& extension_id,
@@ -555,6 +591,11 @@ void ContentVerifier::OnExtensionUnloadedOnIO(
   if (shutdown_on_io_)
     return;
   io_data_->RemoveData(extension_id);
+
+  // Remove all possible cache entries for this extension version.
+  cache_.erase(CacheKey(extension_id, extension_version, true));
+  cache_.erase(CacheKey(extension_id, extension_version, false));
+
   HashHelper* hash_helper = GetOrCreateHashHelper();
   if (hash_helper)
     hash_helper->Cancel(extension_id, extension_version);
@@ -600,6 +641,14 @@ ContentHash::FetchParams ContentVerifier::GetFetchParams(
       delegate_->GetSignatureFetchUrl(extension_id, extension_version));
 }
 
+void ContentVerifier::DidGetContentHash(
+    const CacheKey& cache_key,
+    ContentHashCallback original_callback,
+    scoped_refptr<const ContentHash> content_hash) {
+  cache_[cache_key] = content_hash;
+  std::move(original_callback).Run(content_hash);
+}
+
 void ContentVerifier::BindURLLoaderFactoryRequestOnUIThread(
     network::mojom::URLLoaderFactoryRequest url_loader_factory_request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

extensions/browser/content_verifier.h

@@ -77,7 +77,7 @@ class ContentVerifier : public base::RefCountedThreadSafe<ContentVerifier>,
                            UnloadedExtensionReason reason) override;
 
   using ContentHashCallback =
-      base::OnceCallback<void(const scoped_refptr<const ContentHash>&)>;
+      base::OnceCallback<void(scoped_refptr<const ContentHash>)>;
 
   // Retrieves ContentHash for an extension through |callback|.
   // Must be called on IO thread.
@@ -110,6 +110,7 @@ class ContentVerifier : public base::RefCountedThreadSafe<ContentVerifier>,
 
   void ShutdownOnIO();
 
+  struct CacheKey;
   class HashHelper;
 
   void OnFetchComplete(const scoped_refptr<const ContentHash>& content_hash);
@@ -117,6 +118,10 @@ class ContentVerifier : public base::RefCountedThreadSafe<ContentVerifier>,
       const ExtensionId& extension_id,
       const base::Version& extension_version);
 
+  void DidGetContentHash(const CacheKey& cache_key,
+                         ContentHashCallback orig_callback,
+                         scoped_refptr<const ContentHash> content_hash);
+
   // Binds an URLLoaderFactoryRequest on the UI thread.
   void BindURLLoaderFactoryRequestOnUIThread(
       network::mojom::URLLoaderFactoryRequest url_loader_factory_request);
@@ -162,6 +167,8 @@ class ContentVerifier : public base::RefCountedThreadSafe<ContentVerifier>,
   std::unique_ptr<HashHelper, content::BrowserThread::DeleteOnIOThread>
       hash_helper_;
 
+  std::map<CacheKey, scoped_refptr<const ContentHash>> cache_;
+
   std::unique_ptr<ContentVerifierDelegate> delegate_;
 
   // For observing the ExtensionRegistry.

extensions/browser/content_verifier/content_hash.h

@@ -88,7 +88,7 @@ class ContentHash : public base::RefCountedThreadSafe<ContentHash> {
   //   - |was_cancelled| Indicates whether or not the request was cancelled
   //     through |is_cancelled|, while it was being processed.
   using CreatedCallback =
-      base::OnceCallback<void(const scoped_refptr<ContentHash>& hash,
+      base::OnceCallback<void(scoped_refptr<ContentHash> hash,
                               bool was_cancelled)>;
   static void Create(const ExtensionKey& key,
                      FetchParams fetch_params,

extensions/browser/content_verify_job.cc

@@ -75,7 +75,7 @@ void ContentVerifyJob::Start(ContentVerifier* verifier) {
 }
 
 void ContentVerifyJob::DidGetContentHashOnIO(
-    const scoped_refptr<const ContentHash>& content_hash) {
+    scoped_refptr<const ContentHash> content_hash) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   base::AutoLock auto_lock(lock_);
   if (g_content_verify_job_test_observer)

extensions/browser/content_verify_job.h

@@ -101,7 +101,7 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   virtual ~ContentVerifyJob();
   friend class base::RefCountedThreadSafe<ContentVerifyJob>;
 
-  void DidGetContentHashOnIO(const scoped_refptr<const ContentHash>& hash);
+  void DidGetContentHashOnIO(scoped_refptr<const ContentHash> hash);
 
   // Same as BytesRead, but is run without acquiring lock.
   void BytesReadImpl(int count, const char* data);