Protect ContentVerifyJob::hash_reader_ access using lock.

CVJ::OnHashesReady seems to be missing lock while accessing member
variable. Add lock to the method. OnHashesReady also calls
BytesRead which already had lock. Turn that method to private
BytesReadImpl, remove lock from it, and provide lock to the
publicly callable version (new BytesRead).
This CL also protects CVJ::time_spent_.

Bug: 814966
Test: Internal change only. See bug for running unit_tests.
Change-Id: I632dccf2ee921d1c84392b44c3ab8e0ca52dde3e
Reviewed-on: https://chromium-review.googlesource.com/933245Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538970}

extensions/browser/content_verify_job.cc

@@ -68,8 +68,30 @@ void ContentVerifyJob::Start() {
 }
 
 void ContentVerifyJob::BytesRead(int count, const char* data) {
-  ScopedElapsedTimer timer(&time_spent_);
   base::AutoLock auto_lock(lock_);
+  BytesReadImpl(count, data);
+}
+
+void ContentVerifyJob::DoneReading() {
+  base::AutoLock auto_lock(lock_);
+  ScopedElapsedTimer timer(&time_spent_);
+  if (failed_)
+    return;
+  if (g_ignore_verification_for_tests)
+    return;
+  done_reading_ = true;
+  if (hashes_ready_) {
+    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);
+    }
+  }
+}
+
+void ContentVerifyJob::BytesReadImpl(int count, const char* data) {
+  ScopedElapsedTimer timer(&time_spent_);
   if (failed_)
     return;
   if (g_ignore_verification_for_tests)
@@ -109,24 +131,6 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
   }
 }
 
-void ContentVerifyJob::DoneReading() {
-  ScopedElapsedTimer timer(&time_spent_);
-  base::AutoLock auto_lock(lock_);
-  if (failed_)
-    return;
-  if (g_ignore_verification_for_tests)
-    return;
-  done_reading_ = true;
-  if (hashes_ready_) {
-    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);
-    }
-  }
-}
-
 bool ContentVerifyJob::FinishBlock() {
   DCHECK(!failed_);
   if (current_hash_byte_count_ == 0) {
@@ -160,6 +164,7 @@ bool ContentVerifyJob::FinishBlock() {
 void ContentVerifyJob::OnHashesReady(bool success) {
   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);
@@ -191,7 +196,7 @@ void ContentVerifyJob::OnHashesReady(bool success) {
   if (!queue_.empty()) {
     std::string tmp;
     queue_.swap(tmp);
-    BytesRead(tmp.size(), base::string_as_array(&tmp));
+    BytesReadImpl(tmp.size(), base::string_as_array(&tmp));
     if (failed_)
       return;
   }

extensions/browser/content_verify_job.h

@@ -93,6 +93,9 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   virtual ~ContentVerifyJob();
   friend class base::RefCountedThreadSafe<ContentVerifyJob>;
 
+  // Same as BytesRead, but is run without acquiring lock.
+  void BytesReadImpl(int count, const char* data);
+
   // Called each time we're done adding bytes for the current block, and are
   // ready to finish the hash operation for those bytes and make sure it
   // matches what was expected for that block. Returns true if everything is