Move net-export thread-hopping code into NetLogFileWriter and add IO polled data.

components/net_log/net_log_file_writer.cc

Index: components/net_log/net_log_file_writer.cc
diff --git a/components/net_log/net_log_file_writer.cc b/components/net_log/net_log_file_writer.cc
index db51ca7d2df294065553ee94c372e6bc0723a7e3..78733f8f11831824591514bde7909cd649049511 100644
--- a/components/net_log/net_log_file_writer.cc
+++ b/components/net_log/net_log_file_writer.cc
@@ -6,20 +6,26 @@
 
 #include <utility>
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
+#include "base/single_thread_task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "components/net_log/chrome_net_log.h"
-#include "net/log/write_to_file_net_log_observer.h"
+#include "net/log/file_net_log_observer.h"
+#include "net/log/net_log_util.h"
+#include "net/url_request/url_request_context_getter.h"
 
 namespace net_log {
 
-// Path of logs if relative to default temporary directory of
+// Path of logs relative to default temporary directory given by
 // base::GetTempDir(). Must be kept in sync with
-// chrome/android/java/res/xml/file_paths.xml. Only used if
-// not saving log file to a custom path.
+// chrome/android/java/res/xml/file_paths.xml. Only used if not saving log file
+// to a custom path.
 base::FilePath::CharType kLogRelativePath[] =
     FILE_PATH_LITERAL("net-export/chrome-net-export-log.json");
 
@@ -30,217 +36,316 @@ base::FilePath::CharType kOldLogRelativePath[] =
 
 NetLogFileWriter::~NetLogFileWriter() {
   if (write_to_file_observer_)
-    write_to_file_observer_->StopObserving(nullptr);
+    write_to_file_observer_->StopObserving(nullptr, base::Bind([] {}));
 }
 
-void NetLogFileWriter::ProcessCommand(Command command) {
+NetLogFileWriter::NetLogFileWriter(
+    ChromeNetLog* chrome_net_log,
+    const base::CommandLine::StringType& command_line_string,
+    const std::string& channel_string)
+    : state_(STATE_UNINITIALIZED),
+      log_exists_(false),
+      log_capture_mode_known_(false),
+      log_capture_mode_(net::NetLogCaptureMode::Default()),
+      chrome_net_log_(chrome_net_log),
+      command_line_string_(command_line_string),
+      channel_string_(channel_string),
+      weak_ptr_factory_(this) {}
+
+void NetLogFileWriter::StartNetLog(const base::FilePath& log_path,
+                                   net::NetLogCaptureMode capture_mode,
+                                   const StateCallback& state_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!EnsureInit())
-    return;
+  if (state_ == STATE_UNINITIALIZED) {
+    InitAndCallback(base::Bind(&NetLogFileWriter::StartLogging,

[eroman, 2017/01/11 19:18:38]

Side-comment: A common idiom that we use in //net code is a state-machine using
a single-parameter completion callback. For some examples of that search for
"DoLoop"

To be clear, no action necessary here, the code as written is good; I just
wanted to introduce you to that other approach, which you will be seeing a lot
more of :)

[wangyix1, 2017/01/14 02:15:47]

Acknowledged.

+                               weak_ptr_factory_.GetWeakPtr(), log_path,
+                               capture_mode, state_callback));
+  } else if (state_ == STATE_NOT_LOGGING) {

[eroman, 2017/01/11 19:18:38]

optional: I suggest using a switch statement rather than if/else. The advantage
of switch statements is when you omit a "default" we get compile-time errors
whenever a new value is introduced (which ensures that all code handling state_
gets updated correctly). If you make this change, should be done everywhere
state_ is used for consistency.

[wangyix1, 2017/01/14 02:15:46]

Acknowledged.

+    StartLogging(log_path, capture_mode, state_callback);
+  } else {
+    RunStateCallback(state_callback);
+  }
+}
 
