Add tests to cover the ordering of asynchronous steps in ContentVerifyJob.

ContentVerifyJob has two steps that can run independently of each
other: 1. Reading content hash (OnHashesReady) and 2. Receiving content
bytes (BytesReady and DoneReading). Add tests to make sure the ordering
of those two steps doesn't matter (and avoid future regressions around
this).
Expand existing tests to run these steps without any ordering
guarantee (as is), with step #1 completing before step #2 runs
and vice versa.

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

chrome/browser/extensions/content_verifier_browsertest.cc

@@ -133,12 +133,14 @@ class JobObserver : public ContentVerifyJob::TestObserver {
   bool WaitForExpectedJobs();
 
   // ContentVerifyJob::TestObserver interface
-  void JobStarted(const std::string& extension_id,
+  void JobStarted(const ExtensionId& extension_id,
                   const base::FilePath& relative_path) override;
-
-  void JobFinished(const std::string& extension_id,
+  void JobFinished(const ExtensionId& extension_id,
                    const base::FilePath& relative_path,
                    ContentVerifyJob::FailureReason failure_reason) override;
+  void OnHashesReady(const ExtensionId& extension_id,
+                     const base::FilePath& relative_path,
+                     bool success) override {}
 
  private:
   struct ExpectedResult {
@@ -147,12 +149,10 @@ class JobObserver : public ContentVerifyJob::TestObserver {
     base::FilePath path;
     Result result;
 
-    ExpectedResult(const std::string& extension_id, const base::FilePath& path,
-                   Result result) {
-      this->extension_id = extension_id;
-      this->path = path;
-      this->result = result;
-    }
+    ExpectedResult(const ExtensionId& extension_id,
+                   const base::FilePath& path,
+                   Result result)
+        : extension_id(extension_id), path(path), result(result) {}
   };
   std::list<ExpectedResult> expectations_;
   content::BrowserThread::ID creation_thread_;

chrome/browser/extensions/extension_protocols_unittest.cc

@@ -487,7 +487,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForFileAccessErrors) {
     content::RunAllPendingInMessageLoop();
 
     EXPECT_EQ(net::OK, DoRequest(*extension, kJs));
-    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitAndGetFailureReason());
+    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitForJobFinished());
   }
 
   // chmod -r 1024.js.
@@ -496,8 +496,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForFileAccessErrors) {
     base::FilePath file_path = unzipped_path.AppendASCII(kJs);
     ASSERT_TRUE(base::MakeFileUnreadable(file_path));
     EXPECT_EQ(net::ERR_ACCESS_DENIED, DoRequest(*extension, kJs));
-    EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH,
-              observer.WaitAndGetFailureReason());
+    EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH, observer.WaitForJobFinished());
     // NOTE: In production, hash mismatch would have disabled |extension|, but
     // since UnzipToDirAndLoadExtension() doesn't add the extension to
     // ExtensionRegistry, ChromeContentVerifierDelegate won't disable it.
@@ -511,8 +510,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForFileAccessErrors) {
     base::FilePath file_path = unzipped_path.AppendASCII(kJs);
     ASSERT_TRUE(base::DieFileDie(file_path, false));
     EXPECT_EQ(net::ERR_FILE_NOT_FOUND, DoRequest(*extension, kJs));
-    EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH,
-              observer.WaitAndGetFailureReason());
+    EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH, observer.WaitForJobFinished());
   }
 }
 
@@ -550,7 +548,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForZeroByteFile) {
     content::RunAllPendingInMessageLoop();
 
     EXPECT_EQ(net::OK, DoRequest(*extension, kEmptyJs));
-    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitAndGetFailureReason());
+    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitForJobFinished());
   }
 
   // chmod -r empty.js.
@@ -562,7 +560,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForZeroByteFile) {
     base::FilePath file_path = unzipped_path.AppendASCII(kEmptyJs);
     ASSERT_TRUE(base::MakeFileUnreadable(file_path));
     EXPECT_EQ(net::ERR_ACCESS_DENIED, DoRequest(*extension, kEmptyJs));
-    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitAndGetFailureReason());
+    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitForJobFinished());
   }
 
   // rm empty.js.
@@ -574,7 +572,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForZeroByteFile) {
     base::FilePath file_path = unzipped_path.AppendASCII(kEmptyJs);
     ASSERT_TRUE(base::DieFileDie(file_path, false));
     EXPECT_EQ(net::ERR_FILE_NOT_FOUND, DoRequest(*extension, kEmptyJs));
-    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitAndGetFailureReason());
+    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitForJobFinished());
   }
 }
 

