Talk to the control socket using my old draft tor daemon manager. #14146

riastradh-brave · 2018-05-16T04:24:11Z

This doesn't solve #14043 yet, but it provides a candidate place to hook up a progress indicator. See the TODO(riastradh): Visually update a progress bar! comment for the place where we should update a progress bar, currently c2b3eda#diff-8a9bac13ef6264feb2b6da7c18d86b3cR751.

Depends on Enable tor daemon control port. muon#592 to launch tor with a control port.
Depends on Updated hashes #14145 to update the version of tor to one that has functional control ports, since apparently they were broken in the alpha version we had been using before.
Depends on Call mockery.deregisterAll when done after registering mocks. #14168, currently cherry-picked into the branch, to make the tests function. Should be rebased once tor/0.23.x is rebased to have that commit.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Launch Brave. Confirm that the console output mentions tor bootstrapping.

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

diracdeltas · 2018-05-16T09:59:07Z

app/filtering.js

@@ -47,6 +48,8 @@ const headersReceivedFilteringFns = []
 let partitionsToInitialize = ['default', appConfig.tor.partition]
 let initializedPartitions = {}

+let torDaemon = new tor.TorDaemon()


it looks like you don't reassign to this variable so better to use const instead of let

also this doesn't need to be in the module scope, right? so it can be declared in setupTor instead.

done in 453e327

diracdeltas · 2018-05-16T10:02:00Z

app/tor.js

+        loop(i + 1)
+      })
+    }
+    loop(0)


it would be slightly clearer if you used directories.forEach instead of this recursive loop

Callbacks make forEach a little tricky. If this should be done with mkdirSync, I guess that's OK, but I figured it would be better to avoid that. (I'm not sure this is necessary, but the ordering of directory creation in muon vs browser-laptop is not obvious, so I figured it would be safer to keep this logic than delete it.)

@riastradh-brave i see now that one of the directories is a subdir of the other. in that case you can just use fs-extra's mkdir instead of fs's mkdir, which will behave like mkdir -p: https://github.com/jprichardson/node-fs-extra

How does that help to do multiple ones in a loop? I guess I could replace the whole thing by a single mkdirp for the deepest subdirectory, but I didn't want to embed the assumption that there is a single deepest subdirectory in that function.

diracdeltas · 2018-05-16T10:04:12Z

app/tor.js

+module.exports.torControlParseQuoted = torControlParseQuoted
+module.exports.torrcEscapeBuffer = torrcEscapeBuffer
+module.exports.torrcEscapePath = torrcEscapePath
+module.exports.torrcEscapeString = torrcEscapeString


please add unit tests for these exported methods (located in test/unit, npm run unittest to run)

done in 4282807

thanks for adding tests - could you also document the input types of any externally-callable methods/constructors using jsdoc so it's harder to get the types wrong? http://usejsdoc.org/ - see js/lib/request for an example

done in 8f532a9

riastradh-brave · 2018-05-17T23:28:37Z

In 4b39b3a, I added some automatic tests for the logic to launch tor and wait for a notification that it has launched within a reasonable time, but they are broken because fs.mkdir doesn't work while running npm run unittest.

These tests may also be problematic because they deliberately put in a half-second delay to make sure we exercise both orders of (a) tor process startup, and (b) fs event watch notification logic setup.

What to do?

riastradh-brave · 2018-05-17T23:46:40Z

Never mind, apparently this works now and I have no idea why it failed before.

@diracdeltas

For #14043. Auditors: @diracdeltas Test Plan: 1. Launch Brave. 2. Examine console output. 3. Confirm it mentions tor bootstrapping.

TODO(riastradh): These are currently broken because something futzes with fs.mkdir when run as `npm run unittest'.

This directory will have been populated by tor stuff, which, by intent, nothing else cleans up.

These indicate when tor thinks it's definitely ready, or definitely not ready (any more).

- 'end' means end of input only - 'error' means an error happened but no state change - 'close' means underlying resource is now closed Make sure TorDaemon emits 'exit' event when control closes. Confirm in automatic tests that (a) the TorDaemon emits 'exit', _and_ (b) the tor process exits. This increases the total amount of code by about 70 lines, but the logic in each of the event handlers is smaller because their use more precisely reflects their fine-grained intent in the nodejs APIs, and many of the new lines are in comments on new short internal subroutines.

@diracdeltas