This is an automated email from the git hooks/post-receive script.

pierov pushed a commit to branch tor-browser-102.4.0esr-12.0-1 in repository tor-browser.

The following commit(s) were added to refs/heads/tor-browser-102.4.0esr-12.0-1 by this push: new ed7d92b0d031 fixup! Bug 40933: Add tor-launcher functionality ed7d92b0d031 is described below

commit ed7d92b0d03166825f51d64cc57f59c3594b996b Author: Pier Angelo Vendrame pierov@torproject.org AuthorDate: Fri Oct 21 19:35:58 2022 +0200

fixup! Bug 40933: Add tor-launcher functionality

Bug 41386: Fix a pair of errors when handling the case of tor already running --- .../components/tor-launcher/TorMonitorService.jsm | 107 +++++++++++++++------ toolkit/components/tor-launcher/TorProcess.jsm | 22 ++--- 2 files changed, 88 insertions(+), 41 deletions(-)

diff --git a/toolkit/components/tor-launcher/TorMonitorService.jsm b/toolkit/components/tor-launcher/TorMonitorService.jsm index 881f4a5a7355..27572c30370f 100644 --- a/toolkit/components/tor-launcher/TorMonitorService.jsm +++ b/toolkit/components/tor-launcher/TorMonitorService.jsm @@ -5,7 +5,9 @@ var EXPORTED_SYMBOLS = ["TorMonitorService"];

