Remove the file thread dependency from FileNetLogObserver.

Instead use an internally created sequenced task runer.

BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2966283002
Cr-Commit-Position: refs/heads/master@{#485105}

components/cronet/android/cronet_url_request_context_adapter.cc

@@ -1027,7 +1027,7 @@ void CronetURLRequestContextAdapter::StartNetLogOnNetworkThread(
   if (net_log_file_observer_)
     return;
   net_log_file_observer_ = net::FileNetLogObserver::CreateUnbounded(
-      GetFileThread()->task_runner(), file_path, /*constants=*/nullptr);
+      file_path, /*constants=*/nullptr);
   CreateNetLogEntriesForActiveObjects({context_.get()},
                                       net_log_file_observer_.get());
   net::NetLogCaptureMode capture_mode =
@@ -1052,8 +1052,7 @@ void CronetURLRequestContextAdapter::StartNetLogToBoundedFileOnNetworkThread(
   DCHECK(base::PathIsWritable(file_path));
 
   net_log_file_observer_ = net::FileNetLogObserver::CreateBounded(
-      GetFileThread()->task_runner(), file_path, size, kNumNetLogEventFiles,
-      /*constants=*/nullptr);
+      file_path, size, kNumNetLogEventFiles, /*constants=*/nullptr);
 
   CreateNetLogEntriesForActiveObjects({context_.get()},
                                       net_log_file_observer_.get());

components/net_log/net_export_file_writer.cc

@@ -184,9 +184,9 @@ void NetExportFileWriter::StartNetLog(
 
   std::unique_ptr<base::Value> constants(
       ChromeNetLog::GetConstants(command_line_string, channel_string));
-  // Instantiate a FileNetLogObserver in unbounded mode.
-  file_net_log_observer_ = net::FileNetLogObserver::CreateUnbounded(
-      file_task_runner_, log_path_, std::move(constants));
+
+  file_net_log_observer_ =
+      net::FileNetLogObserver::CreateUnbounded(log_path_, std::move(constants));
 
   net_task_runner_->PostTaskAndReply(
       FROM_HERE,

components/net_log/net_export_file_writer_unittest.cc

@@ -16,8 +16,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_string_value_serializer.h"
-#include "base/message_loop/message_loop.h"
-#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
@@ -437,8 +436,7 @@ class NetExportFileWriterTest : public ::testing::Test {
   TestStateObserver test_state_observer_;
 
  private:
-  // Allows tasks to be posted to the main thread.
-  base::MessageLoop message_loop_;
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
 };
 
 TEST_F(NetExportFileWriterTest, InitFail) {

net/log/file_net_log_observer.h

@@ -15,7 +15,7 @@
 namespace base {
 class Value;
 class FilePath;
-class SingleThreadTaskRunner;
+class SequencedTaskRunner;
 }  // namespace base
 
 namespace net {
@@ -49,10 +49,7 @@ class NET_EXPORT FileNetLogObserver : public NetLog::ThreadSafeObserver {
  public:
   // Creates a FileNetLogObserver in bounded mode.
   //
-  // |file_task_runner| indicates the task runner that should be used to post
-  // tasks from the main thread to the file thread.
-  //
-  // |directory| is the directory where the log files will be.
+  // |directory| is the directory where the log files will be written to.
   //
   // |max_total_size| is the approximate limit on the cumulative size of all
   // netlog files.
@@ -64,7 +61,6 @@ class NET_EXPORT FileNetLogObserver : public NetLog::ThreadSafeObserver {
   // the log. It should generally be a modified version of GetNetConstants().
   // If not present, the output of GetNetConstants() will be used.
   static std::unique_ptr<FileNetLogObserver> CreateBounded(
-      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
       const base::FilePath& directory,
       size_t max_total_size,
       size_t total_num_files,
@@ -72,16 +68,12 @@ class NET_EXPORT FileNetLogObserver : public NetLog::ThreadSafeObserver {
 
   // Creates a FileNetLogObserver in unbounded mode.
   //
-  // |file_task_runner| indicates the task runner that should be used to post
-  // tasks from the main thread to the file thread.
-  //
-  // |log_path| is where the log file will be.
+  // |log_path| is where the log file will be written to.
   //
   // |constants| is an optional legend for decoding constant values used in
   // the log. It should generally be a modified version of GetNetConstants().
   // If not present, the output of GetNetConstants() will be used.
   static std::unique_ptr<FileNetLogObserver> CreateUnbounded(
-      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
       const base::FilePath& log_path,
       std::unique_ptr<base::Value> constants);
 
@@ -113,29 +105,26 @@ class NET_EXPORT FileNetLogObserver : public NetLog::ThreadSafeObserver {
   class BoundedFileWriter;
   class UnboundedFileWriter;
 
-  FileNetLogObserver(
-      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
-      std::unique_ptr<FileWriter> file_writer,
-      scoped_refptr<WriteQueue> write_queue,
-      std::unique_ptr<base::Value> constants);
+  FileNetLogObserver(scoped_refptr<base::SequencedTaskRunner> file_task_runner,
+                     std::unique_ptr<FileWriter> file_writer,
+                     scoped_refptr<WriteQueue> write_queue,
+                     std::unique_ptr<base::Value> constants);
 
-  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;
+  scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
 
-  // The |write_queue_| object is shared between the file thread and the main
-  // thread, and should be alive for the entirety of the observer's lifetime.
-  // It should be destroyed once both the observer has been destroyed and all
-  // tasks posted to the file thread have completed.
+  // The |write_queue_| object is shared between the file task runner and the
+  // main thread, and should be alive for the entirety of the observer's
+  // lifetime. It should be destroyed once both the observer has been destroyed
+  // and all tasks posted to the file task runner have completed.
   scoped_refptr<WriteQueue> write_queue_;
 
-  // This is the owning reference to a file thread object. The observer is
-  // responsible for destroying the file thread object by posting a task from
-  // the main thread to the file thread to destroy the FileWriter when the
-  // observer is destroyed.
+  // The FileNetLogObserver is shared between the main thread and
+  // |file_task_runner_|.
   //
-  // The use of base::Unretained with |file_writer_| to post tasks to the file
-  // thread is safe because the FileWriter object will be alive until the
-  // observer's destruction.
-  FileWriter* file_writer_;
+  // Conceptually FileNetLogObserver owns it, however on destruction its
+  // deletion is deferred until outstanding tasks on |file_task_runner_| have
+  // finished (since it is posted using base::Unretained()).
+  std::unique_ptr<FileWriter> file_writer_;
 
   DISALLOW_COPY_AND_ASSIGN(FileNetLogObserver);
 };

net/log/file_net_log_observer_unittest.cc

@@ -21,6 +21,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/task_scheduler/task_scheduler.h"
 #include "base/threading/thread.h"
 #include "base/values.h"
 #include "net/base/test_completion_callback.h"
@@ -159,21 +160,16 @@ class FileNetLogObserverTest : public ::testing::TestWithParam<bool> {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     bounded_log_dir_ = temp_dir_.GetPath();
     unbounded_log_path_ = bounded_log_dir_.AppendASCII("net-log.json");
-    file_thread_.reset(new base::Thread("NetLog File Thread"));
-    file_thread_->StartWithOptions(
-        base::Thread::Options(base::MessageLoop::TYPE_DEFAULT, 0));
-    ASSERT_TRUE(file_thread_->WaitUntilThreadStarted());
   }
 
   void CreateAndStartObserving(std::unique_ptr<base::Value> constants) {
     bool bounded = GetParam();
     if (bounded) {
       logger_ = FileNetLogObserver::CreateBounded(
-          file_thread_->task_runner(), bounded_log_dir_, kLargeFileSize,
-          kTotalNumFiles, std::move(constants));
+          bounded_log_dir_, kLargeFileSize, kTotalNumFiles,
+          std::move(constants));
     } else {
-      logger_ = FileNetLogObserver::CreateUnbounded(file_thread_->task_runner(),
-                                                    unbounded_log_path_,
+      logger_ = FileNetLogObserver::CreateUnbounded(unbounded_log_path_,
                                                     std::move(constants));
     }
 
@@ -194,6 +190,11 @@ class FileNetLogObserverTest : public ::testing::TestWithParam<bool> {
   }
 
   bool LogFilesExist() {
+    // The log files are written by a sequenced task runner. Drain all the
+    // scheduled tasks to ensure that the file writing ones have run before
+    // checking if they exist.
+    base::TaskScheduler::GetInstance()->FlushForTesting();
+
     bool bounded = GetParam();
     if (bounded) {
       if (base::PathExists(bounded_log_dir_.AppendASCII("constants.json")) ||
@@ -212,7 +213,6 @@ class FileNetLogObserverTest : public ::testing::TestWithParam<bool> {
 
  protected:
   NetLog net_log_;
-  std::unique_ptr<base::Thread> file_thread_;
   std::unique_ptr<FileNetLogObserver> logger_;
 
  private:
@@ -227,18 +227,13 @@ class FileNetLogObserverBoundedTest : public ::testing::Test {
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     bounded_log_dir_ = temp_dir_.GetPath();
-    file_thread_.reset(new base::Thread("NetLog File Thread"));
-    file_thread_->StartWithOptions(
-        base::Thread::Options(base::MessageLoop::TYPE_DEFAULT, 0));
-    ASSERT_TRUE(file_thread_->WaitUntilThreadStarted());
   }
 
   void CreateAndStartObserving(std::unique_ptr<base::Value> constants,
                                int total_file_size,
                                int num_files) {
     logger_ = FileNetLogObserver::CreateBounded(
-        file_thread_->task_runner(), bounded_log_dir_, total_file_size,
-        num_files, std::move(constants));
+        bounded_log_dir_, total_file_size, num_files, std::move(constants));
     logger_->StartObserving(&net_log_, NetLogCaptureMode::Default());
   }
 
@@ -263,7 +258,6 @@ class FileNetLogObserverBoundedTest : public ::testing::Test {
 
  protected:
   NetLog net_log_;
-  std::unique_ptr<base::Thread> file_thread_;
   std::unique_ptr<FileNetLogObserver> logger_;
 
  private:
@@ -276,15 +270,20 @@ INSTANTIATE_TEST_CASE_P(,
                         FileNetLogObserverTest,
                         ::testing::Values(true, false));
 
+// Tests deleting a FileNetLogObserver without first calling StopObserving().
 TEST_P(FileNetLogObserverTest, ObserverDestroyedWithoutStopObserving) {
   CreateAndStartObserving(nullptr);
 
   // Send dummy event
   AddEntries(logger_.get(), 1, kDummyEventSize);
 
+  // The log files should have been started.
+  ASSERT_TRUE(LogFilesExist());
+
   logger_.reset();
-  file_thread_.reset();
 
+  // When the logger is re-set without having called StopObserving(), the
+  // partially written log files are deleted.
   ASSERT_FALSE(LogFilesExist());
 }
 
@@ -801,7 +800,7 @@ void AddEntriesViaNetLog(NetLog* net_log, int num_entries) {
 TEST_P(FileNetLogObserverTest, AddEventsFromMultipleThreadsWithStopObserving) {
   const size_t kNumThreads = 10;
   std::vector<std::unique_ptr<base::Thread>> threads(kNumThreads);
-  // Start all the threads. Waiting for them to start is to hopefuly improve
+  // Start all the threads. Waiting for them to start is to hopefully improve
   // the odds of hitting interesting races once events start being added.
   for (size_t i = 0; i < threads.size(); ++i) {
     threads[i] = base::MakeUnique<base::Thread>(
@@ -828,13 +827,15 @@ TEST_P(FileNetLogObserverTest, AddEventsFromMultipleThreadsWithStopObserving) {
 
   // Join all the threads.
   threads.clear();
+
+  ASSERT_TRUE(LogFilesExist());
 }
 
 TEST_P(FileNetLogObserverTest,
        AddEventsFromMultipleThreadsWithoutStopObserving) {
   const size_t kNumThreads = 10;
   std::vector<std::unique_ptr<base::Thread>> threads(kNumThreads);
-  // Start all the threads. Waiting for them to start is to hopefuly improve
+  // Start all the threads. Waiting for them to start is to hopefully improve
   // the odds of hitting interesting races once events start being added.
   for (size_t i = 0; i < threads.size(); ++i) {
     threads[i] = base::MakeUnique<base::Thread>(
@@ -859,6 +860,9 @@ TEST_P(FileNetLogObserverTest,
 
   // Join all the threads.
   threads.clear();
+
+  // The log file doesn't exist since StopObserving() was not called.
+  ASSERT_FALSE(LogFilesExist());
 }
 
 }  // namespace