-  switch (command) {
-    case DO_START_LOG_BYTES:
-      StartNetLog(LOG_TYPE_LOG_BYTES);
-      break;
-    case DO_START:
-      StartNetLog(LOG_TYPE_NORMAL);
-      break;
-    case DO_START_STRIP_PRIVATE_DATA:
-      StartNetLog(LOG_TYPE_STRIP_PRIVATE_DATA);
-      break;
-    case DO_STOP:
-      StopNetLog();
-      break;
-    default:
-      NOTREACHED();
-      break;
+void NetLogFileWriter::StartLogging(const base::FilePath& log_path,
+                                    net::NetLogCaptureMode capture_mode,
+                                    const StateCallback& state_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(file_task_runner_);
+
+  // Check if NetLogFileWriter is properly initialized.
+  if (state_ == STATE_NOT_LOGGING) {
+    if (!log_path.empty())
+      log_path_ = log_path;
+
+    state_ = STATE_LOGGING;
+    log_exists_ = true;
+    log_capture_mode_known_ = true;
+    log_capture_mode_ = capture_mode;
+
+    DCHECK(!log_path_.empty());
+
+    std::unique_ptr<base::Value> constants(
+        ChromeNetLog::GetConstants(command_line_string_, channel_string_));
+    write_to_file_observer_.reset(
+        new net::FileNetLogObserver(file_task_runner_));
+    write_to_file_observer_->StartObservingUnbounded(
+        chrome_net_log_, capture_mode, log_path_, std::move(constants),
+        nullptr);
+  } else {
+    // Only way to get here is if StartLogging() was called as a result of a
+    // StartNetLog() that triggered initialization and the initialization
+    // failed. In that case, simply run |state_callback| and nothing else.
+    DCHECK_EQ(state_, STATE_UNINITIALIZED);
   }
+
+  RunStateCallback(state_callback);
 }
 
-bool NetLogFileWriter::GetFilePath(base::FilePath* path) {
+void NetLogFileWriter::StopNetLog(
+    std::unique_ptr<base::DictionaryValue> polled_data,
+    scoped_refptr<net::URLRequestContextGetter> context_getter,
+    const StateCallback& state_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (log_type_ == LOG_TYPE_NONE || state_ == STATE_LOGGING)
-    return false;
+  DCHECK(net_task_runner_);
+  DCHECK(context_getter);
+  if (state_ == STATE_LOGGING) {
+    // Stopping the log requires first grabbing the net info on the net thread.
+    // Before we post that task to the net thread, change state to

[eroman, 2017/01/11 19:18:38]

Can you avoid using "we" pronoun throughout this comment block?

[wangyix1, 2017/01/14 02:15:46]

Done.

+    // STATE_STOP_LOGGING so that if the NetLogFileWriter receives a command
+    // while the net info is being gathered on the net thread, we can check
+    // the state and possibly choose to prevent that command from being
+    // executed. It's the responsibility of the commands (StartNetLog(),
+    // StopNetLog(), GetState()) to check the state before performing their
+    // actions.
+    state_ = STATE_STOPPING_LOG;
+
+    base::PostTaskAndReplyWithResult(
+        net_task_runner_.get(), FROM_HERE,
+        base::Bind(&NetLogFileWriter::AddNetInfo, context_getter,
+                   base::Passed(&polled_data)),
+        base::Bind(&NetLogFileWriter::StopLogging,
+                   weak_ptr_factory_.GetWeakPtr(), state_callback));
+  } else {
+    RunStateCallback(state_callback);
+  }
+}
 
-  if (!NetExportLogExists())
-    return false;
+std::unique_ptr<base::DictionaryValue> NetLogFileWriter::AddNetInfo(
+    scoped_refptr<net::URLRequestContextGetter> context_getter,
+    std::unique_ptr<base::DictionaryValue> polled_data) {
+  DCHECK(context_getter);
+  std::unique_ptr<base::DictionaryValue> net_info = net::GetNetInfo(
+      context_getter->GetURLRequestContext(), net::NET_INFO_ALL_SOURCES);
+  if (polled_data)
+    net_info->MergeDictionary(polled_data.get());
+  return net_info;
+}
 
-  DCHECK(!log_path_.empty());
-#if defined(OS_POSIX)
-  // Users, group and others can read, write and traverse.
-  int mode = base::FILE_PERMISSION_MASK;
-  base::SetPosixFilePermissions(log_path_, mode);
-#endif  // defined(OS_POSIX)
+void NetLogFileWriter::StopLogging(
+    const StateCallback& state_callback,
+    std::unique_ptr<base::DictionaryValue> polled_data) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(state_, STATE_STOPPING_LOG);
 
-  *path = log_path_;
-  return true;
+  write_to_file_observer_->StopObserving(
+      std::move(polled_data),
+      base::Bind(&NetLogFileWriter::ResetObserverThenSetStateNotLogging,
+                 weak_ptr_factory_.GetWeakPtr(), state_callback));
+}
+
+void NetLogFileWriter::ResetObserverThenSetStateNotLogging(
+    const StateCallback& state_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  write_to_file_observer_.reset();
+  state_ = STATE_NOT_LOGGING;
+
+  RunStateCallback(state_callback);
 }
 
-base::DictionaryValue* NetLogFileWriter::GetState() {
+void NetLogFileWriter::GetState(const StateCallback& state_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  base::DictionaryValue* dict = new base::DictionaryValue;
+  if (state_ == STATE_UNINITIALIZED) {
+    InitAndCallback(base::Bind(&NetLogFileWriter::RunStateCallback,
+                               weak_ptr_factory_.GetWeakPtr(), state_callback));
+  } else {
+    RunStateCallback(state_callback);
+  }
+}
 
-  EnsureInit();
+std::unique_ptr<base::DictionaryValue> NetLogFileWriter::GetState() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
 
 #ifndef NDEBUG
   dict->SetString("file", log_path_.LossyDisplayName());
 #endif  // NDEBUG
 
   switch (state_) {
+    case STATE_UNINITIALIZED:
+      dict->SetString("state", "UNINITIALIZED");
+      break;
+    case STATE_INITIALIZING:
+      dict->SetString("state", "INITIALIZING");
+      break;
     case STATE_NOT_LOGGING:
       dict->SetString("state", "NOT_LOGGING");
       break;
     case STATE_LOGGING:
       dict->SetString("state", "LOGGING");
       break;
-    case STATE_UNINITIALIZED:
-      dict->SetString("state", "UNINITIALIZED");
+    case STATE_STOPPING_LOG:
+      dict->SetString("state", "STOPPING_LOG");
       break;
   }
 
-  switch (log_type_) {
-    case LOG_TYPE_NONE:
-      dict->SetString("logType", "NONE");
-      break;
-    case LOG_TYPE_UNKNOWN:
-      dict->SetString("logType", "UNKNOWN");
-      break;
-    case LOG_TYPE_LOG_BYTES:
-      dict->SetString("logType", "LOG_BYTES");
-      break;
-    case LOG_TYPE_NORMAL:
-      dict->SetString("logType", "NORMAL");
-      break;
-    case LOG_TYPE_STRIP_PRIVATE_DATA:
-      dict->SetString("logType", "STRIP_PRIVATE_DATA");
-      break;
-  }
+  dict->SetBoolean("logExists", log_exists_);
+  dict->SetBoolean("logCaptureModeKnown", log_capture_mode_known_);
+  dict->SetString("captureMode", CaptureModeToString(log_capture_mode_));
 
   return dict;
 }
 
-NetLogFileWriter::NetLogFileWriter(
-    ChromeNetLog* chrome_net_log,
-    const base::CommandLine::StringType& command_line_string,
-    const std::string& channel_string)
-    : state_(STATE_UNINITIALIZED),
-      log_type_(LOG_TYPE_NONE),
-      chrome_net_log_(chrome_net_log),
-      command_line_string_(command_line_string),
-      channel_string_(channel_string) {
-  // NetLogFileWriter can be created on one thread and used on another.
-  thread_checker_.DetachFromThread();
+void NetLogFileWriter::RunStateCallback(
+    const StateCallback& state_callback) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  state_callback.Run(GetState());
 }
 
-bool NetLogFileWriter::GetNetExportLogBaseDirectory(
-    base::FilePath* path) const {
+void NetLogFileWriter::GetFilePath(
+    const FilePathCallback& path_callback) const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return base::GetTempDir(path);
-}
-
-net::NetLogCaptureMode NetLogFileWriter::GetCaptureModeForLogType(
-    LogType log_type) {
-  switch (log_type) {
-    case LOG_TYPE_LOG_BYTES:
-      return net::NetLogCaptureMode::IncludeSocketBytes();
-    case LOG_TYPE_NORMAL:
-      return net::NetLogCaptureMode::IncludeCookiesAndCredentials();
-    case LOG_TYPE_STRIP_PRIVATE_DATA:
-      return net::NetLogCaptureMode::Default();
-    case LOG_TYPE_NONE:
-    case LOG_TYPE_UNKNOWN:
-      NOTREACHED();
+  if (!log_exists_ || state_ == STATE_LOGGING) {
+    path_callback.Run(base::FilePath());
+    return;
   }
-  return net::NetLogCaptureMode::Default();
-}
 
-bool NetLogFileWriter::EnsureInit() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ != STATE_UNINITIALIZED)
-    return true;
+  DCHECK(file_task_runner_);
+  DCHECK(!log_path_.empty());
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
+      base::Bind(&NetLogFileWriter::SetPosixFilePermissionsAll, log_path_),
+      base::Bind(&NetLogFileWriter::RunFilePathCallback, path_callback,
+                 log_path_));
+}
 
