Remove ContentVerifyJob::TestDelegate.

The delegate used to create separate code paths in ContentVerifyJob,
which makes the code harder to read and also creates a parallel
reality in tests. Remove it entirely since tests using the delegate
can work with the TestObserver in ContentVerifyJob.

Remove two tests: ContentVerifierTest:FailOnRead/FailOnDone, that
heavily used the TestDelegate. These tests were *artificially*
(different from how production code does) creating error conditions
from ContentVerifyJob::BytesRead and ContentVerifyJob::DoneReading.

Extract test utilities related to content verification to a
separate file (e/b/content_verifier/test_utils.h)

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

chrome/browser/extensions/content_verifier_browsertest.cc

@@ -22,7 +22,6 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/policy_extension_reinstaller.h"
 #include "chrome/common/chrome_switches.h"
-#include "chrome/test/base/ui_test_utils.h"
 #include "components/policy/core/browser/browser_policy_connector.h"
 #include "components/policy/core/common/mock_configuration_policy_provider.h"
 #include "content/public/common/browser_side_navigation_policy.h"
@@ -116,56 +115,6 @@ class RegistryObserver : public ExtensionRegistryObserver {
   DISALLOW_COPY_AND_ASSIGN(RegistryObserver);
 };
 
-// Helper for forcing ContentVerifyJob's to return an error.
-class JobDelegate : public ContentVerifyJob::TestDelegate {
- public:
-  JobDelegate()
-      : fail_next_read_(false),
-        fail_next_done_(false),
-        bytes_read_failed_(0),
-        done_reading_failed_(0) {}
-
-  ~JobDelegate() override {}
-
-  void set_id(const ExtensionId& id) { id_ = id; }
-  void fail_next_read() { fail_next_read_ = true; }
-  void fail_next_done() { fail_next_done_ = true; }
-
-  // Return the number of BytesRead/DoneReading calls we actually failed,
-  // respectively.
-  int bytes_read_failed() { return bytes_read_failed_; }
-  int done_reading_failed() { return done_reading_failed_; }
-
-  ContentVerifyJob::FailureReason BytesRead(const ExtensionId& id,
-                                            int count,
-                                            const char* data) override {
-    if (id == id_ && fail_next_read_) {
-      fail_next_read_ = false;
-      bytes_read_failed_++;
-      return ContentVerifyJob::HASH_MISMATCH;
-    }
-    return ContentVerifyJob::NONE;
-  }
-
-  ContentVerifyJob::FailureReason DoneReading(const ExtensionId& id) override {
-    if (id == id_ && fail_next_done_) {
-      fail_next_done_ = false;
-      done_reading_failed_++;
-      return ContentVerifyJob::HASH_MISMATCH;
-    }
-    return ContentVerifyJob::NONE;
-  }
-
- private:
-  ExtensionId id_;
-  bool fail_next_read_;
-  bool fail_next_done_;
-  int bytes_read_failed_;
-  int done_reading_failed_;
-
-  DISALLOW_COPY_AND_ASSIGN(JobDelegate);
-};
-
 class JobObserver : public ContentVerifyJob::TestObserver {
  public:
   JobObserver();
@@ -449,19 +398,6 @@ class ForceInstallProvider : public ManagementPolicy::Provider {
   DISALLOW_COPY_AND_ASSIGN(ForceInstallProvider);
 };
 
-class ScopedContentVerifyJobDelegateOverride {
- public:
-  explicit ScopedContentVerifyJobDelegateOverride(JobDelegate* delegate) {
-    ContentVerifyJob::SetDelegateForTests(delegate);
-  }
-  ~ScopedContentVerifyJobDelegateOverride() {
-    ContentVerifyJob::SetDelegateForTests(nullptr);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ScopedContentVerifyJobDelegateOverride);
-};
-
 }  // namespace
 
 class ContentVerifierTest : public ExtensionBrowserTest {
@@ -478,32 +414,6 @@ class ContentVerifierTest : public ExtensionBrowserTest {
 
   bool ShouldEnableContentVerification() override { return true; }
 
-  virtual void OpenPageAndWaitForUnload() {
-    ScopedContentVerifyJobDelegateOverride scoped_delegate(&delegate_);
-    std::string id = "npnbmohejbjohgpjnmjagbafnjhkmgko";
-    delegate_.set_id(id);
-
-    // |unload_observer| needs to destroy before the ExtensionRegistry gets
-    // deleted, which happens before TearDownOnMainThread is called.
-    RegistryObserver unload_observer(ExtensionRegistry::Get(profile()));
-    const Extension* extension = InstallExtensionFromWebstore(
-        test_data_dir_.AppendASCII("content_verifier/v1.crx"), 1);
-    ASSERT_TRUE(extension);
-    ASSERT_EQ(id, extension->id());
-    page_url_ = extension->GetResourceURL("page.html");
-    // Wait for 0 navigations to complete because with PlzNavigate it's racy
-    // when the didstop IPC arrives relative to the tab being closed. The
-    // wait call below is what the tests care about.
-    ui_test_utils::NavigateToURLWithDispositionBlockUntilNavigationsComplete(
-        browser(), page_url_, 0, WindowOpenDisposition::NEW_FOREGROUND_TAB,
-        ui_test_utils::BROWSER_TEST_NONE);
-
-    EXPECT_TRUE(unload_observer.WaitForUnload(id));
-    ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
-    int reasons = prefs->GetDisableReasons(id);
-    EXPECT_TRUE(reasons & disable_reason::DISABLE_CORRUPTED);
-  }
-
   void TestContentScriptExtension(const std::string& crx_relpath,
                                   const std::string& id,
                                   const std::string& script_relpath) {
@@ -553,24 +463,9 @@ class ContentVerifierTest : public ExtensionBrowserTest {
   }
 
  protected:
-  JobDelegate delegate_;
   GURL page_url_;
 };
 
-IN_PROC_BROWSER_TEST_F(ContentVerifierTest, FailOnRead) {
-  EXPECT_EQ(0, delegate_.bytes_read_failed());
-  delegate_.fail_next_read();
-  OpenPageAndWaitForUnload();
-  EXPECT_EQ(1, delegate_.bytes_read_failed());
-}
-
-IN_PROC_BROWSER_TEST_F(ContentVerifierTest, FailOnDone) {
-  EXPECT_EQ(0, delegate_.done_reading_failed());
-  delegate_.fail_next_done();
-  OpenPageAndWaitForUnload();
-  EXPECT_EQ(1, delegate_.done_reading_failed());
-}
-
 IN_PROC_BROWSER_TEST_F(ContentVerifierTest, DotSlashPaths) {
   JobObserver job_observer;
   std::string id = "hoipipabpcoomfapcecilckodldhmpgl";

chrome/browser/extensions/extension_protocols_unittest.cc

@@ -29,11 +29,13 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/content_verifier.h"
+#include "extensions/browser/content_verifier/test_utils.h"
 #include "extensions/browser/extension_protocols.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
+#include "extensions/common/extension_paths.h"
 #include "extensions/common/file_util.h"
 #include "extensions/test/test_extension_dir.h"
 #include "net/base/request_priority.h"
@@ -55,14 +57,11 @@ base::FilePath GetTestPath(const std::string& name) {
   return path.AppendASCII("extensions").AppendASCII(name);
 }
 
-// Helper function that creates a file at |relative_path| within |directory|
-// and fills it with |content|.
-bool AddFileToDirectory(const base::FilePath& directory,
-                        const base::FilePath& relative_path,
-                        const std::string& content) {
-  base::FilePath full_path = directory.Append(relative_path);
-  int result = base::WriteFile(full_path, content.data(), content.size());
-  return static_cast<size_t>(result) == content.size();
+base::FilePath GetContentVerifierTestPath() {
+  base::FilePath path;
+  EXPECT_TRUE(PathService::Get(extensions::DIR_TEST_DATA, &path));
+  return path.AppendASCII("content_hash_fetcher")
+      .AppendASCII("different_sized_files");
 }
 
 scoped_refptr<Extension> CreateTestExtension(const std::string& name,
@@ -120,60 +119,6 @@ scoped_refptr<Extension> CreateTestResponseHeaderExtension() {
   return extension;
 }
 
-// A ContentVerifyJob::TestDelegate that observes DoneReading().
-class JobDelegate : public ContentVerifyJob::TestDelegate {
- public:
-  explicit JobDelegate(const std::string& expected_contents)
-      : expected_contents_(expected_contents), run_loop_(new base::RunLoop()) {
-    ContentVerifyJob::SetDelegateForTests(this);
-  }
-  ~JobDelegate() override { ContentVerifyJob::SetDelegateForTests(nullptr); }
-
-  ContentVerifyJob::FailureReason BytesRead(const ExtensionId& id,
-                                            int count,
-                                            const char* data) override {
-    read_contents_.append(data, count);
-    return ContentVerifyJob::NONE;
-  }
-
-  ContentVerifyJob::FailureReason DoneReading(const ExtensionId& id) override {
-    seen_done_reading_extension_ids_.insert(id);
-    if (waiting_for_extension_id_ == id)
-      run_loop_->Quit();
-
-    if (!base::StartsWith(expected_contents_, read_contents_,
-                          base::CompareCase::SENSITIVE)) {
-      ADD_FAILURE() << "Unexpected read, expected: " << expected_contents_
-                    << ", but found: " << read_contents_;
-    }
-    return ContentVerifyJob::NONE;
-  }
-
-  void WaitForDoneReading(const ExtensionId& id) {
-    ASSERT_FALSE(waiting_for_extension_id_);
-    if (seen_done_reading_extension_ids_.count(id))
-      return;
-    waiting_for_extension_id_ = id;
-    run_loop_->Run();
-  }
-
-  void Reset() {
-    read_contents_.clear();
-    waiting_for_extension_id_.reset();
-    seen_done_reading_extension_ids_.clear();
-    run_loop_ = std::make_unique<base::RunLoop>();
-  }
-
- private:
-  std::string expected_contents_;
-  std::string read_contents_;
-  std::set<ExtensionId> seen_done_reading_extension_ids_;
-  base::Optional<ExtensionId> waiting_for_extension_id_;
-  std::unique_ptr<base::RunLoop> run_loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(JobDelegate);
-};
-
 }  // namespace
 
 // This test lives in src/chrome instead of src/extensions because it tests
@@ -516,101 +461,121 @@ TEST_F(ExtensionProtocolsTest, MetadataFolder) {
 // Tests that unreadable files and deleted files correctly go through
 // ContentVerifyJob.
 TEST_F(ExtensionProtocolsTest, VerificationSeenForFileAccessErrors) {
-  const char kFooJsContents[] = "hello world.";
-  JobDelegate test_job_delegate(kFooJsContents);
   SetProtocolHandler(false);
 
-  const std::string kFooJs("foo.js");
-  // Create a temporary directory that a fake extension will live in and fill
-  // it with some test files.
+  // Unzip extension containing verification hashes to a temporary directory.
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-  base::FilePath foo_js(FILE_PATH_LITERAL("foo.js"));
-  ASSERT_TRUE(AddFileToDirectory(temp_dir.GetPath(), foo_js, kFooJsContents))
-      << "Failed to write " << temp_dir.GetPath().value() << "/"
-      << foo_js.value();
-
-  ExtensionBuilder builder;
-  builder
-      .SetManifest(DictionaryBuilder()
-                       .Set("name", "Foo")
-                       .Set("version", "1.0")
-                       .Set("manifest_version", 2)
-                       .Set("update_url",
-                            "https://clients2.google.com/service/update2/crx")
-                       .Build())
-      .SetID(crx_file::id_util::GenerateId("whatever"))
-      .SetPath(temp_dir.GetPath())
-      .SetLocation(Manifest::INTERNAL);
-  scoped_refptr<Extension> extension(builder.Build());
-
+  base::FilePath unzipped_path = temp_dir.GetPath();
+  scoped_refptr<Extension> extension =
+      content_verifier_test_utils::UnzipToDirAndLoadExtension(
+          GetContentVerifierTestPath().AppendASCII("source.zip"),
+          unzipped_path);
   ASSERT_TRUE(extension.get());
-  content_verifier_->OnExtensionLoaded(testing_profile_.get(), extension.get());
+  ExtensionId extension_id = extension->id();
+
+  const std::string kJs("1024.js");
+  base::FilePath kRelativePath(FILE_PATH_LITERAL("1024.js"));
+
+  // Valid and readable 1024.js.
+  {
+    TestContentVerifyJobObserver observer(extension_id, kRelativePath);
+
+    content_verifier_->OnExtensionLoaded(testing_profile_.get(),
+                                         extension.get());
     // Wait for PostTask to ContentVerifierIOData::AddData() to finish.
     content::RunAllPendingInMessageLoop();
 
-  // Valid and readable foo.js.
-  EXPECT_EQ(net::OK, DoRequest(*extension, kFooJs));
-  test_job_delegate.WaitForDoneReading(extension->id());
-
-  // chmod -r foo.js.
-  base::FilePath foo_path = temp_dir.GetPath().AppendASCII(kFooJs);
-  ASSERT_TRUE(base::MakeFileUnreadable(foo_path));
-  test_job_delegate.Reset();
-  EXPECT_EQ(net::ERR_ACCESS_DENIED, DoRequest(*extension, kFooJs));
-  test_job_delegate.WaitForDoneReading(extension->id());
-
-  // Delete foo.js.
-  ASSERT_TRUE(base::DieFileDie(foo_path, false));
-  test_job_delegate.Reset();
-  EXPECT_EQ(net::ERR_FILE_NOT_FOUND, DoRequest(*extension, kFooJs));
-  test_job_delegate.WaitForDoneReading(extension->id());
+    EXPECT_EQ(net::OK, DoRequest(*extension, kJs));
+    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitAndGetFailureReason());
+  }
+
+  // chmod -r 1024.js.
+  {
+    TestContentVerifyJobObserver observer(extension->id(), kRelativePath);
+    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());
+    // NOTE: In production, hash mismatch would have disabled |extension|, but
+    // since UnzipToDirAndLoadExtension() doesn't add the extension to
+    // ExtensionRegistry, ChromeContentVerifierDelegate won't disable it.
+    // TODO(lazyboy): We may want to update this to more closely reflect the
+    // real flow.
+  }
+
+  // Delete 1024.js.
+  {
+    TestContentVerifyJobObserver observer(extension_id, kRelativePath);
+    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());
+  }
 }
 
 // Tests that zero byte files correctly go through ContentVerifyJob.
 TEST_F(ExtensionProtocolsTest, VerificationSeenForZeroByteFile) {
-  const char kFooJsContents[] = "";  // Empty.
-  JobDelegate test_job_delegate(kFooJsContents);
   SetProtocolHandler(false);
 
-  const std::string kFooJs("foo.js");
-  // Create a temporary directory that a fake extension will live in and fill
-  // it with some test files.
+  const std::string kEmptyJs("empty.js");
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-  base::FilePath foo_js(FILE_PATH_LITERAL("foo.js"));
-  ASSERT_TRUE(AddFileToDirectory(temp_dir.GetPath(), foo_js, kFooJsContents))
-      << "Failed to write " << temp_dir.GetPath().value() << "/"
-      << foo_js.value();
+  base::FilePath unzipped_path = temp_dir.GetPath();
+
+  scoped_refptr<Extension> extension =
+      content_verifier_test_utils::UnzipToDirAndLoadExtension(
+          GetContentVerifierTestPath().AppendASCII("source.zip"),
+          unzipped_path);
+  ASSERT_TRUE(extension.get());
 
-  // Sanity check foo.js.
-  base::FilePath foo_path = temp_dir.GetPath().AppendASCII(kFooJs);
+  base::FilePath kRelativePath(FILE_PATH_LITERAL("empty.js"));
+  ExtensionId extension_id = extension->id();
+
+  // Sanity check empty.js.
+  base::FilePath file_path = unzipped_path.AppendASCII(kEmptyJs);
   int64_t foo_file_size = -1;
-  ASSERT_TRUE(base::GetFileSize(foo_path, &foo_file_size));
+  ASSERT_TRUE(base::GetFileSize(file_path, &foo_file_size));
   ASSERT_EQ(0, foo_file_size);
 
-  ExtensionBuilder builder;
-  builder
-      .SetManifest(DictionaryBuilder()
-                       .Set("name", "Foo")
-                       .Set("version", "1.0")
-                       .Set("manifest_version", 2)
-                       .Set("update_url",
-                            "https://clients2.google.com/service/update2/crx")
-                       .Build())
-      .SetID(crx_file::id_util::GenerateId("whatever"))
-      .SetPath(temp_dir.GetPath())
-      .SetLocation(Manifest::INTERNAL);
-  scoped_refptr<Extension> extension(builder.Build());
+  // Request empty.js.
+  {
+    TestContentVerifyJobObserver observer(extension_id, kRelativePath);
 
-  ASSERT_TRUE(extension.get());
-  content_verifier_->OnExtensionLoaded(testing_profile_.get(), extension.get());
+    content_verifier_->OnExtensionLoaded(testing_profile_.get(),
+                                         extension.get());
     // Wait for PostTask to ContentVerifierIOData::AddData() to finish.
     content::RunAllPendingInMessageLoop();
 
-  // Request foo.js.
-  EXPECT_EQ(net::OK, DoRequest(*extension, kFooJs));
-  test_job_delegate.WaitForDoneReading(extension->id());
+    EXPECT_EQ(net::OK, DoRequest(*extension, kEmptyJs));
+    EXPECT_EQ(ContentVerifyJob::NONE, observer.WaitAndGetFailureReason());
+  }
+
+  // chmod -r empty.js.
+  // Unreadable empty file doesn't generate hash mismatch. Note that this is the
+  // current behavior of ContentVerifyJob.
+  // TODO(lazyboy): The behavior is probably incorrect.
+  {
+    TestContentVerifyJobObserver observer(extension->id(), kRelativePath);
+    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());
+  }
+
+  // rm empty.js.
+  // Deleted empty file doesn't generate hash mismatch. Note that this is the
+  // current behavior of ContentVerifyJob.
+  // TODO(lazyboy): The behavior is probably incorrect.
+  {
+    TestContentVerifyJobObserver observer(extension_id, kRelativePath);
+    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());
+  }
 }
 
 // Tests that mime types are properly set for returned extension resources.

