[extensions] Fix double calling of FileURLLoaderObserver::OnDoneReading()

In case of disk/access error FileURLLoader called ContentVerifyJob::DoneReading twice:
(the first time from FileURLLoaderObserver::OnOpenComplete and the second
time from FileURLLoaderObserver::OnDoneReading).

This CL:
1. Move calling OnBytesRead+OnDoneReading for bad files to
   file_url_loader_factory;
2. Remove FileURLLoaderObserver::OnOpenComplete method;
3. Add DCHECKs in ContentVerifyJob so that we make sure OnDoneReading
   isn't called more than once. This also makes existing tests
   (e.g. ExtensionProtocolsTest.VerificationSeenForFileAccessErrors)
   serve as regression tests.


TEST=unit_tests.ExtensionProtocolsTest.VerificationSeenForFileAccessErrors

Change-Id: Ib91ecb7eaa5dad760068201903758df3f651dc5b
Reviewed-on: https://chromium-review.googlesource.com/999484
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Commit-Queue: Mikhail Atuchin <atuchin@yandex-team.ru>
Cr-Commit-Position: refs/heads/master@{#552158}

content/browser/file_url_loader_factory.cc

@@ -454,12 +454,12 @@ class FileURLLoader : public network::mojom::URLLoader {
       observer->OnStart();
 
     base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
-    net::Error net_error = net::FileErrorToNetError(file.error_details());
-    if (observer)
-      observer->OnOpenComplete(net_error);
     if (!file.IsValid()) {
-      if (observer)
+      if (observer) {
+        observer->OnBytesRead(nullptr, 0u, file.error_details());
         observer->OnDoneReading();
+      }
+      net::Error net_error = net::FileErrorToNetError(file.error_details());
       client->OnComplete(network::URLLoaderCompletionStatus(net_error));
       MaybeDeleteSelf();
       return;
@@ -471,10 +471,13 @@ class FileURLLoader : public network::mojom::URLLoader {
       base::File::Error read_error = base::File::GetLastFileError();
       DCHECK_NE(base::File::FILE_OK, read_error);
       if (observer) {
+        // This can happen when the file is unreadable (which can happen during
+        // corruption). We need to be sure to inform
+        // the observer that we've finished reading so that it can proceed.
         observer->OnBytesRead(nullptr, 0u, read_error);
         observer->OnDoneReading();
       }
-      net_error = net::FileErrorToNetError(read_error);
+      net::Error net_error = net::FileErrorToNetError(read_error);
       client->OnComplete(network::URLLoaderCompletionStatus(net_error));
       return;
     } else if (observer) {

content/public/browser/file_url_loader.h

@@ -22,7 +22,6 @@ class CONTENT_EXPORT FileURLLoaderObserver
   ~FileURLLoaderObserver() override {}
 
   virtual void OnStart() {}
-  virtual void OnOpenComplete(int result) {}
   virtual void OnSeekComplete(int64_t result) {}
 
  private:

extensions/browser/content_verify_job.cc

@@ -90,6 +90,7 @@ void ContentVerifyJob::DidGetContentHashOnIO(
 
 void ContentVerifyJob::BytesRead(int count, const char* data) {
   base::AutoLock auto_lock(lock_);
+  DCHECK(!done_reading_);
   BytesReadImpl(count, data);
 }
 
@@ -100,6 +101,7 @@ void ContentVerifyJob::DoneReading() {
     return;
   if (g_ignore_verification_for_tests)
     return;
+  DCHECK(!done_reading_);
   done_reading_ = true;
   if (hashes_ready_) {
     if (!FinishBlock()) {

extensions/browser/extension_protocols.cc

@@ -695,20 +695,6 @@ class FileLoaderObserver : public content::FileURLLoaderObserver {
     request_timer_.reset(new base::ElapsedTimer());
   }
 
-  void OnOpenComplete(int result) override {
-    if (result < 0) {
-      // This can happen when the file is unreadable (which can happen during
-      // corruption or third-party interaction). We need to be sure to inform
-      // the verification job that we've finished reading so that it can
-      // proceed; see crbug.com/703888.
-      if (verify_job_.get()) {
-        std::string tmp;
-        verify_job_->BytesRead(0, base::string_as_array(&tmp));
-        verify_job_->DoneReading();
-      }
-    }
-  }
-
   void OnSeekComplete(int64_t result) override {
     DCHECK_EQ(seek_position_, 0);
     base::AutoLock auto_lock(lock_);