const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); -const { setTimeout } = ChromeUtils.import("resource://gre/modules/Timer.jsm"); +const { clearTimeout, setTimeout } = ChromeUtils.import( + "resource://gre/modules/Timer.jsm" +); const { XPCOMUtils } = ChromeUtils.import( "resource://gre/modules/XPCOMUtils.jsm" ); @@ -13,7 +15,7 @@ const { XPCOMUtils } = ChromeUtils.import( const { TorParsers, TorStatuses } = ChromeUtils.import( "resource://gre/modules/TorParsers.jsm" ); -const { TorProcess, TorProcessStatus } = ChromeUtils.import( +const { TorProcess } = ChromeUtils.import( "resource://gre/modules/TorProcess.jsm" );

@@ -69,6 +71,7 @@ const TorMonitorService = { _connection: null, _eventsToMonitor: Object.freeze(["STATUS_CLIENT", "NOTICE", "WARN", "ERR"]), _torLog: [], // Array of objects with date, type, and msg properties. + _startTimeout: null,

_isBootstrapDone: false, _bootstrapErrorOccurred: false, @@ -91,7 +94,7 @@ const TorMonitorService = { this._controlTor(); } else { logger.info( - "Not starting the event monitor, as e do not own the Tor daemon." + "Not starting the event monitor, as we do not own the Tor daemon." ); } logger.debug("TorMonitorService initialized"); @@ -104,6 +107,8 @@ const TorMonitorService = { uninit() { if (this._torProcess) { this._torProcess.forget(); + this._torProcess.onExit = null; + this._torProcess.onRestart = null; this._torProcess = null; } this._shutDownEventMonitor(); @@ -181,35 +186,42 @@ const TorMonitorService = { // Private methods

async _startProcess() { - this._torProcess = new TorProcess(); - this._torProcess.onExit = () => { - Services.obs.notifyObservers(null, TorTopics.ProcessExited); - }; - this._torProcess.onRestart = async () => { - this._shutDownEventMonitor(); - await this._controlTor(); - Services.obs.notifyObservers(null, TorTopics.ProcessRestarted); - }; + // TorProcess should be instanced once, then always reused and restarted + // only through the prompt it exposes when the controlled process dies. + if (!this._torProcess) { + this._torProcess = new TorProcess(); + this._torProcess.onExit = () => { + this._shutDownEventMonitor(); + Services.obs.notifyObservers(null, TorTopics.ProcessExited); + }; + this._torProcess.onRestart = async () => { + this._shutDownEventMonitor(); + await this._controlTor(); + Services.obs.notifyObservers(null, TorTopics.ProcessRestarted); + }; + } + + // Already running, but we did not start it + if (this._torProcess.isRunning) { + return false; + } + try { await this._torProcess.start(); - if (!this._torProcess.isRunning) { - this._torProcess = null; - return false; + if (this._torProcess.isRunning) { + logger.info("tor started"); } } catch (e) { // TorProcess already logs the error. this._bootstrapErrorOccurred = true; this._lastWarningPhase = "startup"; this._lastWarningReason = e.toString(); - this._torProcess = null; - return false; } - logger.info("tor started"); - return true; + return this._torProcess.isRunning; },

async _controlTor() { - if (!this._torProcess && !(await this._startProcess())) { + if (!this._torProcess?.isRunning && !(await this._startProcess())) { logger.error("Tor not running, not starting to monitor it."); return; } @@ -217,7 +229,6 @@ const TorMonitorService = { let delayMS = ControlConnTimings.initialDelayMS; const callback = async () => { if (await this._startEventMonitor()) { - this._status = TorProcessStatus.Running; this.retrieveBootstrapStatus().catch(e => { logger.warn("Could not get the initial bootstrap status", e); }); @@ -225,6 +236,14 @@ const TorMonitorService = { // FIXME: TorProcess is misleading here. We should use a topic related // to having a control port connection, instead. Services.obs.notifyObservers(null, TorTopics.ProcessIsReady); + + // We reset this here hoping that _shutDownEventMonitor can interrupt + // the current monitor, either by calling clearTimeout and preventing it + // from starting, or by closing the control port connection. + if (this._startTimeout === null) { + logger.warn("Someone else reset _startTimeout!"); + } + this._startTimeout = null; } else if ( Date.now() - this._torProcessStartTime > ControlConnTimings.timeoutMS @@ -234,18 +253,28 @@ const TorMonitorService = { this._lastWarningPhase = "startup"; this._lastWarningReason = s; logger.info(s); + if (this._startTimeout === null) { + logger.warn("Someone else reset _startTimeout!"); + } + this._startTimeout = null; } else { delayMS *= 2; if (delayMS > ControlConnTimings.maxRetryMS) { delayMS = ControlConnTimings.maxRetryMS; } - setTimeout(() => { + this._startTimeout = setTimeout(() => { logger.debug(`Control port not ready, waiting ${delayMS / 1000}s.`); callback(); }, delayMS); } }; - setTimeout(callback, delayMS); + // Check again, in the unfortunate case in which the execution was alrady + // queued, but was waiting network code. + if (this._startTimeout === null) { + this._startTimeout = setTimeout(callback, delayMS); + } else { + logger.error("Possible race? Refusing to start the timeout again"); + } },

async _startEventMonitor() { @@ -259,6 +288,16 @@ const TorMonitorService = { conn = await controller(avoidCache); } catch (e) { logger.error("Cannot open a control port connection", e); + if (conn) { + try { + conn.close(); + } catch (e) { + logger.error( + "Also, the connection is not null but cannot be closed", + e + ); + } + } return false; }

@@ -273,12 +312,19 @@ const TorMonitorService = { return false; }

+ // FIXME: At the moment it is not possible to start the event monitor + // when we do start the tor process. So, does it make sense to keep this + // control? if (this._torProcess) { this._torProcess.connectionWorked(); }

if (!TorLauncherUtil.shouldOnlyConfigureTor) { - this._takeTorOwnership(conn); + try { + await this._takeTorOwnership(conn); + } catch (e) { + logger.warn("Could not take ownership of the Tor daemon", e); + } }

this._connection = conn; @@ -449,12 +495,13 @@ const TorMonitorService = { },

_shutDownEventMonitor() { - if (this._connection) { - this._connection.close(); - this._connection = null; - this._eventMonitorInProgressReply = null; - this._isBootstrapDone = false; - this.clearBootstrapError(); + this._connection?.close(); + this._connection = null; + if (this._startTimeout !== null) { + clearTimeout(this._startTimeout); + this._startTimeout = null; } + this._isBootstrapDone = false; + this.clearBootstrapError(); }, }; diff --git a/toolkit/components/tor-launcher/TorProcess.jsm b/toolkit/components/tor-launcher/TorProcess.jsm index 3dd194817d0a..a8ad7b73c95e 100644 --- a/toolkit/components/tor-launcher/TorProcess.jsm +++ b/toolkit/components/tor-launcher/TorProcess.jsm @@ -1,6 +1,6 @@ "use strict";

-var EXPORTED_SYMBOLS = ["TorProcess", "TorProcessStatus"]; +var EXPORTED_SYMBOLS = ["TorProcess"];

const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); const { setTimeout } = ChromeUtils.import("resource://gre/modules/Timer.jsm"); @@ -49,7 +49,7 @@ class TorProcess { _exeFile = null; _dataDir = null; _args = []; - _torProcess = null; // nsIProcess + _subprocess = null; _status = TorProcessStatus.Unknown; _torProcessStartTime = null; // JS Date.now() _didConnectToTorControlPort = false; // Have we ever made a connection? @@ -69,7 +69,7 @@ class TorProcess { }

async start() { - if (this._torProcess) { + if (this._subprocess) { return; }

@@ -135,13 +135,13 @@ class TorProcess { environmentAppend: true, stderr: "pipe", }; - this._torProcess = await Subprocess.call(options); + this._subprocess = await Subprocess.call(options); this._watchProcess(); this._status = TorProcessStatus.Running; this._torProcessStartTime = Date.now(); } catch (e) { this._status = TorProcessStatus.Exited; - this._torProcess = null; + this._subprocess = null; logger.error("startTor error:", e); throw e; } @@ -162,7 +162,7 @@ class TorProcess { // Still, before closing the owning connection, this class should forget about // the process, so that future notifications will be ignored. forget() { - this._torProcess = null; + this._subprocess = null; this._status = TorProcessStatus.Exited; }

@@ -174,14 +174,14 @@ class TorProcess { }

async _watchProcess() { - const watched = this._torProcess; + const watched = this._subprocess; if (!watched) { return; } try { const { exitCode } = await watched.wait();

- if (watched !== this._torProcess) { + if (watched !== this._subprocess) { logger.debug(`A Tor process exited with code ${exitCode}.`); } else if (exitCode) { logger.warn(`The watched Tor process exited with code ${exitCode}.`); @@ -192,13 +192,13 @@ class TorProcess { logger.error("Failed to watch the tor process", e); }

- if (watched === this._torProcess) { + if (watched === this._subprocess) { this._processExitedUnexpectedly(); } }

_processExitedUnexpectedly() { - this._torProcess = null; + this._subprocess = null; this._status = TorProcessStatus.Exited;

// TODO: Move this logic somewhere else? @@ -238,7 +238,7 @@ class TorProcess { } }); } else if (this.onExit) { - this.onExit(unexpected); + this.onExit(); } }