Bug 40938 (part 4): Move from TorProvider=TorMonitorService+TorProtocolService to something that we could really call TorProvider

Merge Info

Related Issues

• #40938 (closed)
• #41986 (closed)
• #41958 (closed)

Backporting

Timeline

• Immediate: patchset needed as soon as possible
• Next Minor Stable Release: patchset that needs to be verified in nightly before backport
• Eventually: patchset that needs to be verified in alpha before backport
• No Backport (preferred): patchset for the next major stable

(Optional) Justification

• Emergency security update: patchset fixes CVEs, 0-days, etc
• Censorship event: patchset enables censorship circumvention
• Critical bug-fix: patchset fixes a bug in core-functionality
• Consistency: patchset which would make development easier if it were in both the alpha and release branches; developer tools, build system changes, etc
• Sponsor required: patchset required for sponsor
• Other: please explain

Merging

• Merge to tor-browser - !fixups to tor-browser-specific commits, new features, security backports
• Merge to base-browser - !fixups to base-browser-specific commits, new features to be shared with mullvad-browser, and security backports
  • NOTE: if your changeset includes patches to both base-browser and tor-browser please clearly label in the change description which commits should be cherry-picked to base-browser after merging

Issue Tracking

• Link resolved issues with appropriate Release Prep issue for changelog generation

Review

Request Reviewer

• Request review from an applications developer depending on modified system:
  • NOTE: if the MR modifies multiple areas, please /cc all the relevant reviewers (since gitlab only allows 1 reviewer)
  • accessibility : henry
  • android : dan
  • build system : boklm
  • extensions : ma1
  • firefox internals (XUL/JS/XPCOM) : ma1
  • fonts : pierov
  • frontend (implementation) : henry
  • frontend (review) : donuts, richard
  • localization : henry, pierov
  • nightly builds : boklm
  • rebases/release-prep : dan_b, ma1, pierov, richard
  • security : ma1
  • signing : boklm, richard
  • updater : pierov
  • misc/other : pierov, richard

Change Description

The final time for the source of discussions MR has come!!

In previous episodes, we stopped using the control port around (!699 (merged)), we did small refactors to the code (!708 (merged)), we reimplemeted the control port (!709 (merged)), we copied TorMonitorService into TorProtocolService and started calling it TorProvider (!716 (merged)) and we started using the new code (!717 (merged)).

Now the time to make the TorMonitorService+TorProtocolService into a more organic TorProvider has come!!

Finally we use just a single control port connection for everything! 🎉 .

No more closing the control port because we received an output we didn't like!

No more strange code to handle reattempts (except for the first connection).

However, this isn't exactly what richard asked me to do. Still, I think this is a step we need to have, before reaching the final form they would prefer. I think this gives us a better starting point for a more definitive form.

So, I'm opening this MR to ask for comments on what I've done so far. If it's quite okay we could fix the small problem, merge and then follow in another issue. Otherwise, we can fix what it needs to be fixed already. If I understand well, the final form could be similar to this, I'll just need to move some methods out of TorProvider in TorProviderBuilder, or in TorLauncherUtil.

Bonus: while reviewing everything once again, I realized that our control port password management had some problems. I created #41986 (closed) for it. Even though it's a new issue, I think it does make sense to deal with it while we're solving all the other problems.

I tried to prepare the MR so that it could be reviewed commit by commit:

1. unified TorController with ControlSocket, and removed CallbackDispatcher: these changes should be pretty easy to review on their own
2. switched from _ to # for private members (and renamed some as well): this kind of change is easy to review on its own, because you can check that basically only _ and # is the only change in every line (GitLab helps to verify this)
3. mostly positioned code where I thought it was more appropriate. This commit should be reviewed offline, with git show --color-moved, to verify that it doesn't change much
4. a big chunk of refcators. Attention needed!
5. solved a race condition, that prevented us from correctly starting the browser
6. another big chunk of changes. Attention needed!
7. another race condition 😒 . And also ESMification, since I was touching that code.
8. shuffle the remaining parsing code from TorProvider to TorControlPort. Here we will have a lot to discuss about the architecture I chose (an object that implements some callbacks - I was really unsure this was the best approach)
9. the password stuff, quite isolated from the rest