-  if (log_path_.empty() && !SetUpDefaultNetExportLogPath())
+bool NetLogFileWriter::SetPosixFilePermissionsAll(const base::FilePath& path) {
+  if (!base::PathExists(path))
     return false;
+#if defined(OS_POSIX)
+  return base::SetPosixFilePermissions(path, base::FILE_PERMISSION_MASK);
+#else
+  return true;
+#endif
+}
 
-  state_ = STATE_NOT_LOGGING;
-  if (NetExportLogExists())
-    log_type_ = LOG_TYPE_UNKNOWN;
+void NetLogFileWriter::RunFilePathCallback(
+    const FilePathCallback& path_callback,
+    const base::FilePath& path,
+    bool set_file_permissions_success) {
+  if (set_file_permissions_success)
+    path_callback.Run(path);
   else
-    log_type_ = LOG_TYPE_NONE;
-
-  return true;
+    path_callback.Run(base::FilePath());
 }
 
-void NetLogFileWriter::StartNetLog(LogType log_type) {
+void NetLogFileWriter::InitAndCallback(const base::Closure& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ == STATE_LOGGING)
-    return;
-
-  DCHECK_NE(STATE_UNINITIALIZED, state_);
-  DCHECK(!log_path_.empty());
+  DCHECK(file_task_runner_);
+
+  // Before posting the file thread tasks, change state to STATE_INITIALIZING so
+  // that if the NetLogFileWriter receives a command while initialization tasks
+  // are running on the file thread, we can check the state and possibly choose
+  // to prevent that command from being executed. It's the responsibility of the
+  // commands (StartNetLog(), StopNetLog(), GetState()) to check the state
+  // before performing their actions.
+  DCHECK_EQ(state_, STATE_UNINITIALIZED);
+  state_ = STATE_INITIALIZING;
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
+      base::Bind(&NetLogFileWriter::SetUpDefaultLogPath),
+      base::Bind(&NetLogFileWriter::InitStateAndCallback,
+                 weak_ptr_factory_.GetWeakPtr(), callback));
+}
 
