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..f03c386d817f8cf3e861feb3cbd66e500be1aefe 100644
--- a/components/net_log/net_log_file_writer.cc
+++ b/components/net_log/net_log_file_writer.cc
@@ -6,13 +6,19 @@
 
 #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 {
 
@@ -30,58 +36,115 @@ 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) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!EnsureInit())
-    return;
+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) {
+  // NetLogFileWriter can be created on one thread and used on another.
+  thread_checker_.DetachFromThread();

[eroman, 2017/01/06 21:46:23]

Is this necessary? I believe the creation and destruction happens from the UI
thread (as do all the public entry points), so this is no longer needed.

[wangyix1, 2017/01/10 22:20:15]

Done.

+}
 
-  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::StartNetLog(net::NetLogCaptureMode capture_mode,
+    StateCallback state_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (state_ == STATE_UNINITIALIZED) {
+    InitAndCallback(
+        base::Bind(&NetLogFileWriter::StartLogging,
+                   weak_ptr_factory_.GetWeakPtr(), capture_mode,
+                   state_callback));
+  } else if (state_ == STATE_NOT_LOGGING) {
+    StartLogging(capture_mode, state_callback);
   }
 }
 
-bool NetLogFileWriter::GetFilePath(base::FilePath* path) {
+void NetLogFileWriter::StartLogging(net::NetLogCaptureMode capture_mode,
+    StateCallback state_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (log_type_ == LOG_TYPE_NONE || state_ == STATE_LOGGING)
-    return false;
-
-  if (!NetExportLogExists())
-    return false;
+  DCHECK(file_task_runner_);
 
+  DCHECK_EQ(STATE_NOT_LOGGING, state_);
   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)
 
-  *path = log_path_;
-  return true;
+  state_ = STATE_LOGGING;
+  log_exists_ = true;
+  log_capture_mode_known_ = true;
+  log_capture_mode_ = capture_mode;
+
+  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_,

[eroman, 2017/01/06 21:46:23]

StartLogging() will eventually need to take a URLRequestContextGetter parameter
(crbug.com/679021)

Don't need to address that in this changelist, but something to be aware of in
the design

I think StartLogging() will want to eventually take a URLRequestContextGetter,
and do a thread-hop to the IO thread in order to start logging here.

[wangyix1, 2017/01/10 22:20:15]

Acknowledged.

+      capture_mode, log_path_, std::move(constants), nullptr);
+
+  RunStateCallback(state_callback);
+}
+
+void NetLogFileWriter::StopNetLog(
+    std::unique_ptr<base::DictionaryValue> polled_data,
+    scoped_refptr<net::URLRequestContextGetter> context_getter,
+    StateCallback state_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(net_task_runner_);
+  DCHECK(context_getter);
+  if (state_ == STATE_LOGGING) {
+    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));
+  }
+}
+
+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;
+}
+
+void NetLogFileWriter::StopLogging(StateCallback state_callback,
+    std::unique_ptr<base::DictionaryValue> polled_data) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(STATE_LOGGING, state_);

[eroman, 2017/01/06 21:46:23]

I don't think this DCHECK holds() -- Let's say StopNetLog() was called at about
the same time from 2 different tabs. One of them would hit this DCHECK().

[wangyix1, 2017/01/10 22:20:15]

Done.

+  write_to_file_observer_->StopObserving(std::move(polled_data),
+                                         base::Bind([] {}));

[eroman, 2017/01/06 21:46:23]

Shouldn't we wait for this to complete before reporting completion of
StopLogging?

In particular, is there a possible race between stopping logging, transitioning
the state to STATE_NOT_LOGGING, and then emailing the file (however the file has
not finished writing yet).

[wangyix1, 2017/01/10 22:20:15]

Done.

+  write_to_file_observer_.reset();
+  state_ = STATE_NOT_LOGGING;
+
+  RunStateCallback(state_callback);
+}
+
+void NetLogFileWriter::GetState(StateCallback state_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (state_ == STATE_UNINITIALIZED) {
+    InitAndCallback(base::Bind(&NetLogFileWriter::RunStateCallback,
+                               weak_ptr_factory_.GetWeakPtr(), state_callback));
+  } else {
+    RunStateCallback(state_callback);
+  }
 }
 
 base::DictionaryValue* NetLogFileWriter::GetState() {
   DCHECK(thread_checker_.CalledOnValidThread());
   base::DictionaryValue* dict = new base::DictionaryValue;
 
-  EnsureInit();
-
 #ifndef NDEBUG
   dict->SetString("file", log_path_.LossyDisplayName());
 #endif  // NDEBUG
@@ -98,149 +161,129 @@ base::DictionaryValue* NetLogFileWriter::GetState() {
       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;
-  }
-
-  return dict;
-}
+  dict->SetBoolean("logExists", log_exists_);
+  dict->SetBoolean("logCaptureModeKnown", log_capture_mode_known_);
 
-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();
-}
+  if (log_capture_mode_ == net::NetLogCaptureMode::Default())
+      dict->SetString("captureMode", "STRIP_PRIVATE_DATA");
+  else if (log_capture_mode_ ==
+           net::NetLogCaptureMode::IncludeCookiesAndCredentials())
+      dict->SetString("captureMode", "NORMAL");
+  else if (log_capture_mode_ == net::NetLogCaptureMode::IncludeSocketBytes())
+      dict->SetString("captureMode", "LOG_BYTES");

[eroman, 2017/01/06 21:46:23]

Formatting.

[wangyix1, 2017/01/10 22:20:15]

