Simplify ContentHashReader lifetime and creation.

Instead of creating CHR and then calling CHR::Init(), create it
in one go (in a blocking allowed thread). The instance is immuatable and
stored inside ContentVerifyJob, hence CHR doens't need to be
RefCountedThreadSafe anymore.

ContentHashReader only needs extension related information on creation,
so move all such params from the class members (to ContentVerifyJob).

Bug: 796395
Change-Id: Ia65938d96e99d5c95562c75920cd614935026240
Reviewed-on: https://chromium-review.googlesource.com/917007Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538994}

extensions/browser/content_hash_reader.cc

@@ -5,6 +5,7 @@
 #include "extensions/browser/content_hash_reader.h"
 
 #include "base/files/file_util.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/timer/elapsed_timer.h"
@@ -17,41 +18,36 @@
 
 namespace extensions {
 
-ContentHashReader::ContentHashReader(const std::string& extension_id,
-                                     const base::Version& extension_version,
-                                     const base::FilePath& extension_root,
-                                     const base::FilePath& relative_path,
-                                     const ContentVerifierKey& key)
-    : extension_id_(extension_id),
-      extension_version_(extension_version.GetString()),
-      extension_root_(extension_root),
-      relative_path_(relative_path),
-      key_(key) {}
-
-ContentHashReader::~ContentHashReader() {
-}
+ContentHashReader::ContentHashReader() {}
+
+ContentHashReader::~ContentHashReader() {}
 
-bool ContentHashReader::Init() {
+// static.
+std::unique_ptr<const ContentHashReader> ContentHashReader::Create(
+    const ExtensionId& extension_id,
+    const base::Version& extension_version,
+    const base::FilePath& extension_root,
+    const base::FilePath& relative_path,
+    const ContentVerifierKey& key) {
   base::ElapsedTimer timer;
-  DCHECK_EQ(status_, NOT_INITIALIZED);
-  status_ = FAILURE;
 
   std::unique_ptr<ContentHash> content_hash =
       ContentHash::Create(ContentHash::ExtensionKey(
-          extension_id_, extension_root_, extension_version_, key_));
+          extension_id, extension_root, extension_version, key));
+  auto hash_reader = base::WrapUnique(new ContentHashReader);
 
   if (!content_hash->succeeded())
-    return false;
+    return hash_reader;  // FAILURE.
 
-  has_content_hashes_ = true;
+  hash_reader->has_content_hashes_ = true;
 
   const VerifiedContents& verified_contents = content_hash->verified_contents();
 
   // Extensions sometimes request resources that do not have an entry in
   // verified_contents.json. This can happen when an extension sends an XHR to a
   // resource.
-  if (!verified_contents.HasTreeHashRoot(relative_path_)) {
-    base::FilePath full_path = extension_root_.Append(relative_path_);
+  if (!verified_contents.HasTreeHashRoot(relative_path)) {
+    base::FilePath full_path = extension_root.Append(relative_path);
     // Making a request to a non-existent file or to a directory should not
     // result in content verification failure.
     // TODO(proberge): This logic could be simplified if |content_verify_job|
@@ -59,35 +55,34 @@ bool ContentHashReader::Init() {
     // A content verification failure should be triggered if there is a mismatch
     // between the file read state and the existence of verification hashes.
     if (!base::PathExists(full_path) || base::DirectoryExists(full_path))
-      file_missing_from_verified_contents_ = true;
+      hash_reader->file_missing_from_verified_contents_ = true;
 
-    return false;
+    return hash_reader;  // FAILURE.
   }
 
   const ComputedHashes::Reader& reader = content_hash->computed_hashes();
-  if (!reader.GetHashes(relative_path_, &block_size_, &hashes_) ||
-      block_size_ % crypto::kSHA256Length != 0) {
-    return false;
+  if (!reader.GetHashes(relative_path, &hash_reader->block_size_,
+                        &hash_reader->hashes_) ||
+      hash_reader->block_size_ % crypto::kSHA256Length != 0) {
+    return hash_reader;
   }
 
-  std::string root =
-      ComputeTreeHashRoot(hashes_, block_size_ / crypto::kSHA256Length);
-  if (!verified_contents.TreeHashRootEquals(relative_path_, root))
-    return false;
+  std::string root = ComputeTreeHashRoot(
+      hash_reader->hashes_, hash_reader->block_size_ / crypto::kSHA256Length);
+  if (!verified_contents.TreeHashRootEquals(relative_path, root))
+    return hash_reader;
 
-  status_ = SUCCESS;
+  hash_reader->status_ = SUCCESS;
   UMA_HISTOGRAM_TIMES("ExtensionContentHashReader.InitLatency",
                       timer.Elapsed());
-  return true;
+  return hash_reader;  // SUCCESS.
 }
 
 int ContentHashReader::block_count() const {
-  DCHECK(status_ != NOT_INITIALIZED);
   return hashes_.size();
 }
 
 int ContentHashReader::block_size() const {
-  DCHECK(status_ != NOT_INITIALIZED);
   return block_size_;
 }
 

extensions/browser/content_hash_reader.h

@@ -13,35 +13,35 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/version.h"
-#include "extensions/browser/content_verifier_delegate.h"
+#include "extensions/browser/content_verifier/content_verifier_key.h"
+#include "extensions/common/extension_id.h"
 
 namespace extensions {
 
 // This class creates an object that will read expected hashes that may have
 // been fetched/calculated by the ContentHashFetcher, and vends them out for
 // use in ContentVerifyJob's.
-class ContentHashReader : public base::RefCountedThreadSafe<ContentHashReader> {
+class ContentHashReader {
  public:
-  // Create one of these to get expected hashes for the file at |relative_path|
-  // within an extension.
-  ContentHashReader(const std::string& extension_id,
-                    const base::Version& extension_version,
-                    const base::FilePath& extension_root,
-                    const base::FilePath& relative_path,
-                    const ContentVerifierKey& key);
-
-  const std::string& extension_id() const { return extension_id_; }
-  const base::FilePath& relative_path() const { return relative_path_; }
-
-  // This should be called to initialize this object (reads the expected hashes
-  // from storage, etc.). Must be called on a thread that is allowed to do file
-  // I/O. Returns a boolean indicating success/failure. On failure, this object
-  // should likely be discarded.
-  bool Init();
+  ~ContentHashReader();
+
+  // Factory to create ContentHashReader to get expected hashes for the file at
+  // |relative_path| within an extension.
+  // Must be called on a thread that is allowed to do file I/O. Returns an
+  // instance whose succees/failure can be determined by calling succeeded()
+  // method. On failure, this object should likely be discarded.
+  static std::unique_ptr<const ContentHashReader> Create(
+      const ExtensionId& extension_id,
+      const base::Version& extension_version,
+      const base::FilePath& extension_root,
+      const base::FilePath& relative_path,
+      const ContentVerifierKey& key);
+
+  bool succeeded() const { return status_ == SUCCESS; }
 
   // Returns true if we found valid verified_contents.json and
   // computed_hashes.json files. Note that this can be true even if we didn't
-  // find an entry for |relative_path_| in them.
+  // find an entry for |relative_path| in them.
   bool has_content_hashes() const { return has_content_hashes_; }
   // Returns whether or not this resource's entry exists in
   // verified_contents.json (given that |has_content_hashes_| is true.
@@ -59,18 +59,11 @@ class ContentHashReader : public base::RefCountedThreadSafe<ContentHashReader> {
   bool GetHashForBlock(int block_index, const std::string** result) const;
 
  private:
-  friend class base::RefCountedThreadSafe<ContentHashReader>;
-  virtual ~ContentHashReader();
+  enum InitStatus { SUCCESS, FAILURE };
 
-  enum InitStatus { NOT_INITIALIZED, SUCCESS, FAILURE };
+  ContentHashReader();
 
-  std::string extension_id_;
-  base::Version extension_version_;
-  base::FilePath extension_root_;
-  base::FilePath relative_path_;
-  ContentVerifierKey key_;
-
-  InitStatus status_ = NOT_INITIALIZED;
+  InitStatus status_ = FAILURE;
 
   bool has_content_hashes_ = false;
   bool file_missing_from_verified_contents_ = false;

extensions/browser/content_verifier.cc

@@ -15,7 +15,6 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
 #include "extensions/browser/content_hash_fetcher.h"
-#include "extensions/browser/content_hash_reader.h"
 #include "extensions/browser/content_verifier_delegate.h"
 #include "extensions/browser/content_verifier_io_data.h"
 #include "extensions/browser/extension_registry.h"
@@ -124,8 +123,8 @@ ContentVerifyJob* ContentVerifier::CreateJobFor(
   // TODO(asargent) - we can probably get some good performance wins by having
   // a cache of ContentHashReader's that we hold onto past the end of each job.
   return new ContentVerifyJob(
-      new ContentHashReader(extension_id, data->version, extension_root,
-                            normalized_unix_path, delegate_->GetPublicKey()),
+      extension_id, data->version, extension_root, normalized_unix_path,
+      delegate_->GetPublicKey(),
       base::BindOnce(&ContentVerifier::VerifyFailed, this, extension_id));
 }
 

extensions/browser/content_verify_job.cc

@@ -40,14 +40,22 @@ class ScopedElapsedTimer {
 
 }  // namespace
 
-ContentVerifyJob::ContentVerifyJob(ContentHashReader* hash_reader,
+ContentVerifyJob::ContentVerifyJob(const ExtensionId& extension_id,
+                                   const base::Version& extension_version,
+                                   const base::FilePath& extension_root,
+                                   const base::FilePath& relative_path,
+                                   const ContentVerifierKey& key,
                                    FailureCallback failure_callback)
     : done_reading_(false),
       hashes_ready_(false),
       total_bytes_read_(0),
       current_block_(0),
       current_hash_byte_count_(0),
-      hash_reader_(hash_reader),
+      extension_id_(extension_id),
+      extension_version_(extension_version),
+      extension_root_(extension_root),
+      relative_path_(relative_path),
+      content_verifier_key_(key),
       failure_callback_(std::move(failure_callback)),
       failed_(false) {}
 
@@ -59,12 +67,14 @@ ContentVerifyJob::~ContentVerifyJob() {
 void ContentVerifyJob::Start() {
   base::AutoLock auto_lock(lock_);
   if (g_content_verify_job_test_observer)
-    g_content_verify_job_test_observer->JobStarted(
-        hash_reader_->extension_id(), hash_reader_->relative_path());
+    g_content_verify_job_test_observer->JobStarted(extension_id_,
+                                                   relative_path_);
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-      base::Bind(&ContentHashReader::Init, hash_reader_),
-      base::Bind(&ContentVerifyJob::OnHashesReady, this));
+      base::BindOnce(&ContentHashReader::Create, extension_id_,
+                     extension_version_, extension_root_, relative_path_,
+                     content_verifier_key_),
+      base::BindOnce(&ContentVerifyJob::OnHashesReady, this));
 }
 
 void ContentVerifyJob::BytesRead(int count, const char* data) {
@@ -84,8 +94,8 @@ void ContentVerifyJob::DoneReading() {
     if (!FinishBlock()) {
       DispatchFailureCallback(HASH_MISMATCH);
     } else if (g_content_verify_job_test_observer) {
-      g_content_verify_job_test_observer->JobFinished(
-          hash_reader_->extension_id(), hash_reader_->relative_path(), NONE);
+      g_content_verify_job_test_observer->JobFinished(extension_id_,
+                                                      relative_path_, NONE);
     }
   }
 }
@@ -161,18 +171,19 @@ bool ContentVerifyJob::FinishBlock() {
   return true;
 }
 
-void ContentVerifyJob::OnHashesReady(bool success) {
+void ContentVerifyJob::OnHashesReady(
+    std::unique_ptr<const ContentHashReader> hash_reader) {
+  base::AutoLock auto_lock(lock_);
+  const bool success = hash_reader->succeeded();
+  hash_reader_ = std::move(hash_reader);
+
   if (g_ignore_verification_for_tests)
     return;
-  base::AutoLock auto_lock(lock_);
   if (g_content_verify_job_test_observer) {
-    g_content_verify_job_test_observer->OnHashesReady(
-        hash_reader_->extension_id(), hash_reader_->relative_path(), success);
+    g_content_verify_job_test_observer->OnHashesReady(extension_id_,
+                                                      relative_path_, success);
   }
   if (!success) {
-    // TODO(lazyboy): Make ContentHashReader::Init return an enum instead of
-    // bool. This should make the following checks on |hash_reader_| easier
-    // to digest and will avoid future bugs from creeping up.
     if (!hash_reader_->has_content_hashes()) {
       DispatchFailureCallback(MISSING_ALL_HASHES);
       return;
@@ -181,8 +192,8 @@ void ContentVerifyJob::OnHashesReady(bool success) {
     if (hash_reader_->file_missing_from_verified_contents()) {
       // Ignore verification of non-existent resources.
       if (g_content_verify_job_test_observer) {
-        g_content_verify_job_test_observer->JobFinished(
-            hash_reader_->extension_id(), hash_reader_->relative_path(), NONE);
+        g_content_verify_job_test_observer->JobFinished(extension_id_,
+                                                        relative_path_, NONE);
       }
       return;
     }
@@ -205,8 +216,8 @@ void ContentVerifyJob::OnHashesReady(bool success) {
     if (!FinishBlock()) {
       DispatchFailureCallback(HASH_MISMATCH);
     } else if (g_content_verify_job_test_observer) {
-      g_content_verify_job_test_observer->JobFinished(
-          hash_reader_->extension_id(), hash_reader_->relative_path(), NONE);
+      g_content_verify_job_test_observer->JobFinished(extension_id_,
+                                                      relative_path_, NONE);
     }
   }
 }
@@ -229,14 +240,13 @@ void ContentVerifyJob::DispatchFailureCallback(FailureReason reason) {
   DCHECK(!failed_);
   failed_ = true;
   if (!failure_callback_.is_null()) {
-    VLOG(1) << "job failed for " << hash_reader_->extension_id() << " "
-            << hash_reader_->relative_path().MaybeAsASCII()
-            << " reason:" << reason;
+    VLOG(1) << "job failed for " << extension_id_ << " "
+            << relative_path_.MaybeAsASCII() << " reason:" << reason;
     std::move(failure_callback_).Run(reason);
   }
   if (g_content_verify_job_test_observer) {
-    g_content_verify_job_test_observer->JobFinished(
-        hash_reader_->extension_id(), hash_reader_->relative_path(), reason);
+    g_content_verify_job_test_observer->JobFinished(extension_id_,
+                                                    relative_path_, reason);
   }
 }
 

extensions/browser/content_verify_job.h

@@ -11,9 +11,12 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
+#include "base/version.h"
+#include "extensions/browser/content_verifier/content_verifier_key.h"
 #include "extensions/common/extension_id.h"
 
 namespace base {
@@ -52,7 +55,11 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   using FailureCallback = base::OnceCallback<void(FailureReason)>;
 
   // The |failure_callback| will be called at most once if there was a failure.
-  ContentVerifyJob(ContentHashReader* hash_reader,
+  ContentVerifyJob(const ExtensionId& extension_id,
+                   const base::Version& extension_version,
+                   const base::FilePath& extension_root,
+                   const base::FilePath& relative_path,
+                   const ContentVerifierKey& content_verifier_key,
                    FailureCallback failure_callback);
 
   // This begins the process of getting expected hashes, so it should be called
@@ -106,7 +113,7 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   void DispatchFailureCallback(FailureReason reason);
 
   // Called when our ContentHashReader has finished initializing.
-  void OnHashesReady(bool success);
+  void OnHashesReady(std::unique_ptr<const ContentHashReader> hash_reader);
 
   // Indicates whether the caller has told us they are done calling BytesRead.
   bool done_reading_;
@@ -130,7 +137,16 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   // The number of bytes we've already input into |current_hash_|.
   int current_hash_byte_count_;
 
-  scoped_refptr<ContentHashReader> hash_reader_;
+  // Valid and set after |hashes_ready_| is set to true.
+  std::unique_ptr<const ContentHashReader> hash_reader_;
+
+  // Resource info for this verify job.
+  const ExtensionId extension_id_;
+  const base::Version extension_version_;
+  const base::FilePath extension_root_;
+  const base::FilePath relative_path_;
+
+  const ContentVerifierKey content_verifier_key_;
 
   base::TimeDelta time_spent_;
 

extensions/browser/content_verify_job_unittest.cc

@@ -10,7 +10,6 @@
 #include "base/version.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "extensions/browser/content_hash_reader.h"
 #include "extensions/browser/content_verifier/test_utils.h"
 #include "extensions/browser/extensions_test.h"
 #include "extensions/common/constants.h"
@@ -37,16 +36,6 @@ enum ContentVerifyJobAsyncRunMode {
   kHashesReadyBeforeContentRead,
 };
 
-scoped_refptr<ContentHashReader> CreateContentHashReader(
-    const Extension& extension,
-    const base::FilePath& extension_resource_path) {
-  return base::MakeRefCounted<ContentHashReader>(
-      extension.id(), extension.version(), extension.path(),
-      extension_resource_path,
-      ContentVerifierKey(kWebstoreSignaturesPublicKey,
-                         kWebstoreSignaturesPublicKeySize));
-}
-
 }  // namespace
 
 class ContentVerifyJobUnittest : public ExtensionsTest {
@@ -73,10 +62,11 @@ class ContentVerifyJobUnittest : public ExtensionsTest {
       std::string& resource_contents,
       ContentVerifyJobAsyncRunMode run_mode) {
     TestContentVerifyJobObserver observer(extension.id(), resource_path);
-    scoped_refptr<ContentHashReader> content_hash_reader =
-        CreateContentHashReader(extension, resource_path);
-    scoped_refptr<ContentVerifyJob> verify_job =
-        new ContentVerifyJob(content_hash_reader.get(), base::DoNothing());
+    scoped_refptr<ContentVerifyJob> verify_job = new ContentVerifyJob(
+        extension.id(), extension.version(), extension.path(), resource_path,
+        ContentVerifierKey(kWebstoreSignaturesPublicKey,
+                           kWebstoreSignaturesPublicKeySize),
+        base::DoNothing());
 
     auto run_content_read_step = [](ContentVerifyJob* verify_job,
                                     std::string* resource_contents) {