-  // Try to make sure we can create the file.
-  // TODO(rtenneti): Find a better for doing the following. Surface some error
-  // to the user if we couldn't create the file.
-  base::ScopedFILE file(base::OpenFile(log_path_, "w"));
-  if (!file)
-    return;
+NetLogFileWriter::DefaultLogPathResults
+    NetLogFileWriter::SetUpDefaultLogPath() {
+  DefaultLogPathResults results;
+  results.default_log_path_success = false;

[eroman, 2017/01/11 19:18:38]

Can you also initialize log_exists = false ? (as there is a short-circuit which
may leave this uninitialized on return).

(The downstream code (InitStateAndCallback) doesn't currently care as it only
accesses log_exists if default_log_path_success, but it is still good hygiene to
do so.)

[wangyix1, 2017/01/14 02:15:47]

Done.

 
-  log_type_ = log_type;
-  state_ = STATE_LOGGING;
+  base::FilePath temp_dir;
+  if (!base::GetTempDir(&temp_dir))
+    return results;
 
-  std::unique_ptr<base::Value> constants(
-      ChromeNetLog::GetConstants(command_line_string_, channel_string_));
-  write_to_file_observer_.reset(new net::WriteToFileNetLogObserver());
-  write_to_file_observer_->set_capture_mode(GetCaptureModeForLogType(log_type));
-  write_to_file_observer_->StartObserving(chrome_net_log_, std::move(file),
-                                          constants.get(), nullptr);
-}
+  // Delete log file at old location, if present.
+  base::DeleteFile(temp_dir.Append(kOldLogRelativePath), false);
 
