Add extension content verification in Network Service.

Bug: 782015
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I877446e14fb53d0c6c0ea73b5eda8399821668fc
Reviewed-on: https://chromium-review.googlesource.com/801235Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526770}

chrome/browser/chrome_content_browser_client.cc

@@ -458,7 +458,6 @@ using content::PosixFileDescriptorInfo;
 using extensions::APIPermission;
 using extensions::ChromeContentBrowserClientExtensionsPart;
 using extensions::Extension;
-using extensions::InfoMap;
 using extensions::Manifest;
 #endif
 

content/public/browser/file_url_loader.h

@@ -5,16 +5,34 @@
 #ifndef CONTENT_PUBLIC_BROWSER_FILE_URL_LOADER_H_
 #define CONTENT_PUBLIC_BROWSER_FILE_URL_LOADER_H_
 
+#include <memory>
+
 #include "content/common/content_export.h"
 #include "content/public/common/url_loader.mojom.h"
+#include "mojo/public/cpp/system/file_data_pipe_producer.h"
 
 namespace content {
 
 struct ResourceRequest;
 
+class CONTENT_EXPORT FileURLLoaderObserver
+    : public mojo::FileDataPipeProducer::Observer {
+ public:
+  FileURLLoaderObserver() {}
+  ~FileURLLoaderObserver() override {}
+
+  virtual void OnStart() {}
+  virtual void OnOpenComplete(int result) {}
+  virtual void OnSeekComplete(int64_t result) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FileURLLoaderObserver);
+};
+
 // Helper to create a self-owned URLLoader instance which fulfills |request|
 // using the contents of the file at |path|. The URL in |request| must be a
-// file:// URL.
+// file:// URL. The *optionally* supplied |observer| will be called to report
+// progress during the file loading.
 //
 // Note that this does not restrict filesystem access *in any way*, so if the
 // file at |path| is accessible to the browser, it will be loaded and used to
@@ -23,9 +41,11 @@ struct ResourceRequest;
 // The URLLoader created by this function does *not* automatically follow
 // filesytem links (e.g. Windows shortcuts) or support directory listing.
 // A directory path will always yield a FILE_NOT_FOUND network error.
-CONTENT_EXPORT void CreateFileURLLoader(const ResourceRequest& request,
-                                        mojom::URLLoaderRequest loader,
-                                        mojom::URLLoaderClientPtr client);
+CONTENT_EXPORT void CreateFileURLLoader(
+    const ResourceRequest& request,
+    mojom::URLLoaderRequest loader,
+    mojom::URLLoaderClientPtr client,
+    std::unique_ptr<FileURLLoaderObserver> observer);
 
 }  // namespace content
 

extensions/browser/content_verify_job.cc

