diff --git a/atom/browser/api/atom_api_net_log.cc b/atom/browser/api/atom_api_net_log.cc index 88e3044c919..220612440fc 100644 --- a/atom/browser/api/atom_api_net_log.cc +++ b/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 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 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(); } + if (net_log_exporter_) { + args->ThrowError("There is already a net log running"); + return v8::Local(); + } + + pending_start_promise_ = base::make_optional(isolate()); + v8::Local 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 NetLog::StopLogging(mate::Arguments* args) { util::Promise promise(isolate()); v8::Local 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, - util::CopyablePromise(promise))); - } - stop_callback_queue_.clear(); - } -} - // static mate::Handle 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); } diff --git a/atom/browser/api/atom_api_net_log.h b/atom/browser/api/atom_api_net_log.h index 360a0b32495..1490c6b777e 100644 --- a/atom/browser/api/atom_api_net_log.h +++ b/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, - public net_log::NetExportFileWriter::StateObserver { +class NetLog : public mate::TrackableObject { public: static mate::Handle Create(v8::Isolate* isolate, AtomBrowserContext* browser_context); @@ -31,24 +31,33 @@ class NetLog : public mate::TrackableObject, static void BuildPrototype(v8::Isolate* isolate, v8::Local prototype); - void StartLogging(mate::Arguments* args); - std::string GetLoggingState() const; - bool IsCurrentlyLogging() const; - std::string GetCurrentlyLoggingPath() const; + v8::Local StartLogging(mate::Arguments* args); v8::Local 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 stop_callback_queue_; - std::unique_ptr net_log_state_; + + network::mojom::NetLogExporterPtr net_log_exporter_; + + base::Optional pending_start_promise_; + + scoped_refptr file_task_runner_; + + base::WeakPtrFactory weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN(NetLog); }; diff --git a/docs/api/net-log.md b/docs/api/net-log.md index 75d585ee6d1..6800639011a 100644 --- a/docs/api/net-log.md +++ b/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` - 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. diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index fcc26df6b96..351cdb43658 100644 --- a/lib/browser/api/session.js +++ b/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 + } + } +}) diff --git a/lib/common/api/deprecate.ts b/lib/common/api/deprecate.ts index d51cbcc3820..c9f5d9a7b00 100644 --- a/lib/common/api/deprecate.ts +++ b/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) => { diff --git a/spec/api-net-log-spec.js b/spec-main/api-net-log-spec.js similarity index 77% rename from spec/api-net-log-spec.js rename to spec-main/api-net-log-spec.js index 3a276fe73f8..4d51f84d52f 100644 --- a/spec/api-net-log-spec.js +++ b/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, diff --git a/spec-main/fixtures/api/net-log/main.js b/spec-main/fixtures/api/net-log/main.js new file mode 100644 index 00000000000..040aece614d --- /dev/null +++ b/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() +}) diff --git a/spec/fixtures/api/net-log/package.json b/spec-main/fixtures/api/net-log/package.json similarity index 100% rename from spec/fixtures/api/net-log/package.json rename to spec-main/fixtures/api/net-log/package.json diff --git a/spec/fixtures/api/net-log/main.js b/spec/fixtures/api/net-log/main.js deleted file mode 100644 index bfe38a31bf4..00000000000 --- a/spec/fixtures/api/net-log/main.js +++ /dev/null @@ -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() - }) -}) diff --git a/typings/internal-electron.d.ts b/typings/internal-electron.d.ts index e19353d6558..a2254a40600 100644 --- a/typings/internal-electron.d.ts +++ b/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;