[Extensions] Make g_test_content_verify_job_observer thread-safe

ContentVerifyJob has ability to inject test observer, but before this CL
the observer is accessed from different threads, leading to flaky
crashes in tests. See https://crrev.com/c/2032998.

Now this observer (ContentVerifyJob::TestObserver and pointer to it from
g_test_content_verify_gob_observer in content_verify_job.cc) supports
thread-safe refcounted pointing, therefore is not destroyed too early.
Also it's wrappend into lazy initialized since we need access to it be
no-op outside of the tests.

Bug: 796395, 958794
Change-Id: I7479216f23083d4fd41dae08cb2e7acb62da0abd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033159Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Oleg Davydov <burunduk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739387}

chrome/browser/extensions/extension_protocols_unittest.cc

@@ -573,7 +573,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForFileAccessErrors) {
 
   // chmod -r 1024.js.
   {
-    TestContentVerifySingleJobObserver observer(extension->id(), kRelativePath);
+    TestContentVerifySingleJobObserver observer(extension_id, kRelativePath);
     base::FilePath file_path = unzipped_path.AppendASCII(kJs);
     ASSERT_TRUE(base::MakeFileUnreadable(file_path));
     EXPECT_EQ(net::ERR_ACCESS_DENIED, DoRequestOrLoad(extension, kJs).result());
@@ -637,7 +637,7 @@ TEST_F(ExtensionProtocolsTest, VerificationSeenForZeroByteFile) {
   // current behavior of ContentVerifyJob.
   // TODO(lazyboy): The behavior is probably incorrect.
   {
-    TestContentVerifySingleJobObserver observer(extension->id(), kRelativePath);
+    TestContentVerifySingleJobObserver observer(extension_id, kRelativePath);
     base::FilePath file_path = unzipped_path.AppendASCII(kEmptyJs);
     ASSERT_TRUE(base::MakeFileUnreadable(file_path));
     EXPECT_EQ(net::ERR_ACCESS_DENIED,

extensions/browser/content_verifier/test_utils.cc

@@ -22,18 +22,46 @@ namespace extensions {
 TestContentVerifySingleJobObserver::TestContentVerifySingleJobObserver(
     const ExtensionId& extension_id,
     const base::FilePath& relative_path)
-    : extension_id_(extension_id), relative_path_(relative_path) {
-  ContentVerifyJob::SetObserverForTests(this);
+    : client_(
+          base::MakeRefCounted<ObserverClient>(extension_id, relative_path)) {
+  ContentVerifyJob::SetObserverForTests(client_);
 }
 
 TestContentVerifySingleJobObserver::~TestContentVerifySingleJobObserver() {
   ContentVerifyJob::SetObserverForTests(nullptr);
 }
 
-void TestContentVerifySingleJobObserver::JobFinished(
+ContentVerifyJob::FailureReason
+TestContentVerifySingleJobObserver::WaitForJobFinished() {
+  return client_->WaitForJobFinished();
+}
+
+void TestContentVerifySingleJobObserver::WaitForOnHashesReady() {
+  client_->WaitForOnHashesReady();
+}
+
+TestContentVerifySingleJobObserver::ObserverClient::ObserverClient(
+    const ExtensionId& extension_id,
+    const base::FilePath& relative_path)
+    : extension_id_(extension_id), relative_path_(relative_path) {
+  EXPECT_TRUE(
+      content::BrowserThread::GetCurrentThreadIdentifier(&creation_thread_));
+}
+
+TestContentVerifySingleJobObserver::ObserverClient::~ObserverClient() = default;
+
+void TestContentVerifySingleJobObserver::ObserverClient::JobFinished(
     const ExtensionId& extension_id,
     const base::FilePath& relative_path,
     ContentVerifyJob::FailureReason reason) {
+  if (!content::BrowserThread::CurrentlyOn(creation_thread_)) {
+    base::PostTask(
+        FROM_HERE, {creation_thread_},
+        base::BindOnce(
+            &TestContentVerifySingleJobObserver::ObserverClient::JobFinished,
+            this, extension_id, relative_path, reason));
+    return;
+  }
   if (extension_id != extension_id_ || relative_path != relative_path_)
     return;
   EXPECT_FALSE(failure_reason_.has_value());
@@ -41,10 +69,18 @@ void TestContentVerifySingleJobObserver::JobFinished(
   job_finished_run_loop_.Quit();
 }
 
-void TestContentVerifySingleJobObserver::OnHashesReady(
+void TestContentVerifySingleJobObserver::ObserverClient::OnHashesReady(
     const ExtensionId& extension_id,
     const base::FilePath& relative_path,
     bool success) {
+  if (!content::BrowserThread::CurrentlyOn(creation_thread_)) {
+    base::PostTask(
+        FROM_HERE, {creation_thread_},
+        base::BindOnce(
+            &TestContentVerifySingleJobObserver::ObserverClient::OnHashesReady,
+            this, extension_id, relative_path, success));
+    return;
+  }
   if (extension_id != extension_id_ || relative_path != relative_path_)
     return;
   EXPECT_FALSE(seen_on_hashes_ready_);
@@ -53,60 +89,57 @@ void TestContentVerifySingleJobObserver::OnHashesReady(
 }
 
 ContentVerifyJob::FailureReason
-TestContentVerifySingleJobObserver::WaitForJobFinished() {
+TestContentVerifySingleJobObserver::ObserverClient::WaitForJobFinished() {
   // Run() returns immediately if Quit() has already been called.
   job_finished_run_loop_.Run();
   EXPECT_TRUE(failure_reason_.has_value());
   return failure_reason_.value_or(ContentVerifyJob::FAILURE_REASON_MAX);
 }
 
-void TestContentVerifySingleJobObserver::WaitForOnHashesReady() {
+void TestContentVerifySingleJobObserver::ObserverClient::
+    WaitForOnHashesReady() {
   // Run() returns immediately if Quit() has already been called.
   on_hashes_ready_run_loop_.Run();
 }
 
 // TestContentVerifyJobObserver ------------------------------------------------
+TestContentVerifyJobObserver::TestContentVerifyJobObserver()
+    : client_(base::MakeRefCounted<ObserverClient>()) {
+  ContentVerifyJob::SetObserverForTests(client_);
+}
+
+TestContentVerifyJobObserver::~TestContentVerifyJobObserver() {
+  ContentVerifyJob::SetObserverForTests(nullptr);
+}
+
 void TestContentVerifyJobObserver::ExpectJobResult(
     const ExtensionId& extension_id,
     const base::FilePath& relative_path,
     Result expected_result) {
-  expectations_.push_back(
-      ExpectedResult(extension_id, relative_path, expected_result));
+  client_->ExpectJobResult(extension_id, relative_path, expected_result);
 }
 
-TestContentVerifyJobObserver::TestContentVerifyJobObserver() {
-  EXPECT_TRUE(
-      content::BrowserThread::GetCurrentThreadIdentifier(&creation_thread_));
-  ContentVerifyJob::SetObserverForTests(this);
-}
-
-TestContentVerifyJobObserver::~TestContentVerifyJobObserver() {
-  ContentVerifyJob::SetObserverForTests(nullptr);
+bool TestContentVerifyJobObserver::WaitForExpectedJobs() {
+  return client_->WaitForExpectedJobs();
 }
 
-bool TestContentVerifyJobObserver::WaitForExpectedJobs() {
-  EXPECT_TRUE(content::BrowserThread::CurrentlyOn(creation_thread_));
-  if (!expectations_.empty()) {
-    base::RunLoop run_loop;
-    job_quit_closure_ = run_loop.QuitClosure();
-    run_loop.Run();
-  }
-  return expectations_.empty();
+TestContentVerifyJobObserver::ObserverClient::ObserverClient() {
+  EXPECT_TRUE(
+      content::BrowserThread::GetCurrentThreadIdentifier(&creation_thread_));
 }
 
-void TestContentVerifyJobObserver::JobStarted(
-    const ExtensionId& extension_id,
-    const base::FilePath& relative_path) {}
+TestContentVerifyJobObserver::ObserverClient::~ObserverClient() = default;
 
-void TestContentVerifyJobObserver::JobFinished(
+void TestContentVerifyJobObserver::ObserverClient::JobFinished(
     const ExtensionId& extension_id,
     const base::FilePath& relative_path,
     ContentVerifyJob::FailureReason failure_reason) {
   if (!content::BrowserThread::CurrentlyOn(creation_thread_)) {
-    base::PostTask(FROM_HERE, {creation_thread_},
-                   base::BindOnce(&TestContentVerifyJobObserver::JobFinished,
-                                  base::Unretained(this), extension_id,
-                                  relative_path, failure_reason));
+    base::PostTask(
+        FROM_HERE, {creation_thread_},
+        base::BindOnce(
+            &TestContentVerifyJobObserver::ObserverClient::JobFinished, this,
+            extension_id, relative_path, failure_reason));
     return;
   }
   Result result = failure_reason == ContentVerifyJob::NONE ? Result::SUCCESS
@@ -130,6 +163,24 @@ void TestContentVerifyJobObserver::JobFinished(
   }
 }
 
+void TestContentVerifyJobObserver::ObserverClient::ExpectJobResult(
+    const ExtensionId& extension_id,
+    const base::FilePath& relative_path,
+    Result expected_result) {
+  expectations_.push_back(
+      ExpectedResult(extension_id, relative_path, expected_result));
+}
+
+bool TestContentVerifyJobObserver::ObserverClient::WaitForExpectedJobs() {
+  EXPECT_TRUE(content::BrowserThread::CurrentlyOn(creation_thread_));
+  if (!expectations_.empty()) {
+    base::RunLoop run_loop;
+    job_quit_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+  return expectations_.empty();
+}
+
 // MockContentVerifierDelegate ------------------------------------------------
 MockContentVerifierDelegate::MockContentVerifierDelegate() = default;
 MockContentVerifierDelegate::~MockContentVerifierDelegate() = default;

extensions/browser/content_verifier/test_utils.h

@@ -24,22 +24,12 @@ 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 TestContentVerifySingleJobObserver : ContentVerifyJob::TestObserver {
+class TestContentVerifySingleJobObserver {
  public:
   TestContentVerifySingleJobObserver(const ExtensionId& extension_id,
                                      const base::FilePath& relative_path);
   ~TestContentVerifySingleJobObserver();
 
-  // ContentVerifyJob::TestObserver:
-  void JobStarted(const ExtensionId& extension_id,
-                  const base::FilePath& relative_path) override {}
-  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 WaitForJobFinished() WARN_UNUSED_RESULT;
 
@@ -47,22 +37,55 @@ class TestContentVerifySingleJobObserver : ContentVerifyJob::TestObserver {
   void WaitForOnHashesReady();
 
  private:
-  base::RunLoop job_finished_run_loop_;
-  base::RunLoop on_hashes_ready_run_loop_;
+  class ObserverClient : public ContentVerifyJob::TestObserver {
+   public:
+    ObserverClient(const ExtensionId& extension_id,
+                   const base::FilePath& relative_path);
+
+    // ContentVerifyJob::TestObserver:
+    void JobStarted(const ExtensionId& extension_id,
+                    const base::FilePath& relative_path) override {}
+    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;
+
+    // Passed methods from ContentVerifySingleJobObserver:
+    ContentVerifyJob::FailureReason WaitForJobFinished() WARN_UNUSED_RESULT;
+    void WaitForOnHashesReady();
+
+   private:
+    ~ObserverClient() override;
+
+    ObserverClient(const ObserverClient&) = delete;
+    ObserverClient& operator=(const ObserverClient&) = delete;
+
+    content::BrowserThread::ID creation_thread_;
 
-  ExtensionId extension_id_;
-  base::FilePath relative_path_;
-  base::Optional<ContentVerifyJob::FailureReason> failure_reason_;
-  bool seen_on_hashes_ready_ = false;
+    base::RunLoop job_finished_run_loop_;
+    base::RunLoop on_hashes_ready_run_loop_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestContentVerifySingleJobObserver);
+    ExtensionId extension_id_;
+    base::FilePath relative_path_;
+    base::Optional<ContentVerifyJob::FailureReason> failure_reason_;
+    bool seen_on_hashes_ready_ = false;
+  };
+
+  TestContentVerifySingleJobObserver(
+      const TestContentVerifySingleJobObserver&) = delete;
+  TestContentVerifySingleJobObserver& operator=(
+      const TestContentVerifySingleJobObserver&) = delete;
+
+  scoped_refptr<ObserverClient> client_;
 };
 
 // Test class to observe expected set of ContentVerifyJobs.
-class TestContentVerifyJobObserver : public ContentVerifyJob::TestObserver {
+class TestContentVerifyJobObserver {
  public:
   TestContentVerifyJobObserver();
-  virtual ~TestContentVerifyJobObserver();
+  ~TestContentVerifyJobObserver();
 
   enum class Result { SUCCESS, FAILURE };
 
@@ -75,34 +98,56 @@ class TestContentVerifyJobObserver : public ContentVerifyJob::TestObserver {
   // finish, or false if there was an error or timeout.
   bool WaitForExpectedJobs();
 
-  // ContentVerifyJob::TestObserver interface
-  void JobStarted(const ExtensionId& extension_id,
-                  const base::FilePath& relative_path) override;
-  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 {
+  class ObserverClient : public ContentVerifyJob::TestObserver {
    public:
-    ExtensionId extension_id;
-    base::FilePath path;
-    Result result;
-
-    ExpectedResult(const ExtensionId& extension_id,
-                   const base::FilePath& path,
-                   Result result)
-        : extension_id(extension_id), path(path), result(result) {}
+    ObserverClient();
+
+    // ContentVerifyJob::TestObserver:
+    void JobStarted(const ExtensionId& extension_id,
+                    const base::FilePath& relative_path) override {}
+    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 {}
+
+    // Passed methods from TestContentVerifyJobObserver:
+    void ExpectJobResult(const ExtensionId& extension_id,
+                         const base::FilePath& relative_path,
+                         Result expected_result);
+    bool WaitForExpectedJobs();
+
+   private:
+    struct ExpectedResult {
+     public:
+      ExtensionId extension_id;
+      base::FilePath path;
+      Result result;
+
+      ExpectedResult(const ExtensionId& extension_id,
+                     const base::FilePath& path,
+                     Result result)
+          : extension_id(extension_id), path(path), result(result) {}
+    };
+
+    ~ObserverClient() override;
+
+    ObserverClient(const ObserverClient&) = delete;
+    ObserverClient& operator=(const ObserverClient&) = delete;
+
+    std::list<ExpectedResult> expectations_;
+    content::BrowserThread::ID creation_thread_;
+    // Accessed on |creation_thread_|.
+    base::OnceClosure job_quit_closure_;
   };
-  std::list<ExpectedResult> expectations_;
-  content::BrowserThread::ID creation_thread_;
-  // Accessed on |creation_thread_|.
-  base::OnceClosure job_quit_closure_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestContentVerifyJobObserver);
+  TestContentVerifyJobObserver(const TestContentVerifyJobObserver&) = delete;
+  TestContentVerifyJobObserver& operator=(const TestContentVerifyJobObserver&) =
+      delete;
+
+  scoped_refptr<ObserverClient> client_;
 };
 
 // An extensions/ implementation of ContentVerifierDelegate for using in tests.

extensions/browser/content_verify_job.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "base/bind.h"
+#include "base/lazy_instance.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/task/post_task.h"
@@ -23,7 +24,15 @@ namespace extensions {
 namespace {
 
 bool g_ignore_verification_for_tests = false;
-ContentVerifyJob::TestObserver* g_content_verify_job_test_observer = NULL;
+
+base::LazyInstance<scoped_refptr<ContentVerifyJob::TestObserver>>::Leaky
+    g_content_verify_job_test_observer = LAZY_INSTANCE_INITIALIZER;
+
+scoped_refptr<ContentVerifyJob::TestObserver> GetTestObserver() {
+  if (!g_content_verify_job_test_observer.IsCreated())
+    return nullptr;
+  return g_content_verify_job_test_observer.Get();
+}
 
 class ScopedElapsedTimer {
  public:
@@ -86,9 +95,9 @@ void ContentVerifyJob::DidGetContentHashOnIO(
     scoped_refptr<const ContentHash> content_hash) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   base::AutoLock auto_lock(lock_);
-  if (g_content_verify_job_test_observer)
-    g_content_verify_job_test_observer->JobStarted(extension_id_,
-                                                   relative_path_);
+  scoped_refptr<TestObserver> test_observer = GetTestObserver();
+  if (test_observer)
+    test_observer->JobStarted(extension_id_, relative_path_);
   // Build |hash_reader_|.
   base::PostTaskAndReplyWithResult(
       FROM_HERE,
@@ -119,10 +128,9 @@ void ContentVerifyJob::Done() {
 
   const bool can_proceed = has_ignorable_read_error_ || FinishBlock();
   if (can_proceed) {
-    if (g_content_verify_job_test_observer) {
-      g_content_verify_job_test_observer->JobFinished(extension_id_,
-                                                      relative_path_, NONE);
-    }
+    scoped_refptr<TestObserver> test_observer = GetTestObserver();
+    if (test_observer)
+      test_observer->JobFinished(extension_id_, relative_path_, NONE);
   } else {
     DispatchFailureCallback(HASH_MISMATCH);
   }
@@ -214,10 +222,9 @@ void ContentVerifyJob::OnHashesReady(
 
   if (g_ignore_verification_for_tests)
     return;
-  if (g_content_verify_job_test_observer) {
-    g_content_verify_job_test_observer->OnHashesReady(extension_id_,
-                                                      relative_path_, success);
-  }
+  scoped_refptr<TestObserver> test_observer = GetTestObserver();
+  if (test_observer)
+    test_observer->OnHashesReady(extension_id_, relative_path_, success);
   if (!success) {
     if (!hash_reader_->has_content_hashes()) {
       DispatchFailureCallback(MISSING_ALL_HASHES);
@@ -226,10 +233,9 @@ void ContentVerifyJob::OnHashesReady(
 
     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(extension_id_,
-                                                        relative_path_, NONE);
-      }
+      scoped_refptr<TestObserver> test_observer = GetTestObserver();
+      if (test_observer)
+        test_observer->JobFinished(extension_id_, relative_path_, NONE);
       return;
     }
     DispatchFailureCallback(NO_HASHES_FOR_FILE);
@@ -250,9 +256,10 @@ void ContentVerifyJob::OnHashesReady(
     ScopedElapsedTimer timer(&time_spent_);
     if (!has_ignorable_read_error_ && !FinishBlock()) {
       DispatchFailureCallback(HASH_MISMATCH);
-    } else if (g_content_verify_job_test_observer) {
-      g_content_verify_job_test_observer->JobFinished(extension_id_,
-                                                      relative_path_, NONE);
+    } else {
+      scoped_refptr<TestObserver> test_observer = GetTestObserver();
+      if (test_observer)
+        test_observer->JobFinished(extension_id_, relative_path_, NONE);
     }
   }
 }
@@ -264,11 +271,13 @@ void ContentVerifyJob::SetIgnoreVerificationForTests(bool value) {
 }
 
 // static
-void ContentVerifyJob::SetObserverForTests(TestObserver* observer) {
-  DCHECK(observer == nullptr || g_content_verify_job_test_observer == nullptr)
+void ContentVerifyJob::SetObserverForTests(
+    scoped_refptr<TestObserver> observer) {
+  DCHECK(observer == nullptr ||
+         g_content_verify_job_test_observer.Get() == nullptr)
       << "SetObserverForTests does not support interleaving. Observers should "
       << "be set and then cleared one at a time.";
-  g_content_verify_job_test_observer = observer;
+  g_content_verify_job_test_observer.Get() = std::move(observer);
 }
 
 void ContentVerifyJob::DispatchFailureCallback(FailureReason reason) {
@@ -279,10 +288,9 @@ void ContentVerifyJob::DispatchFailureCallback(FailureReason reason) {
             << 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(extension_id_,
-                                                    relative_path_, reason);
-  }
+  scoped_refptr<TestObserver> test_observer = GetTestObserver();
+  if (test_observer)
+    test_observer->JobFinished(extension_id_, relative_path_, reason);
 }
 
 }  // namespace extensions

extensions/browser/content_verify_job.h

@@ -80,7 +80,7 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   // is not so appropriate.
   void Done();
 
-  class TestObserver {
+  class TestObserver : public base::RefCountedThreadSafe<TestObserver> {
    public:
     virtual void JobStarted(const ExtensionId& extension_id,
                             const base::FilePath& relative_path) = 0;
@@ -92,12 +92,16 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
     virtual void OnHashesReady(const ExtensionId& extension_id,
                                const base::FilePath& relative_path,
                                bool success) = 0;
+
+   protected:
+    virtual ~TestObserver() = default;
+    friend class base::RefCountedThreadSafe<TestObserver>;
   };
 
   static void SetIgnoreVerificationForTests(bool value);
 
   // Note: having interleaved observer is not supported.
-  static void SetObserverForTests(TestObserver* observer);
+  static void SetObserverForTests(scoped_refptr<TestObserver> observer);
 
  private:
   virtual ~ContentVerifyJob();