@@ -49,11 +49,7 @@ ContentVerifyJob::ContentVerifyJob(ContentHashReader* hash_reader,
       current_hash_byte_count_(0),
       hash_reader_(hash_reader),
       failure_callback_(std::move(failure_callback)),
-      failed_(false) {
-  // It's ok for this object to be constructed on a different thread from where
-  // it's used.
-  thread_checker_.DetachFromThread();
-}
+      failed_(false) {}
 
 ContentVerifyJob::~ContentVerifyJob() {
   UMA_HISTOGRAM_COUNTS("ExtensionContentVerifyJob.TimeSpentUS",
@@ -61,7 +57,7 @@ ContentVerifyJob::~ContentVerifyJob() {
 }
 
 void ContentVerifyJob::Start() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock auto_lock(lock_);
   if (g_content_verify_job_test_observer)
     g_content_verify_job_test_observer->JobStarted(
         hash_reader_->extension_id(), hash_reader_->relative_path());
@@ -73,7 +69,7 @@ void ContentVerifyJob::Start() {
 
 void ContentVerifyJob::BytesRead(int count, const char* data) {
   ScopedElapsedTimer timer(&time_spent_);
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock auto_lock(lock_);
   if (failed_)
     return;
   if (g_test_delegate) {
@@ -120,7 +116,7 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
 
 void ContentVerifyJob::DoneReading() {
   ScopedElapsedTimer timer(&time_spent_);
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock auto_lock(lock_);
   if (failed_)
     return;
   if (g_test_delegate) {

extensions/browser/content_verify_job.h

@@ -13,7 +13,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/threading/thread_checker.h"
+#include "base/synchronization/lock.h"
 
 namespace base {
 class FilePath;
@@ -29,8 +29,7 @@ class ContentHashReader;
 
 // Objects of this class are responsible for verifying that the actual content
 // read from an extension file matches an expected set of hashes. This class
-// can be created on any thread but the rest of the methods should be called
-// from only one thread.
+// can be created and used on any thread.
 class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
  public:
   enum FailureReason {
@@ -146,8 +145,8 @@ class ContentVerifyJob : public base::RefCountedThreadSafe<ContentVerifyJob> {
   // Set to true if we detected a mismatch and called the failure callback.
   bool failed_;
 
-  // For ensuring methods on called on the right thread.
-  base::ThreadChecker thread_checker_;
+  // Used to synchronize all public methods.
+  base::Lock lock_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentVerifyJob);
 };

extensions/browser/extension_protocols.cc

@@ -10,6 +10,7 @@
 #include <algorithm>
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/base64.h"
@@ -37,6 +38,7 @@
 #include "base/timer/elapsed_timer.h"
 #include "build/build_config.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/file_url_loader.h"
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/render_frame_host.h"
@@ -50,6 +52,7 @@
 #include "extensions/browser/content_verify_job.h"
 #include "extensions/browser/extension_navigation_ui_data.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_util.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/guest_view/web_view/web_view_guest.h"
@@ -652,23 +655,130 @@ ExtensionProtocolHandler::MaybeCreateJob(
                                     verify_job);
 }
 
+class FileLoaderObserver : public content::FileURLLoaderObserver {
+ public:
+  explicit FileLoaderObserver(scoped_refptr<ContentVerifyJob> verify_job)
+      : verify_job_(std::move(verify_job)) {}
+  ~FileLoaderObserver() override {
+    base::AutoLock auto_lock(lock_);
+    UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.TotalKbRead", bytes_read_ / 1024);
+    UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.SeekPosition", seek_position_);
+    if (request_timer_.get())
+      UMA_HISTOGRAM_TIMES("ExtensionUrlRequest.Latency",
+                          request_timer_->Elapsed());
+  }
+
+  void OnStart() override {
+    base::AutoLock auto_lock(lock_);
+    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_);
+    seek_position_ = result;
+    // TODO(asargent) - we'll need to add proper support for range headers.
+    // crbug.com/369895.
+    if (result > 0 && verify_job_.get())
+      verify_job_ = nullptr;
+  }
+
+  void OnBytesRead(const void* data,
+                   size_t num_bytes_read,
+                   base::File::Error read_result) override {
+    if (read_result == base::File::FILE_OK) {
+      UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.OnReadCompleteResult",
+                           read_result);
+      base::AutoLock auto_lock(lock_);
+      bytes_read_ += num_bytes_read;
+      if (verify_job_.get())
+        verify_job_->BytesRead(num_bytes_read, static_cast<const char*>(data));
+    } else {
+      net::Error net_error = net::FileErrorToNetError(read_result);
+      UMA_HISTOGRAM_SPARSE_SLOWLY("ExtensionUrlRequest.OnReadCompleteError",
+                                  net_error);
+    }
+  }
+
+  void OnDoneReading() override {
+    base::AutoLock auto_lock(lock_);
+    if (verify_job_.get())
+      verify_job_->DoneReading();
+  }
+
+ private:
+  int64_t bytes_read_ = 0;
+  int64_t seek_position_ = 0;
+  std::unique_ptr<base::ElapsedTimer> request_timer_;
+  scoped_refptr<ContentVerifyJob> verify_job_;
+  // To synchronize access to all members.
+  base::Lock lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileLoaderObserver);
+};
+
 void LoadExtensionResourceFromFileOnBackgroundSequence(
     const content::ResourceRequest& request,
     const std::string& extension_id,
     const base::FilePath& directory_path,
     const base::FilePath& relative_path,
     content::mojom::URLLoaderRequest loader,
-    content::mojom::URLLoaderClientPtrInfo client_info) {
+    content::mojom::URLLoaderClientPtrInfo client_info,
+    scoped_refptr<ContentVerifyJob> verify_job) {
   // NOTE: ExtensionResource::GetFilePath() must be called on a sequence which
   // tolerates blocking operations.
   ExtensionResource resource(extension_id, directory_path, relative_path);
   content::mojom::URLLoaderClientPtr client;
   client.Bind(std::move(client_info));
 
+  auto loader_observer =
+      std::make_unique<FileLoaderObserver>(std::move(verify_job));
+
   content::ResourceRequest file_request = request;
   file_request.url = net::FilePathToFileURL(resource.GetFilePath());
   content::CreateFileURLLoader(file_request, std::move(loader),
-                               std::move(client));
+                               std::move(client), std::move(loader_observer));
+}
+
+void CreateVerifierAndLoadFile(
+    const content::ResourceRequest& request,
+    const std::string& extension_id,
+    const base::FilePath& directory_path,
+    const base::FilePath& relative_path,
+    content::mojom::URLLoaderRequest loader,
+    content::mojom::URLLoaderClientPtr client,
+    scoped_refptr<extensions::InfoMap> extension_info_map) {
+  scoped_refptr<ContentVerifyJob> verify_job;
+  ContentVerifier* verifier = extension_info_map->content_verifier();
+  if (verifier) {
+    verify_job =
+        verifier->CreateJobFor(extension_id, directory_path, relative_path);
+    if (verify_job)
+      verify_job->Start();
+  }
+
+  auto task_runner = base::CreateSequencedTaskRunnerWithTraits(
+      {base::MayBlock(), base::TaskPriority::BACKGROUND});
+  task_runner->PostTask(
+      FROM_HERE,
+      base::BindOnce(&LoadExtensionResourceFromFileOnBackgroundSequence,
+                     request, extension_id, directory_path, relative_path,
+                     std::move(loader), client.PassInterface(),
+                     std::move(verify_job)));
 }
 
 class ExtensionURLLoaderFactory : public content::mojom::URLLoaderFactory {
@@ -679,7 +789,9 @@ class ExtensionURLLoaderFactory : public content::mojom::URLLoaderFactory {
   // |frame_host|.
   explicit ExtensionURLLoaderFactory(content::RenderFrameHost* frame_host,
                                      const GURL& frame_url)
-      : frame_host_(frame_host), frame_url_(frame_url) {}
+      : frame_host_(frame_host),
+        frame_url_(frame_url),
+        extension_info_map_(nullptr) {}
   ~ExtensionURLLoaderFactory() override = default;
 
   // content::mojom::URLLoaderFactory:
@@ -758,7 +870,7 @@ class ExtensionURLLoaderFactory : public content::mojom::URLLoaderFactory {
       return;
     }
 
-    // TODO(crbug.com/782015): Support component extension resource loading from
+    // TODO(crbug.com/782025): Support component extension resource loading from
     // the embedder's resource files. This would be the right place to try to
     // resolve such resources, before we attempt to hit other files on disk.
 
@@ -796,18 +908,16 @@ class ExtensionURLLoaderFactory : public content::mojom::URLLoaderFactory {
       }
     }
 
-    // TODO(crbug.com/782015): Support content verification on extension
-    // resource requests. This is roughly the point at which we'd want to create
-    // a ContentVerifyJob and somehow hook it into the file URLLoader we set up
-    // below.
+    if (!extension_info_map_) {
+      extension_info_map_ =
+          extensions::ExtensionSystem::Get(browser_context)->info_map();
+    }
 
-    auto task_runner = base::CreateSequencedTaskRunnerWithTraits(
-        {base::MayBlock(), base::TaskPriority::BACKGROUND});
-    task_runner->PostTask(
-        FROM_HERE,
-        base::BindOnce(&LoadExtensionResourceFromFileOnBackgroundSequence,
-                       request, extension_id, directory_path, relative_path,
-                       std::move(loader), client.PassInterface()));
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&CreateVerifierAndLoadFile, request, extension_id,
+                       directory_path, relative_path, std::move(loader),
+                       std::move(client), extension_info_map_));
   }
 
   void Clone(content::mojom::URLLoaderFactoryRequest request) override {
@@ -817,6 +927,7 @@ class ExtensionURLLoaderFactory : public content::mojom::URLLoaderFactory {
  private:
   content::RenderFrameHost* const frame_host_;
   const GURL frame_url_;
+  scoped_refptr<extensions::InfoMap> extension_info_map_;
 
   mojo::BindingSet<content::mojom::URLLoaderFactory> bindings_;
 

mojo/public/cpp/system/file_data_pipe_producer.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <limits>
 #include <memory>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ -60,12 +61,14 @@ class FileDataPipeProducer::FileSequenceState
       ScopedDataPipeProducerHandle producer_handle,
       scoped_refptr<base::SequencedTaskRunner> file_task_runner,
       CompletionCallback callback,
-      scoped_refptr<base::SequencedTaskRunner> callback_task_runner)
+      scoped_refptr<base::SequencedTaskRunner> callback_task_runner,
+      std::unique_ptr<Observer> observer)
       : base::RefCountedDeleteOnSequence<FileSequenceState>(file_task_runner),
         file_task_runner_(std::move(file_task_runner)),
         callback_task_runner_(std::move(callback_task_runner)),
         producer_handle_(std::move(producer_handle)),
-        callback_(std::move(callback)) {}
+        callback_(std::move(callback)),
+        observer_(std::move(observer)) {}
 
   void Cancel() {
     base::AutoLock lock(lock_);
@@ -165,8 +168,14 @@ class FileDataPipeProducer::FileSequenceState
       if (read_size < 0) {
         read_error = base::File::GetLastFileError();
         DCHECK_NE(base::File::FILE_OK, read_error);
+        if (observer_)
+          observer_->OnBytesRead(pipe_buffer, 0u, read_error);
       } else {
         read_error = base::File::FILE_OK;
+        if (observer_) {
+          observer_->OnBytesRead(pipe_buffer, static_cast<size_t>(read_size),
+                                 base::File::FILE_OK);
+        }
       }
       producer_handle_->EndWriteData(
           read_size >= 0 ? static_cast<uint32_t>(read_size) : 0);
@@ -195,6 +204,10 @@ class FileDataPipeProducer::FileSequenceState
   }
 
   void Finish(MojoResult result) {
+    if (observer_) {
+      observer_->OnDoneReading();
+      observer_ = nullptr;
+    }
     watcher_.reset();
     callback_task_runner_->PostTask(
         FROM_HERE, base::BindOnce(std::move(callback_),
@@ -215,13 +228,17 @@ class FileDataPipeProducer::FileSequenceState
   // Guards |is_cancelled_|.
   base::Lock lock_;
   bool is_cancelled_ = false;
+  std::unique_ptr<Observer> observer_;
 
   DISALLOW_COPY_AND_ASSIGN(FileSequenceState);
 };
 
 FileDataPipeProducer::FileDataPipeProducer(
-    ScopedDataPipeProducerHandle producer)
-    : producer_(std::move(producer)), weak_factory_(this) {}
+    ScopedDataPipeProducerHandle producer,
+    std::unique_ptr<Observer> observer)
+    : producer_(std::move(producer)),
+      observer_(std::move(observer)),
+      weak_factory_(this) {}
 
 FileDataPipeProducer::~FileDataPipeProducer() {
   if (file_sequence_state_)
@@ -255,7 +272,7 @@ void FileDataPipeProducer::InitializeNewRequest(CompletionCallback callback) {
       std::move(producer_), file_task_runner,
       base::BindOnce(&FileDataPipeProducer::OnWriteComplete,
                      weak_factory_.GetWeakPtr(), std::move(callback)),
-      base::SequencedTaskRunnerHandle::Get());
+      base::SequencedTaskRunnerHandle::Get(), std::move(observer_));
 }
 
 void FileDataPipeProducer::OnWriteComplete(

mojo/public/cpp/system/file_data_pipe_producer.h

@@ -5,6 +5,8 @@
 #ifndef MOJO_PUBLIC_CPP_SYSTEM_FILE_DATA_PIPE_PRODUCER_H_
 #define MOJO_PUBLIC_CPP_SYSTEM_FILE_DATA_PIPE_PRODUCER_H_
 
+#include <memory>
+
 #include "base/callback_forward.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
@@ -29,8 +31,32 @@ class MOJO_CPP_SYSTEM_EXPORT FileDataPipeProducer {
  public:
   using CompletionCallback = base::OnceCallback<void(MojoResult result)>;
 
+  // Interface definition of an optional object that may be supplied to the
+  // FileDataPipeProducer so that the data being read from the consumer can be
+  // observed.
+  class Observer {
+   public:
+    virtual ~Observer() {}
+
+    // Called once per read attempt. |data| contains the read data (if any).
+    // |num_bytes_read| is the number of read bytes, 0 indicates EOF. Both
+    // parameters may only be used when |read_result| is base::File::FILE_OK.
+    // Can be called on any sequence.
+    virtual void OnBytesRead(const void* data,
+                             size_t num_bytes_read,
+                             base::File::Error read_result) = 0;
+
+    // Called when the FileDataPipeProducer has finished reading all data. Will
+    // be called even if there was an error opening the file or reading the
+    // data. Can be called on any sequence.
+    virtual void OnDoneReading() = 0;
+  };
+
   // Constructs a new FileDataPipeProducer which will write data to |producer|.
-  explicit FileDataPipeProducer(ScopedDataPipeProducerHandle producer);
+  // Caller may supply an optional |observer| if observation of the read file
+  // data is desired.
+  FileDataPipeProducer(ScopedDataPipeProducerHandle producer,
+                       std::unique_ptr<Observer> observer);
   ~FileDataPipeProducer();
 
   // Attempts to eventually write all of |file|'s contents to the pipe. Invokes
@@ -77,6 +103,7 @@ class MOJO_CPP_SYSTEM_EXPORT FileDataPipeProducer {
 
   ScopedDataPipeProducerHandle producer_;
   scoped_refptr<FileSequenceState> file_sequence_state_;
+  std::unique_ptr<Observer> observer_;
   base::WeakPtrFactory<FileDataPipeProducer> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FileDataPipeProducer);

mojo/public/cpp/system/tests/file_data_pipe_producer_unittest.cc

@@ -138,6 +138,40 @@ class FileDataPipeProducerTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(FileDataPipeProducerTest);
 };
 
+struct DataPipeObserverData {
+  int num_read_errors = 0;
+  size_t bytes_read = 0;
+  int done_called = 0;
+};
+
+class TestObserver : public FileDataPipeProducer::Observer {
+ public:
+  explicit TestObserver(DataPipeObserverData* observer_data)
+      : observer_data_(observer_data) {}
+
+  void OnBytesRead(const void* data,
+                   size_t num_bytes_read,
+                   base::File::Error read_result) override {
+    base::AutoLock auto_lock(lock_);
+    if (read_result == base::File::FILE_OK)
+      observer_data_->bytes_read += num_bytes_read;
+    else
+      observer_data_->num_read_errors++;
+  }
+
+  void OnDoneReading() override {
+    base::AutoLock auto_lock(lock_);
+    observer_data_->done_called++;
+  }
+
+ private:
+  DataPipeObserverData* observer_data_;
+  // Observer may be called on any sequence.
+  base::Lock lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestObserver);
+};
+
 TEST_F(FileDataPipeProducerTest, WriteFromFile) {
   const std::string kTestStringFragment = "Hello, world!";
   constexpr size_t kNumRepetitions = 1000;
@@ -153,12 +187,18 @@ TEST_F(FileDataPipeProducerTest, WriteFromFile) {
                         loop.QuitClosure());
 
   base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  DataPipeObserverData observer_data;
+  auto observer = std::make_unique<TestObserver>(&observer_data);
   WriteFromFileThenCloseWriter(
-      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle)),
+      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle),
+                                             std::move(observer)),
       std::move(file));
   loop.Run();
 
   EXPECT_EQ(test_string, reader.data());