-void NetLogFileWriter::StopNetLog() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ != STATE_LOGGING)
-    return;
+  results.default_log_path = temp_dir.Append(kLogRelativePath);
+  if (!base::CreateDirectoryAndGetError(results.default_log_path.DirName(),
+                                        nullptr))
+    return results;
 
-  write_to_file_observer_->StopObserving(nullptr);
-  write_to_file_observer_.reset();
-  state_ = STATE_NOT_LOGGING;
+  results.log_exists = base::PathExists(results.default_log_path);
+  results.default_log_path_success = true;
+  return results;
 }
 
-void NetLogFileWriter::SetUpNetExportLogPath(
-    const base::FilePath& custom_path) {
+void NetLogFileWriter::InitStateAndCallback(
+    const base::Closure& callback,
+    const DefaultLogPathResults& results) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(state_, STATE_INITIALIZING);
+
+  if (results.default_log_path_success) {
+    state_ = STATE_NOT_LOGGING;
+    log_path_ = results.default_log_path;
+    log_exists_ = results.log_exists;
+    DCHECK(!log_capture_mode_known_);
+  } else {
+    state_ = STATE_UNINITIALIZED;
+  }
 
-  // The directory should always exist because the custom path
-  // is taken from a file selector dialog window.
-  DCHECK(base::PathExists(custom_path.DirName()));
-
-  log_path_ = custom_path;
+  callback.Run();
 }
 
-bool NetLogFileWriter::SetUpDefaultNetExportLogPath() {
+void NetLogFileWriter::SetTaskRunners(
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> net_task_runner) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  base::FilePath temp_dir;
-  if (!GetNetExportLogBaseDirectory(&temp_dir))
-    return false;
-
-  // Delete log file at old location, if present.
-  DeleteFile(temp_dir.Append(kOldLogRelativePath), false);
+  if (file_task_runner_)
+    DCHECK_EQ(file_task_runner, file_task_runner_);
+  file_task_runner_ = file_task_runner;
 
-  base::FilePath log_path = temp_dir.Append(kLogRelativePath);
+  if (net_task_runner_)
+    DCHECK_EQ(net_task_runner, net_task_runner_);
+  net_task_runner_ = net_task_runner;
+}
 
-  if (!base::CreateDirectoryAndGetError(log_path.DirName(), nullptr)) {
-    return false;
+std::string NetLogFileWriter::CaptureModeToString(
+    net::NetLogCaptureMode capture_mode) {
+  if (capture_mode == net::NetLogCaptureMode::Default()) {
+    return "STRIP_PRIVATE_DATA";
+  } else if (capture_mode ==
+             net::NetLogCaptureMode::IncludeCookiesAndCredentials()) {
+    return "NORMAL";
+  } else {

[eroman, 2017/01/11 19:18:38]

Please change the default case.

If |capture_mode| has an unknown value we should return "NORMAL" rather than
"LOG_BYTES".

Moreover, I suggest adding a NOT_REACHED() for that case (as it implies our
conversion code is incomplete).

[wangyix1, 2017/01/14 02:15:46]

Done.

+    DCHECK(capture_mode == net::NetLogCaptureMode::IncludeSocketBytes());
+    return "LOG_BYTES";
   }
-
-  log_path_ = log_path;
-  return true;
 }
 
-bool NetLogFileWriter::NetExportLogExists() const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!log_path_.empty());
-  return base::PathExists(log_path_);
+net::NetLogCaptureMode NetLogFileWriter::CaptureModeFromString(
+    const std::string& capture_mode_string) {
+  if (capture_mode_string == "STRIP_PRIVATE_DATA") {
+    return net::NetLogCaptureMode::Default();
+  } else if (capture_mode_string == "NORMAL") {
+    return net::NetLogCaptureMode::IncludeCookiesAndCredentials();
+  } else {
+    DCHECK_EQ(capture_mode_string, "LOG_BYTES");

[eroman, 2017/01/11 19:18:38]

Same comment as above.

The default (unrecognized string) case should be "NORMAL" and not "LOG_BYTES".

This is important since LOG_BYTES is pretty heavy-weight, and we wouldn't want
to default to it if something went wrong.

Moreover, if the string cannot be parsed let's add a NOT_REACHED(), since it
implies a misunderstanding between this C++ code and the javascript frontend.

[wangyix1, 2017/01/14 02:15:46]

Done.

+    return net::NetLogCaptureMode::IncludeSocketBytes();
+  }
 }
 
 }  // namespace net_log