refactor: netLog directly uses network service (#18289)

atom/browser/api/atom_api_net_log.cc

@@ -8,6 +8,7 @@
 
 #include "atom/browser/atom_browser_context.h"
 #include "atom/browser/net/system_network_context_manager.h"
+#include "atom/common/atom_version.h"
 #include "atom/common/native_mate_converters/callback.h"
 #include "atom/common/native_mate_converters/file_path_converter.h"
 #include "atom/common/node_includes.h"
@@ -21,103 +22,154 @@
 
 namespace atom {
 
+namespace {
+
+scoped_refptr<base::SequencedTaskRunner> CreateFileTaskRunner() {
+  // The tasks posted to this sequenced task runner do synchronous File I/O for
+  // checking paths and setting permissions on files.
+  //
+  // These operations can be skipped on shutdown since FileNetLogObserver's API
+  // doesn't require things to have completed until notified of completion.
+  return base::CreateSequencedTaskRunnerWithTraits(
+      {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
+       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
+}
+
+base::File OpenFileForWriting(base::FilePath path) {
+  return base::File(path,
+                    base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+}
+
+void ResolvePromiseWithNetError(util::Promise promise, int32_t error) {
+  if (error == net::OK) {
+    promise.Resolve();
+  } else {
+    promise.RejectWithErrorMessage(net::ErrorToString(error));
+  }
+}
+
+}  // namespace
+
 namespace api {
 
 NetLog::NetLog(v8::Isolate* isolate, AtomBrowserContext* browser_context)
-    : browser_context_(browser_context) {
+    : browser_context_(browser_context), weak_ptr_factory_(this) {
   Init(isolate);
-
-  net_log_writer_ = g_browser_process->system_network_context_manager()
-                        ->GetNetExportFileWriter();
-  net_log_writer_->AddObserver(this);
+  file_task_runner_ = CreateFileTaskRunner();
 }
 
-NetLog::~NetLog() {
-  net_log_writer_->RemoveObserver(this);
-}
+NetLog::~NetLog() = default;
 
-void NetLog::StartLogging(mate::Arguments* args) {
+v8::Local<v8::Promise> NetLog::StartLogging(mate::Arguments* args) {
   base::FilePath log_path;
   if (!args->GetNext(&log_path) || log_path.empty()) {
     args->ThrowError("The first parameter must be a valid string");
-    return;
+    return v8::Local<v8::Promise>();
   }
 
+  if (net_log_exporter_) {
+    args->ThrowError("There is already a net log running");
+    return v8::Local<v8::Promise>();
+  }
+
+  pending_start_promise_ = base::make_optional<util::Promise>(isolate());
+  v8::Local<v8::Promise> handle = pending_start_promise_->GetHandle();
+
+  auto command_line_string =
+      base::CommandLine::ForCurrentProcess()->GetCommandLineString();
+  auto channel_string = std::string("Electron " ATOM_VERSION);
+  base::Value custom_constants = base::Value::FromUniquePtrValue(
+      net_log::ChromeNetLog::GetPlatformConstants(command_line_string,
+                                                  channel_string));
+
   auto* network_context =
       content::BrowserContext::GetDefaultStoragePartition(browser_context_)
           ->GetNetworkContext();
 
+  network_context->CreateNetLogExporter(mojo::MakeRequest(&net_log_exporter_));
+  net_log_exporter_.set_connection_error_handler(
+      base::BindOnce(&NetLog::OnConnectionError, base::Unretained(this)));
+
   // TODO(deepak1556): Provide more flexibility to this module
   // by allowing customizations on the capturing options.
-  net_log_writer_->StartNetLog(
-      log_path, net::NetLogCaptureMode::Default(),
-      net_log::NetExportFileWriter::kNoLimit /* file size limit */,
-      base::CommandLine::ForCurrentProcess()->GetCommandLineString(),
-      std::string(), network_context);
+  auto capture_mode = network::mojom::NetLogCaptureMode::DEFAULT;
+  auto max_file_size = network::mojom::NetLogExporter::kUnlimitedFileSize;
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
+      base::BindOnce(OpenFileForWriting, log_path),
+      base::BindOnce(&NetLog::StartNetLogAfterCreateFile,
+                     weak_ptr_factory_.GetWeakPtr(), capture_mode,
+                     max_file_size, std::move(custom_constants)));
+
+  return handle;
 }
 
-std::string NetLog::GetLoggingState() const {
-  if (!net_log_state_)
-    return std::string();
-  const base::Value* current_log_state =
-      net_log_state_->FindKeyOfType("state", base::Value::Type::STRING);
-  if (!current_log_state)
-    return std::string();
-  return current_log_state->GetString();
+void NetLog::StartNetLogAfterCreateFile(
+    network::mojom::NetLogCaptureMode capture_mode,
+    uint64_t max_file_size,
+    base::Value custom_constants,
+    base::File output_file) {
+  if (!net_log_exporter_) {
+    // Theoretically the mojo pipe could have been closed by the time we get
+    // here via the connection error handler. If so, the promise has already
+    // been resolved.
+    return;
+  }
+  DCHECK(pending_start_promise_);
+  if (!output_file.IsValid()) {
+    std::move(*pending_start_promise_)
+        .RejectWithErrorMessage(
+            base::File::ErrorToString(output_file.error_details()));
+    net_log_exporter_.reset();
+    return;
+  }
+  net_log_exporter_->Start(
+      std::move(output_file), std::move(custom_constants), capture_mode,
+      max_file_size,
+      base::BindOnce(&NetLog::NetLogStarted, base::Unretained(this)));
+}
+
+void NetLog::NetLogStarted(int32_t error) {
+  DCHECK(pending_start_promise_);
+  ResolvePromiseWithNetError(std::move(*pending_start_promise_), error);
+}
+
+void NetLog::OnConnectionError() {
+  net_log_exporter_.reset();
+  if (pending_start_promise_) {
+    std::move(*pending_start_promise_)
+        .RejectWithErrorMessage("Failed to start net log exporter");
+  }
 }
 
 bool NetLog::IsCurrentlyLogging() const {
-  const std::string log_state = GetLoggingState();
-  return (log_state == "STARTING_LOG") || (log_state == "LOGGING");
-}
-
-std::string NetLog::GetCurrentlyLoggingPath() const {
-  // Net log exporter has a default path which will be used
-  // when no log path is provided, but since we don't allow
-  // net log capture without user provided file path, this
-  // check is completely safe.
-  if (IsCurrentlyLogging()) {
-    const base::Value* current_log_path =
-        net_log_state_->FindKeyOfType("file", base::Value::Type::STRING);
-    if (current_log_path)
-      return current_log_path->GetString();
-  }
-
-  return std::string();
+  return !!net_log_exporter_;
 }
 
 v8::Local<v8::Promise> NetLog::StopLogging(mate::Arguments* args) {
   util::Promise promise(isolate());
   v8::Local<v8::Promise> handle = promise.GetHandle();
 
-  if (IsCurrentlyLogging()) {
-    stop_callback_queue_.emplace_back(std::move(promise));
-    net_log_writer_->StopNetLog(nullptr);
+  if (net_log_exporter_) {
+    // Move the net_log_exporter_ into the callback to ensure that the mojo
+    // pointer lives long enough to resolve the promise. Moving it into the
+    // callback will cause the instance variable to become empty.
+    net_log_exporter_->Stop(
+        base::Value(base::Value::Type::DICTIONARY),
+        base::BindOnce(
+            [](network::mojom::NetLogExporterPtr, util::Promise promise,
+               int32_t error) {
+              ResolvePromiseWithNetError(std::move(promise), error);
+            },
+            std::move(net_log_exporter_), std::move(promise)));
   } else {
-    promise.Resolve(base::FilePath());
+    promise.RejectWithErrorMessage("No net log in progress");
   }
 
   return handle;
 }
 
-void NetLog::OnNewState(const base::DictionaryValue& state) {
-  net_log_state_ = state.CreateDeepCopy();
-
-  if (stop_callback_queue_.empty())
-    return;
-
-  if (GetLoggingState() == "NOT_LOGGING") {
-    for (auto& promise : stop_callback_queue_) {
-      // TODO(zcbenz): Remove the use of CopyablePromise when the
-      // GetFilePathToCompletedLog API accepts OnceCallback.
-      net_log_writer_->GetFilePathToCompletedLog(base::BindRepeating(
-          util::CopyablePromise::ResolveCopyablePromise<const base::FilePath&>,
-          util::CopyablePromise(promise)));
-    }
-    stop_callback_queue_.clear();
-  }
-}
-
 // static
 mate::Handle<NetLog> NetLog::Create(v8::Isolate* isolate,
                                     AtomBrowserContext* browser_context) {
@@ -130,7 +182,6 @@ void NetLog::BuildPrototype(v8::Isolate* isolate,
   prototype->SetClassName(mate::StringToV8(isolate, "NetLog"));
   mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
       .SetProperty("currentlyLogging", &NetLog::IsCurrentlyLogging)
-      .SetProperty("currentlyLoggingPath", &NetLog::GetCurrentlyLoggingPath)
       .SetMethod("startLogging", &NetLog::StartLogging)
       .SetMethod("stopLogging", &NetLog::StopLogging);
 }

atom/browser/api/atom_api_net_log.h

@@ -12,9 +12,10 @@
 #include "atom/browser/api/trackable_object.h"
 #include "atom/common/promise_util.h"
 #include "base/callback.h"
+#include "base/optional.h"
 #include "base/values.h"
-#include "components/net_log/net_export_file_writer.h"
 #include "native_mate/handle.h"
+#include "services/network/public/mojom/net_log.mojom.h"
 
 namespace atom {
 
@@ -22,8 +23,7 @@ class AtomBrowserContext;
 
 namespace api {
 
-class NetLog : public mate::TrackableObject<NetLog>,
-               public net_log::NetExportFileWriter::StateObserver {
+class NetLog : public mate::TrackableObject<NetLog> {
  public:
   static mate::Handle<NetLog> Create(v8::Isolate* isolate,
                                      AtomBrowserContext* browser_context);
@@ -31,24 +31,33 @@ class NetLog : public mate::TrackableObject<NetLog>,
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::FunctionTemplate> prototype);
 
-  void StartLogging(mate::Arguments* args);
-  std::string GetLoggingState() const;
-  bool IsCurrentlyLogging() const;
-  std::string GetCurrentlyLoggingPath() const;
+  v8::Local<v8::Promise> StartLogging(mate::Arguments* args);
   v8::Local<v8::Promise> StopLogging(mate::Arguments* args);
+  bool IsCurrentlyLogging() const;
 
  protected:
   explicit NetLog(v8::Isolate* isolate, AtomBrowserContext* browser_context);
   ~NetLog() override;
 
-  // net_log::NetExportFileWriter::StateObserver implementation
-  void OnNewState(const base::DictionaryValue& state) override;
+  void OnConnectionError();
+
+  void StartNetLogAfterCreateFile(
+      network::mojom::NetLogCaptureMode capture_mode,
+      uint64_t max_file_size,
+      base::Value custom_constants,
+      base::File output_file);
+  void NetLogStarted(int32_t error);
 
  private:
   AtomBrowserContext* browser_context_;
-  net_log::NetExportFileWriter* net_log_writer_;
-  std::list<atom::util::Promise> stop_callback_queue_;
-  std::unique_ptr<base::DictionaryValue> net_log_state_;
+
+  network::mojom::NetLogExporterPtr net_log_exporter_;
+
+  base::Optional<util::Promise> pending_start_promise_;
+
+  scoped_refptr<base::TaskRunner> file_task_runner_;
+
+  base::WeakPtrFactory<NetLog> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NetLog);
 };

docs/api/net-log.md

@@ -7,8 +7,8 @@ Process: [Main](../glossary.md#main-process)
 ```javascript
 const { netLog } = require('electron')
 
-app.on('ready', async function () {
-  netLog.startLogging('/path/to/net-log')
+app.on('ready', async () => {
+  await netLog.startLogging('/path/to/net-log')
   // After some network events
   const path = await netLog.stopLogging()
   console.log('Net-logs written to', path)
@@ -26,6 +26,8 @@ of the `app` module gets emitted.
 
 * `path` String - File path to record network logs.
 
+Returns `Promise<void>` - resolves when the net log has begun recording.
+
 Starts recording network events to `path`.
 
 ### `netLog.stopLogging()`
@@ -40,6 +42,6 @@ Stops recording network events. If not called, net logging will automatically en
 
 A `Boolean` property that indicates whether network logs are recorded.
 
-### `netLog.currentlyLoggingPath`
+### `netLog.currentlyLoggingPath` **Deprecated**
 
 A `String` property that returns the path to the current log file.

lib/browser/api/session.js

@@ -22,3 +22,31 @@ Object.setPrototypeOf(Cookies.prototype, EventEmitter.prototype)
 Session.prototype._init = function () {
   app.emit('session-created', this)
 }
+
+const _originalStartLogging = NetLog.prototype.startLogging
+NetLog.prototype.startLogging = function (path) {
+  this._currentlyLoggingPath = path
+  try {
+    return _originalStartLogging.call(this, path)
+  } catch (e) {
+    this._currentlyLoggingPath = null
+    throw e
+  }
+}
+
+const _originalStopLogging = NetLog.prototype.stopLogging
+NetLog.prototype.stopLogging = function () {
+  this._currentlyLoggingPath = null
+  return _originalStopLogging.call(this)
+}
+
+const currentlyLoggingPathDeprecated = deprecate.warnOnce('currentlyLoggingPath')
+Object.defineProperties(NetLog.prototype, {
+  currentlyLoggingPath: {
+    enumerable: true,
+    get () {
+      currentlyLoggingPathDeprecated()
+      return this._currentlyLoggingPath == null ? '' : this._currentlyLoggingPath
+    }
+  }
+})

lib/common/api/deprecate.ts

@@ -14,6 +14,7 @@ function warnOnce (oldName: string, newName?: string) {
 }
 
 const deprecate: ElectronInternal.DeprecationUtil = {
+  warnOnce,
   setHandler: (handler) => { deprecationHandler = handler },
   getHandler: () => deprecationHandler,
   warn: (oldName, newName) => {

spec-main/api-net-log-spec.js

@@ -5,15 +5,14 @@ const fs = require('fs')
 const os = require('os')
 const path = require('path')
 const ChildProcess = require('child_process')
-const { remote } = require('electron')
-const { session } = remote
+const {session} = require('electron')
 const appPath = path.join(__dirname, 'fixtures', 'api', 'net-log')
 const dumpFile = path.join(os.tmpdir(), 'net_log.json')
 const dumpFileDynamic = path.join(os.tmpdir(), 'net_log_dynamic.json')
 
 const { expect } = chai
 chai.use(dirtyChai)
-const isCI = remote.getGlobal('isCi')
+const isCI = global.isCI
 const netLog = session.fromPartition('net-log').netLog
 
 describe('netLog module', () => {
@@ -47,6 +46,9 @@ describe('netLog module', () => {
     })
   })
 
+  beforeEach(() => {
+    expect(netLog.currentlyLogging).to.be.false()
+  })
   afterEach(() => {
     try {
       if (fs.existsSync(dumpFile)) {
@@ -58,36 +60,29 @@ describe('netLog module', () => {
     } catch (e) {
       // Ignore error
     }
+    expect(netLog.currentlyLogging).to.be.false()
   })
 
   it('should begin and end logging to file when .startLogging() and .stopLogging() is called', async () => {
-    expect(netLog.currentlyLogging).to.be.false()
-    expect(netLog.currentlyLoggingPath).to.equal('')
-
-    netLog.startLogging(dumpFileDynamic)
+    await netLog.startLogging(dumpFileDynamic)
 
     expect(netLog.currentlyLogging).to.be.true()
+
     expect(netLog.currentlyLoggingPath).to.equal(dumpFileDynamic)
 
-    const path = await netLog.stopLogging()
+    await netLog.stopLogging()
 
-    expect(netLog.currentlyLogging).to.be.false()
-    expect(netLog.currentlyLoggingPath).to.equal('')
-
-    expect(path).to.equal(dumpFileDynamic)
     expect(fs.existsSync(dumpFileDynamic)).to.be.true()
   })
 
-  it('should silence when .stopLogging() is called without calling .startLogging()', async () => {
-    expect(netLog.currentlyLogging).to.be.false()
-    expect(netLog.currentlyLoggingPath).to.equal('')
+  it('should throw an error when .stopLogging() is called without calling .startLogging()', async () => {
+    await expect(netLog.stopLogging()).to.be.rejectedWith('No net log in progress')
+  })
 
-    const path = await netLog.stopLogging()
-
-    expect(netLog.currentlyLogging).to.be.false()
-    expect(netLog.currentlyLoggingPath).to.equal('')
-
-    expect(path).to.equal('')
+  it('should throw an error when .startLogging() is called with an invalid argument', () => {
+    expect(() => netLog.startLogging('')).to.throw()
+    expect(() => netLog.startLogging(null)).to.throw()
+    expect(() => netLog.startLogging([])).to.throw()
   })
 
   it('should begin and end logging automatically when --log-net-log is passed', done => {
@@ -96,7 +91,7 @@ describe('netLog module', () => {
       return
     }
 
-    const appProcess = ChildProcess.spawn(remote.process.execPath,
+    const appProcess = ChildProcess.spawn(process.execPath,
       [appPath], {
         env: {
           TEST_REQUEST_URL: server.url,
@@ -110,14 +105,13 @@ describe('netLog module', () => {
     })
   })
 
-  // FIXME(deepak1556): Ch69 follow up.
   it('should begin and end logging automtically when --log-net-log is passed, and behave correctly when .startLogging() and .stopLogging() is called', done => {
     if (isCI && process.platform === 'linux') {
       done()
       return
     }
 
-    const appProcess = ChildProcess.spawn(remote.process.execPath,
+    const appProcess = ChildProcess.spawn(process.execPath,
       [appPath], {
         env: {
           TEST_REQUEST_URL: server.url,
@@ -140,7 +134,7 @@ describe('netLog module', () => {
       return
     }
 
-    const appProcess = ChildProcess.spawn(remote.process.execPath,
+    const appProcess = ChildProcess.spawn(process.execPath,
       [appPath], {
         env: {
           TEST_REQUEST_URL: server.url,

spec-main/fixtures/api/net-log/main.js

@@ -0,0 +1,31 @@
+const { app, net, session } = require('electron')
+
+if (process.env.TEST_DUMP_FILE) {
+  app.commandLine.appendSwitch('log-net-log', process.env.TEST_DUMP_FILE)
+}
+
+function request () {
+  return new Promise((resolve) => {
+    const req = net.request(process.env.TEST_REQUEST_URL)
+    req.on('response', () => {
+      resolve()
+    })
+    req.end()
+  })
+}
+
+app.on('ready', async () => {
+  const netLog = session.defaultSession.netLog
+
+  if (process.env.TEST_DUMP_FILE_DYNAMIC) {
+    await netLog.startLogging(process.env.TEST_DUMP_FILE_DYNAMIC)
+  }
+
+  await request()
+
+  if (process.env.TEST_MANUAL_STOP) {
+    await netLog.stopLogging()
+  }
+
+  app.quit()
+})

spec/fixtures/api/net-log/main.js

@@ -1,38 +0,0 @@
-const { app, net, session } = require('electron')
-
-if (process.env.TEST_DUMP_FILE) {
-  app.commandLine.appendSwitch('log-net-log', process.env.TEST_DUMP_FILE)
-}
-
-function request () {
-  return new Promise((resolve) => {
-    const req = net.request(process.env.TEST_REQUEST_URL)
-    req.on('response', () => {
-      resolve()
-    })
-    req.end()
-  })
-}
-
-app.on('ready', async () => {
-  const netLog = session.defaultSession.netLog
-
-  // The net log exporter becomes ready only after
-  // default path is setup, which is posted as task
-  // to a sequenced task runner due to sync IO operations,
-  // the task are blocked for some reason,
-  // revisit task scheduling after 69 upgrade and fix this workaround.
-  setImmediate(async () => {
-    if (process.env.TEST_DUMP_FILE_DYNAMIC) {
-      netLog.startLogging(process.env.TEST_DUMP_FILE_DYNAMIC)
-    }
-
-    await request()
-
-    if (process.env.TEST_MANUAL_STOP) {
-      await netLog.stopLogging()
-    }
-
-    app.quit()
-  })
-})

typings/internal-electron.d.ts

@@ -71,6 +71,7 @@ declare namespace Electron {
 declare namespace ElectronInternal {
   type DeprecationHandler = (message: string) => void;
   interface DeprecationUtil {
+    warnOnce(oldName: string, newName?: string): () => void;
     setHandler(handler: DeprecationHandler): void;
     getHandler(): DeprecationHandler | null;
     warn(oldName: string, newName: string): void;