+  EXPECT_EQ(0, observer_data.num_read_errors);
+  EXPECT_EQ(test_string.size(), observer_data.bytes_read);
+  EXPECT_EQ(1, observer_data.done_called);
 }
 
 TEST_F(FileDataPipeProducerTest, WriteFromFilePartial) {
@@ -172,12 +212,18 @@ TEST_F(FileDataPipeProducerTest, WriteFromFilePartial) {
                         loop.QuitClosure());
 
   base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  DataPipeObserverData observer_data;
+  auto observer = std::make_unique<TestObserver>(&observer_data);
   WriteFromFileThenCloseWriter(
-      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle)),
+      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle),
+                                             std::move(observer)),
       std::move(file), kBytesToWrite);
   loop.Run();
 
   EXPECT_EQ(kTestString.substr(0, kBytesToWrite), reader.data());
+  EXPECT_EQ(0, observer_data.num_read_errors);
+  EXPECT_EQ(kBytesToWrite, observer_data.bytes_read);
+  EXPECT_EQ(1, observer_data.done_called);
 }
 
 TEST_F(FileDataPipeProducerTest, WriteFromInvalidFile) {
@@ -186,16 +232,22 @@ TEST_F(FileDataPipeProducerTest, WriteFromInvalidFile) {
 
   base::RunLoop loop;
   DataPipe pipe(kBytesToWrite);
+  DataPipeObserverData observer_data;
+  auto observer = std::make_unique<TestObserver>(&observer_data);
   DataPipeReader reader(std::move(pipe.consumer_handle), kBytesToWrite,
                         loop.QuitClosure());
 
   base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
   WriteFromFileThenCloseWriter(
-      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle)),
+      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle),
+                                             std::move(observer)),
       std::move(file), kBytesToWrite);
   loop.Run();
 
   EXPECT_EQ(0UL, reader.data().size());