extensions/browser/content_verifier/test_utils.cc

@@ -30,17 +30,33 @@ void TestContentVerifyJobObserver::JobFinished(
     return;
   EXPECT_FALSE(failure_reason_.has_value());
   failure_reason_ = reason;
-  run_loop_.Quit();
+  job_finished_run_loop_.Quit();
+}
+
+void TestContentVerifyJobObserver::OnHashesReady(
+    const ExtensionId& extension_id,
+    const base::FilePath& relative_path,
+    bool success) {
+  if (extension_id != extension_id_ || relative_path != relative_path_)
+    return;
+  EXPECT_FALSE(seen_on_hashes_ready_);
+  seen_on_hashes_ready_ = true;
+  on_hashes_ready_run_loop_.Quit();
 }
 
 ContentVerifyJob::FailureReason
-TestContentVerifyJobObserver::WaitAndGetFailureReason() {
+TestContentVerifyJobObserver::WaitForJobFinished() {
   // Run() returns immediately if Quit() has already been called.
-  run_loop_.Run();
+  job_finished_run_loop_.Run();
   EXPECT_TRUE(failure_reason_.has_value());
   return failure_reason_.value_or(ContentVerifyJob::FAILURE_REASON_MAX);
 }
 
+void TestContentVerifyJobObserver::WaitForOnHashesReady() {
+  // Run() returns immediately if Quit() has already been called.
+  on_hashes_ready_run_loop_.Run();
+}
+
 namespace content_verifier_test_utils {
 
 scoped_refptr<Extension> UnzipToDirAndLoadExtension(

extensions/browser/content_verifier/test_utils.h

@@ -24,21 +24,30 @@ class TestContentVerifyJobObserver : ContentVerifyJob::TestObserver {
                                const base::FilePath& relative_path);
   ~TestContentVerifyJobObserver();
 
-  void JobStarted(const std::string& extension_id,
+  // ContentVerifyJob::TestObserver:
+  void JobStarted(const ExtensionId& extension_id,
                   const base::FilePath& relative_path) override {}
-
-  void JobFinished(const std::string& extension_id,
+  void JobFinished(const ExtensionId& extension_id,
                    const base::FilePath& relative_path,
                    ContentVerifyJob::FailureReason reason) override;
+  void OnHashesReady(const ExtensionId& extension_id,
+                     const base::FilePath& relative_path,
+                     bool success) override;
 
   // Waits for a ContentVerifyJob to finish and returns job's status.
-  ContentVerifyJob::FailureReason WaitAndGetFailureReason();
+  ContentVerifyJob::FailureReason WaitForJobFinished() WARN_UNUSED_RESULT;
+
+  // Waits for ContentVerifyJob to finish the attempt to read content hashes.
+  void WaitForOnHashesReady();
 
  private:
-  base::RunLoop run_loop_;
+  base::RunLoop job_finished_run_loop_;
+  base::RunLoop on_hashes_ready_run_loop_;
+
   ExtensionId extension_id_;
   base::FilePath relative_path_;
   base::Optional<ContentVerifyJob::FailureReason> failure_reason_;
+  bool seen_on_hashes_ready_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestContentVerifyJobObserver);
 };

extensions/browser/content_verify_job.cc

@@ -160,6 +160,10 @@ bool ContentVerifyJob::FinishBlock() {
 void ContentVerifyJob::OnHashesReady(bool success) {
   if (g_ignore_verification_for_tests)
     return;
+  if (g_content_verify_job_test_observer) {
+    g_content_verify_job_test_observer->OnHashesReady(
+        hash_reader_->extension_id(), hash_reader_->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

extensions/browser/content_verify_job.h

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
+#include "extensions/common/extension_id.h"
 
 namespace base {
 class FilePath;
@@ -71,12 +72,16 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
 
   class TestObserver {
    public:
-    virtual void JobStarted(const std::string& extension_id,
+    virtual void JobStarted(const ExtensionId& extension_id,
                             const base::FilePath& relative_path) = 0;
 
-    virtual void JobFinished(const std::string& extension_id,
+    virtual void JobFinished(const ExtensionId& extension_id,
                              const base::FilePath& relative_path,
                              FailureReason failure_reason) = 0;
+
+    virtual void OnHashesReady(const ExtensionId& extension_id,
+                               const base::FilePath& relative_path,
+                               bool success) = 0;
   };
 
   static void SetIgnoreVerificationForTests(bool value);