Done.

 
-bool NetLogFileWriter::GetNetExportLogBaseDirectory(
-    base::FilePath* path) 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();
-  }
-  return net::NetLogCaptureMode::Default();
+  return dict;
 }
 
-bool NetLogFileWriter::EnsureInit() {
+void NetLogFileWriter::RunStateCallback(StateCallback state_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ != STATE_UNINITIALIZED)
-    return true;
-
-  if (log_path_.empty() && !SetUpDefaultNetExportLogPath())
-    return false;
-
-  state_ = STATE_NOT_LOGGING;
-  if (NetExportLogExists())
-    log_type_ = LOG_TYPE_UNKNOWN;
-  else
-    log_type_ = LOG_TYPE_NONE;
-
-  return true;
+  state_callback.Run(std::unique_ptr<base::DictionaryValue>(GetState()));

[eroman, 2017/01/06 21:46:22]

See comment about making GetState() return a unique_ptr

[wangyix1, 2017/01/10 22:20:15]

Done.

 }
 
-void NetLogFileWriter::StartNetLog(LogType log_type) {
+void NetLogFileWriter::GetFilePath(FilePathCallback success_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ == STATE_LOGGING)
+  if (!log_exists_ || state_ == STATE_LOGGING)
     return;

[eroman, 2017/01/06 21:46:23]

Would it make sense to always invoke the callback, but possibly with an empty
path to indicate "failure"?

[wangyix1, 2017/01/10 22:20:15]

Done.

 
-  DCHECK_NE(STATE_UNINITIALIZED, state_);
+  DCHECK(file_task_runner_);
   DCHECK(!log_path_.empty());
 
-  // 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;
-
-  log_type_ = log_type;
-  state_ = STATE_LOGGING;
-
-  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);
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
+      base::Bind(&NetLogFileWriter::SetPosixFilePermissionsAll, log_path_),
+      base::Bind(&NetLogFileWriter::RunFilePathCallback, success_callback,
+                 log_path_));
 }
 
-void NetLogFileWriter::StopNetLog() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (state_ != STATE_LOGGING)
-    return;
+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
+}
 
-  write_to_file_observer_->StopObserving(nullptr);
-  write_to_file_observer_.reset();
-  state_ = STATE_NOT_LOGGING;
+void NetLogFileWriter::RunFilePathCallback(FilePathCallback path_callback,
+                                           const base::FilePath& path,
+                                           bool runCallback) {
+  if (runCallback)

[eroman, 2017/01/06 21:46:22]

hacker_style_notation rather than camelCaseNotation

[wangyix1, 2017/01/10 22:20:15]

Done.

+    path_callback.Run(path);
 }
 
 void NetLogFileWriter::SetUpNetExportLogPath(
     const base::FilePath& custom_path) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  // 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;
 }
 
-bool NetLogFileWriter::SetUpDefaultNetExportLogPath() {
+void NetLogFileWriter::InitAndCallback(base::Closure success_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(file_task_runner_);
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
+      base::Bind(&NetLogFileWriter::SetUpDefaultLogPath),
+      base::Bind(&NetLogFileWriter::InitStateAndCallback,
+                 weak_ptr_factory_.GetWeakPtr(), success_callback));
+}
+
+NetLogFileWriter::DefaultLogPathResults
+    NetLogFileWriter::SetUpDefaultLogPath() {
+  DefaultLogPathResults results;
+  results.default_log_path_success = false;
+
   base::FilePath temp_dir;
-  if (!GetNetExportLogBaseDirectory(&temp_dir))
-    return false;
+  if (!base::GetTempDir(&temp_dir))
+    return results;
 
   // Delete log file at old location, if present.
-  DeleteFile(temp_dir.Append(kOldLogRelativePath), false);
+  base::DeleteFile(temp_dir.Append(kOldLogRelativePath), false);
 
-  base::FilePath log_path = temp_dir.Append(kLogRelativePath);
+  results.default_log_path = temp_dir.Append(kLogRelativePath);
+  if (!base::CreateDirectoryAndGetError(results.default_log_path.DirName(),
+                                        nullptr))
+    return results;
 
-  if (!base::CreateDirectoryAndGetError(log_path.DirName(), nullptr)) {
-    return false;
-  }
-
-  log_path_ = log_path;
-  return true;
+  results.log_exists = base::PathExists(results.default_log_path);
+  results.default_log_path_success = true;
+  return results;
 }
 
-bool NetLogFileWriter::NetExportLogExists() const {
+void NetLogFileWriter::InitStateAndCallback(base::Closure sucess_callback,
+    const DefaultLogPathResults& results) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!log_path_.empty());
-  return base::PathExists(log_path_);
+
+  if (!results.default_log_path_success)
+    return;
+
+  log_path_ = results.default_log_path;
+  state_ = STATE_NOT_LOGGING;
+  log_exists_ = results.log_exists;
+  DCHECK(!log_capture_mode_known_);
+
+  sucess_callback.Run();
+}
+
+void NetLogFileWriter::SetFileTaskRunner(
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner) {
+  file_task_runner_ = file_task_runner;
+}
+
+void NetLogFileWriter::SetNetTaskRunner(
+    scoped_refptr<base::SingleThreadTaskRunner> net_task_runner) {
+  net_task_runner_ = net_task_runner;
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+    NetLogFileWriter::GetFileTaskRunner() const {
+  return file_task_runner_;
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+    NetLogFileWriter::GetNetTaskRunner() const {
+  return net_task_runner_;
 }
 
 }  // namespace net_log