+  EXPECT_EQ(0, observer_data.num_read_errors);
+  EXPECT_EQ(0UL, observer_data.bytes_read);
+  EXPECT_EQ(1, observer_data.done_called);
 }
 
 TEST_F(FileDataPipeProducerTest, WriteFromPath) {
@@ -212,12 +264,18 @@ TEST_F(FileDataPipeProducerTest, WriteFromPath) {
   DataPipeReader reader(std::move(pipe.consumer_handle), 16,
                         loop.QuitClosure());
 
+  DataPipeObserverData observer_data;
+  auto observer = std::make_unique<TestObserver>(&observer_data);
   WriteFromPathThenCloseWriter(
-      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle)),
+      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle),
+                                             std::move(observer)),
       path);
   loop.Run();
 
   EXPECT_EQ(test_string, reader.data());
+  EXPECT_EQ(0, observer_data.num_read_errors);
+  EXPECT_EQ(test_string.size(), observer_data.bytes_read);
+  EXPECT_EQ(1, observer_data.done_called);
 }
 
 TEST_F(FileDataPipeProducerTest, TinyFile) {
@@ -227,12 +285,18 @@ TEST_F(FileDataPipeProducerTest, TinyFile) {
   DataPipe pipe(16);
   DataPipeReader reader(std::move(pipe.consumer_handle), 16,
                         loop.QuitClosure());
+  DataPipeObserverData observer_data;
+  auto observer = std::make_unique<TestObserver>(&observer_data);
   WriteFromPathThenCloseWriter(
-      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle)),
+      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle),
+                                             std::move(observer)),
       path);
   loop.Run();
 
   EXPECT_EQ(kTestString, reader.data());