extensions/BUILD.gn

@@ -71,6 +71,8 @@ static_library("test_support") {
     "browser/api_unittest.h",
     "browser/app_window/test_app_window_contents.cc",
     "browser/app_window/test_app_window_contents.h",
+    "browser/content_verifier/test_utils.cc",
+    "browser/content_verifier/test_utils.h",
     "browser/extension_error_test_util.cc",
     "browser/extension_error_test_util.h",
     "browser/extensions_test.cc",

extensions/browser/content_verifier/test_utils.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/browser/content_verifier/test_utils.h"
+
+#include "extensions/common/extension.h"
+#include "extensions/common/file_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/zlib/google/zip.h"
+
+namespace extensions {
+
+TestContentVerifyJobObserver::TestContentVerifyJobObserver(
+    const ExtensionId& extension_id,
+    const base::FilePath& relative_path)
+    : extension_id_(extension_id), relative_path_(relative_path) {
+  ContentVerifyJob::SetObserverForTests(this);
+}
+
+TestContentVerifyJobObserver::~TestContentVerifyJobObserver() {
+  ContentVerifyJob::SetObserverForTests(nullptr);
+}
+
+void TestContentVerifyJobObserver::JobFinished(
+    const ExtensionId& extension_id,
+    const base::FilePath& relative_path,
+    ContentVerifyJob::FailureReason reason) {
+  if (extension_id != extension_id_ || relative_path != relative_path_)
+    return;
+  EXPECT_FALSE(failure_reason_.has_value());
+  failure_reason_ = reason;
+  run_loop_.Quit();
+}
+
+ContentVerifyJob::FailureReason
+TestContentVerifyJobObserver::WaitAndGetFailureReason() {
+  // Run() returns immediately if Quit() has already been called.
+  run_loop_.Run();
+  EXPECT_TRUE(failure_reason_.has_value());
+  return failure_reason_.value_or(ContentVerifyJob::FAILURE_REASON_MAX);
+}
+
+namespace content_verifier_test_utils {
+
+scoped_refptr<Extension> UnzipToDirAndLoadExtension(
+    const base::FilePath& extension_zip,
+    const base::FilePath& unzip_dir) {
+  if (!zip::Unzip(extension_zip, unzip_dir)) {
+    ADD_FAILURE() << "Failed to unzip path.";
+    return nullptr;
+  }
+  std::string error;
+  scoped_refptr<Extension> extension = file_util::LoadExtension(
+      unzip_dir, Manifest::INTERNAL, 0 /* flags */, &error);
+  EXPECT_NE(nullptr, extension.get()) << " error:'" << error << "'";
+  return extension;
+}
+
+}  // namespace content_verifier_test_utils
+
+}  // namespace extensions

extensions/browser/content_verifier/test_utils.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_BROWSER_CONTENT_VERIFIER_TEST_UTILS_H_
+#define EXTENSIONS_BROWSER_CONTENT_VERIFIER_TEST_UTILS_H_
+
+#include "base/files/file_path.h"
+#include "base/optional.h"
+#include "base/run_loop.h"
+#include "extensions/browser/content_verify_job.h"
+#include "extensions/common/extension_id.h"
+
+namespace extensions {
+
+class Extension;
+
+// Test class to observe a particular extension resource's ContentVerifyJob
+// lifetime.  Provides a way to wait for a job to finish and return
+// the job's result.
+class TestContentVerifyJobObserver : ContentVerifyJob::TestObserver {
+ public:
+  TestContentVerifyJobObserver(const ExtensionId& extension_id,
+                               const base::FilePath& relative_path);
+  ~TestContentVerifyJobObserver();
+
+  void JobStarted(const std::string& extension_id,
+                  const base::FilePath& relative_path) override {}
+
+  void JobFinished(const std::string& extension_id,
+                   const base::FilePath& relative_path,
+                   ContentVerifyJob::FailureReason reason) override;
+
+  // Waits for a ContentVerifyJob to finish and returns job's status.
+  ContentVerifyJob::FailureReason WaitAndGetFailureReason();
+
+ private:
+  base::RunLoop run_loop_;
+  ExtensionId extension_id_;
+  base::FilePath relative_path_;
+  base::Optional<ContentVerifyJob::FailureReason> failure_reason_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestContentVerifyJobObserver);
+};
+
+namespace content_verifier_test_utils {
+
+// Unzips the extension source from |extension_zip| into |unzip_dir|
+// directory and loads it. Returns the resulting Extension object.
+// |destination| points to the path where the extension was extracted.
+//
+// TODO(lazyboy): Move this function to a generic file.
+scoped_refptr<Extension> UnzipToDirAndLoadExtension(
+    const base::FilePath& extension_zip,
+    const base::FilePath& unzip_dir);
+
+}  // namespace content_verifier_test_utils
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_CONTENT_VERIFIER_TEST_UTILS_H_

extensions/browser/content_verify_job.cc

@@ -18,7 +18,7 @@ namespace extensions {
 
 namespace {
 
-ContentVerifyJob::TestDelegate* g_test_delegate = NULL;
+bool g_ignore_verification_for_tests = false;
 ContentVerifyJob::TestObserver* g_content_verify_job_test_observer = NULL;
 
 class ScopedElapsedTimer {
@@ -72,13 +72,8 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
   base::AutoLock auto_lock(lock_);
   if (failed_)
     return;
-  if (g_test_delegate) {
-    FailureReason reason =
-        g_test_delegate->BytesRead(hash_reader_->extension_id(), count, data);
-    if (reason != NONE)
-      DispatchFailureCallback(reason);
+  if (g_ignore_verification_for_tests)
     return;
-  }
   if (!hashes_ready_) {
     queue_.append(data, count);
     return;
@@ -119,13 +114,8 @@ void ContentVerifyJob::DoneReading() {
   base::AutoLock auto_lock(lock_);
   if (failed_)
     return;
-  if (g_test_delegate) {
-    FailureReason reason =
-        g_test_delegate->DoneReading(hash_reader_->extension_id());
-    if (reason != NONE)
-      DispatchFailureCallback(reason);
+  if (g_ignore_verification_for_tests)
     return;
-  }
   done_reading_ = true;
   if (hashes_ready_) {
     if (!FinishBlock()) {
@@ -167,7 +157,9 @@ bool ContentVerifyJob::FinishBlock() {
 }
 
 void ContentVerifyJob::OnHashesReady(bool success) {
-  if (!success && !g_test_delegate) {
+  if (g_ignore_verification_for_tests)
+    return;
+  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.
@@ -206,15 +198,16 @@ void ContentVerifyJob::OnHashesReady(bool success) {
 }
 
 // static
-void ContentVerifyJob::SetDelegateForTests(TestDelegate* delegate) {
-  DCHECK(delegate == nullptr || g_test_delegate == nullptr)
-      << "SetDelegateForTests does not support interleaving. Delegates should "
-      << "be set and then cleared one at a time.";
-  g_test_delegate = delegate;
+void ContentVerifyJob::SetIgnoreVerificationForTests(bool value) {
+  DCHECK_NE(g_ignore_verification_for_tests, value);
+  g_ignore_verification_for_tests = value;
 }
 
 // static
 void ContentVerifyJob::SetObserverForTests(TestObserver* observer) {
+  DCHECK(observer == nullptr || g_content_verify_job_test_observer == nullptr)
+      << "SetObserverForTests does not support interleaving. Observers should "
+      << "be set and then cleared one at a time.";
   g_content_verify_job_test_observer = observer;
 }
 

extensions/browser/content_verify_job.h

@@ -69,19 +69,6 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   // Call once when finished adding bytes via BytesRead.
   void DoneReading();
 
-  class TestDelegate {
-   public:
-    virtual ~TestDelegate() {}
-
-    // These methods will be called inside BytesRead/DoneReading respectively.
-    // If either return something other than NONE, then the failure callback
-    // will be dispatched with that reason.
-    virtual FailureReason BytesRead(const std::string& extension_id,
-                                    int count,
-                                    const char* data) = 0;
-    virtual FailureReason DoneReading(const std::string& extension_id) = 0;
-  };
-
   class TestObserver {
    public:
     virtual void JobStarted(const std::string& extension_id,
@@ -92,9 +79,9 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
                              FailureReason failure_reason) = 0;
   };
 
-  // Note: having interleaved delegates is not supported.
-  static void SetDelegateForTests(TestDelegate* delegate);
+  static void SetIgnoreVerificationForTests(bool value);
 
+  // Note: having interleaved observer is not supported.
   static void SetObserverForTests(TestObserver* observer);
 
  private:

extensions/browser/content_verify_job_unittest.cc

@@ -7,11 +7,11 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ptr_util.h"
 #include "base/path_service.h"
-#include "base/run_loop.h"
 #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"
 #include "extensions/common/extension_paths.h"
@@ -35,43 +35,6 @@ scoped_refptr<ContentHashReader> CreateContentHashReader(
 
 void DoNothingWithReasonParam(ContentVerifyJob::FailureReason reason) {}
 
-class JobTestObserver : public ContentVerifyJob::TestObserver {
- public:
-  JobTestObserver(const std::string& extension_id,
-                  const base::FilePath& relative_path)
-      : extension_id_(extension_id), relative_path_(relative_path) {
-    ContentVerifyJob::SetObserverForTests(this);
-  }
-  ~JobTestObserver() { ContentVerifyJob::SetObserverForTests(nullptr); }
-
-  void JobStarted(const std::string& extension_id,
-                  const base::FilePath& relative_path) override {}
-
-  void JobFinished(const std::string& extension_id,
-                   const base::FilePath& relative_path,
-                   ContentVerifyJob::FailureReason reason) override {
-    if (extension_id != extension_id_ || relative_path != relative_path_)
-      return;
-    failure_reason_ = reason;
-    run_loop_.Quit();
-  }
-
-  ContentVerifyJob::FailureReason WaitAndGetFailureReason() {
-    // Run() returns immediately if Quit() has already been called.
-    run_loop_.Run();
-    EXPECT_TRUE(failure_reason_.has_value());
-    return failure_reason_.value_or(ContentVerifyJob::FAILURE_REASON_MAX);
-  }
-
- private:
-  base::RunLoop run_loop_;
-  std::string extension_id_;
-  base::FilePath relative_path_;
-  base::Optional<ContentVerifyJob::FailureReason> failure_reason_;
-
-  DISALLOW_COPY_AND_ASSIGN(JobTestObserver);
-};
-
 }  // namespace
 
 class ContentVerifyJobUnittest : public ExtensionsTest {
@@ -91,29 +54,12 @@ class ContentVerifyJobUnittest : public ExtensionsTest {
     return base_path.Append(relative_path);
   }
 
-  // Unzips the extension source from |extension_zip| into a temporary
-  // directory and loads it. Returns the resuling Extension object.
-  // |destination| points to the path where the extension was extracted.
-  scoped_refptr<Extension> UnzipToTempDirAndLoad(
-      const base::FilePath& extension_zip,
-      base::FilePath* destination) {
-    EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
-    *destination = temp_dir_.GetPath();
-    EXPECT_TRUE(zip::Unzip(extension_zip, *destination));
-
-    std::string error;
-    scoped_refptr<Extension> extension = file_util::LoadExtension(
-        *destination, Manifest::INTERNAL, 0 /* flags */, &error);
-    EXPECT_NE(nullptr, extension.get()) << " error:'" << error << "'";
-    return extension;
-  }
-
  protected:
   ContentVerifyJob::FailureReason RunContentVerifyJob(
       const Extension& extension,
       const base::FilePath& resource_path,
       std::string& resource_contents) {
-    JobTestObserver observer(extension.id(), resource_path);
+    TestContentVerifyJobObserver observer(extension.id(), resource_path);
     scoped_refptr<ContentHashReader> content_hash_reader =
         CreateContentHashReader(extension, resource_path);
     scoped_refptr<ContentVerifyJob> verify_job = new ContentVerifyJob(
@@ -128,9 +74,43 @@ class ContentVerifyJobUnittest : public ExtensionsTest {
     return observer.WaitAndGetFailureReason();
   }
 
- private:
+  // Runs test to verify that a modified extension resource (background.js)
+  // causes ContentVerifyJob to fail with HASH_MISMATCH. The string
+  // |content_to_append_for_mismatch| is appended to the resource for
+  // modification.
+  void RunContentMismatchTest(
+      const std::string& content_to_append_for_mismatch) {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    base::FilePath unzipped_path = temp_dir_.GetPath();
+    base::FilePath test_dir_base = GetTestPath(
+        base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));
+    scoped_refptr<Extension> extension =
+        content_verifier_test_utils::UnzipToDirAndLoadExtension(
+            test_dir_base.AppendASCII("source_all.zip"), unzipped_path);
+    ASSERT_TRUE(extension.get());
+    // Make sure there is a verified_contents.json file there as this test
+    // cannot fetch it.
+    EXPECT_TRUE(base::PathExists(
+        file_util::GetVerifiedContentsPath(extension->path())));
+
+    const base::FilePath::CharType kResource[] =
+        FILE_PATH_LITERAL("background.js");
+    base::FilePath existent_resource_path(kResource);
+    {
+      // Make sure modified background.js fails content verification.
+      std::string modified_contents;
+      base::ReadFileToString(unzipped_path.Append(base::FilePath(kResource)),
+                             &modified_contents);
+      modified_contents.append(content_to_append_for_mismatch);
+      EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH,
+                RunContentVerifyJob(*extension.get(), existent_resource_path,
+                                    modified_contents));
+    }
+  }
+
   base::ScopedTempDir temp_dir_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(ContentVerifyJobUnittest);
 };
 
@@ -138,11 +118,13 @@ class ContentVerifyJobUnittest : public ExtensionsTest {
 // Also tests that non-existent file request does not trigger content
 // verification failure.
 TEST_F(ContentVerifyJobUnittest, DeletedAndMissingFiles) {
-  base::FilePath unzipped_path;
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  base::FilePath unzipped_path = temp_dir_.GetPath();
   base::FilePath test_dir_base =
       GetTestPath(base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));
-  scoped_refptr<Extension> extension = UnzipToTempDirAndLoad(
-      test_dir_base.AppendASCII("source_all.zip"), &unzipped_path);
+  scoped_refptr<Extension> extension =
+      content_verifier_test_utils::UnzipToDirAndLoadExtension(
+          test_dir_base.AppendASCII("source_all.zip"), unzipped_path);
   ASSERT_TRUE(extension.get());
   // Make sure there is a verified_contents.json file there as this test cannot
   // fetch it.
@@ -222,41 +204,20 @@ TEST_F(ContentVerifyJobUnittest, DeletedAndMissingFiles) {
 
 // Tests that content modification causes content verification failure.
 TEST_F(ContentVerifyJobUnittest, ContentMismatch) {
-  base::FilePath unzipped_path;
-  base::FilePath test_dir_base =
-      GetTestPath(base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));
-  scoped_refptr<Extension> extension = UnzipToTempDirAndLoad(
-      test_dir_base.AppendASCII("source_all.zip"), &unzipped_path);
-  ASSERT_TRUE(extension.get());
-  // Make sure there is a verified_contents.json file there as this test cannot
-  // fetch it.
-  EXPECT_TRUE(
-      base::PathExists(file_util::GetVerifiedContentsPath(extension->path())));
-
-  const base::FilePath::CharType kResource[] =
-      FILE_PATH_LITERAL("background.js");
-  base::FilePath existent_resource_path(kResource);
-  {
-    // Make sure modified background.js fails content verification.
-    std::string modified_contents;
-    base::ReadFileToString(unzipped_path.Append(base::FilePath(kResource)),
-                           &modified_contents);
-    modified_contents.append("console.log('modified');");
-    EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH,
-              RunContentVerifyJob(*extension.get(), existent_resource_path,
-                                  modified_contents));
-  }
+  RunContentMismatchTest("console.log('modified');");
 }
 
 // Tests that extension resources that are originally 0 byte behave correctly
 // with content verification.
 TEST_F(ContentVerifyJobUnittest, LegitimateZeroByteFile) {
-  base::FilePath unzipped_path;
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  base::FilePath unzipped_path = temp_dir_.GetPath();
   base::FilePath test_dir_base =
       GetTestPath(base::FilePath(FILE_PATH_LITERAL("zero_byte_file")));
   // |extension| has a 0 byte background.js file in it.
-  scoped_refptr<Extension> extension = UnzipToTempDirAndLoad(
-      test_dir_base.AppendASCII("source.zip"), &unzipped_path);
+  scoped_refptr<Extension> extension =
+      content_verifier_test_utils::UnzipToDirAndLoadExtension(
+          test_dir_base.AppendASCII("source.zip"), unzipped_path);
   ASSERT_TRUE(extension.get());
   // Make sure there is a verified_contents.json file there as this test cannot
   // fetch it.
@@ -288,11 +249,13 @@ TEST_F(ContentVerifyJobUnittest, LegitimateZeroByteFile) {
 // Regression test for https://crbug.com/720597, where content verification
 // always failed for sizes multiple of content hash's block size (4096 bytes).
 TEST_F(ContentVerifyJobUnittest, DifferentSizedFiles) {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  base::FilePath unzipped_path = temp_dir_.GetPath();
   base::FilePath test_dir_base =
       GetTestPath(base::FilePath(FILE_PATH_LITERAL("different_sized_files")));
-  base::FilePath unzipped_path;
-  scoped_refptr<Extension> extension = UnzipToTempDirAndLoad(
-      test_dir_base.AppendASCII("source.zip"), &unzipped_path);
+  scoped_refptr<Extension> extension =
+      content_verifier_test_utils::UnzipToDirAndLoadExtension(
+          test_dir_base.AppendASCII("source.zip"), unzipped_path);
   ASSERT_TRUE(extension.get());
   // Make sure there is a verified_contents.json file there as this test cannot
   // fetch it.

extensions/browser/scoped_ignore_content_verifier_for_test.cc

@@ -4,26 +4,16 @@
 
 #include "extensions/browser/scoped_ignore_content_verifier_for_test.h"
 
+#include "extensions/browser/content_verify_job.h"
+
 namespace extensions {
 
 ScopedIgnoreContentVerifierForTest::ScopedIgnoreContentVerifierForTest() {
-  ContentVerifyJob::SetDelegateForTests(this);
+  ContentVerifyJob::SetIgnoreVerificationForTests(true);
 }
 
 ScopedIgnoreContentVerifierForTest::~ScopedIgnoreContentVerifierForTest() {
-  ContentVerifyJob::SetDelegateForTests(nullptr);
-}
-
-ContentVerifyJob::FailureReason ScopedIgnoreContentVerifierForTest::BytesRead(
-    const std::string& extension_id,
-    int count,
-    const char* data) {
-  return ContentVerifyJob::NONE;
-}
-
-ContentVerifyJob::FailureReason ScopedIgnoreContentVerifierForTest::DoneReading(
-    const std::string& extension_id) {
-  return ContentVerifyJob::NONE;
+  ContentVerifyJob::SetIgnoreVerificationForTests(false);
 }
 
 }  // namespace extensions

extensions/browser/scoped_ignore_content_verifier_for_test.h

@@ -5,28 +5,17 @@
 #ifndef EXTENSIONS_BROWSER_SCOPED_IGNORE_CONTENT_VERIFIER_FOR_TEST_H_
 #define EXTENSIONS_BROWSER_SCOPED_IGNORE_CONTENT_VERIFIER_FOR_TEST_H_
 
-#include <string>
-
-#include "extensions/browser/content_verify_job.h"
+#include "base/macros.h"
 
 namespace extensions {
 
 // A class for use in tests to make content verification failures be ignored
 // during the lifetime of an instance of it. Note that only one instance should
-// be alive at any given time, and that it is not compatible with other
-// concurrent objects using the ContentVerifyJob::TestDelegate interface.
-class ScopedIgnoreContentVerifierForTest
-    : public ContentVerifyJob::TestDelegate {
+// be alive at any given time.
+class ScopedIgnoreContentVerifierForTest {
  public:
   ScopedIgnoreContentVerifierForTest();
-  ~ScopedIgnoreContentVerifierForTest() override;
-
-  // ContentVerifyJob::TestDelegate interface
-  ContentVerifyJob::FailureReason BytesRead(const std::string& extension_id,
-                                            int count,
-                                            const char* data) override;
-  ContentVerifyJob::FailureReason DoneReading(
-      const std::string& extension_id) override;
+  ~ScopedIgnoreContentVerifierForTest();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ScopedIgnoreContentVerifierForTest);