+  EXPECT_EQ(0, observer_data.num_read_errors);
+  EXPECT_EQ(kTestString.size(), observer_data.bytes_read);
+  EXPECT_EQ(1, observer_data.done_called);
 }
 
 TEST_F(FileDataPipeProducerTest, HugeFile) {
@@ -254,12 +318,18 @@ TEST_F(FileDataPipeProducerTest, HugeFile) {
   DataPipeReader reader(std::move(pipe.consumer_handle), kDataPipeSize,
                         loop.QuitClosure());
 
+  DataPipeObserverData observer_data;
+  auto observer = std::make_unique<TestObserver>(&observer_data);
   WriteFromPathThenCloseWriter(
-      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle)),
+      std::make_unique<FileDataPipeProducer>(std::move(pipe.producer_handle),
+                                             std::move(observer)),
       path);
   loop.Run();
 
   EXPECT_EQ(test_string, reader.data());
+  EXPECT_EQ(0, observer_data.num_read_errors);
+  EXPECT_EQ(kHugeFileSize, observer_data.bytes_read);
+  EXPECT_EQ(1, observer_data.done_called);
 }
 
 }  // namespace

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -22,9 +22,6 @@
 -ChromeSecurityExploitBrowserTest.CreateFilesystemURLInExtensionOrigin
 -ChromeSitePerProcessTest.LaunchExternalProtocolFromSubframe
 -ContentFaviconDriverTest.ReloadBypassingCache
--ContentVerifierTest.DotSlashPaths
--ContentVerifierTest.FailOnDone
--ContentVerifierTest.FailOnRead
 -CredentialManagerBrowserTest.CreatePublicKeyCredentialAlgorithmNotSupported
 -CredentialManagerBrowserTest.CreatePublicKeyCredentialNotImplemented
 -CredentialManagerBrowserTest.MojoConnectionRecreatedAfterNavigation