Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance Tor process cleanup with __OwningControllerProcess and TAKEOWNERSHIP #14044

Closed
darkdh opened this issue Feb 8, 2021 · 3 comments · Fixed by brave/brave-core#7927
Closed

Comments

@darkdh
Copy link
Member

darkdh commented Feb 8, 2021

We currently rely on cleaning orphaned Tor process by pidFile when launching the new Tor process

And we can prevent orphaned Tor process by launching Tor with option __OwningControllerProcess plus utility process pid and when control connection established send TAKEOWNERSHIP so no matter how we crash Brave browser process or kill utility process, there shouldn't be any orphaned Tor process left
https://github.com/torproject/torspec/blob/d3a4dadac149c2be162344a6c2286653b8ac3753/control-spec.txt#L1525

@darkdh darkdh self-assigned this Feb 24, 2021
@darkdh darkdh added this to the 1.23.x - Nightly milestone Mar 3, 2021
@stephendonner stephendonner added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Mar 9, 2021
@stephendonner
Copy link

stephendonner commented Mar 9, 2021

Verified FIXED using nightly build

Brave 1.23.23 Chromium: 89.0.4389.72 (Official Build) nightly (x86_64)
Revision 3f345f156bfd157bd1bea06310e55f3fb2490359-refs/branch-heads/4389@{#1393}
OS macOS Version 11.2.3 (Build 20D91)

Verified both test cases from the plan in brave/brave-core#7927:

Case 1: Browser process crashed

  1. Open Tor window
  2. Make sure tor-* process is running, the process name is platform dependent.
  3. In normal window, type chrome://inducebrowsercrashforrealz/ in URL bar
  4. Browser should crash and tor-* process should no longer exist
screencast

Case 2: Kill Tor launcher utility process

  1. Open Tor window
  2. Make sure tor-* process is running, the process name is platform dependent and remember its pid
  3. Open Task Manager in Brave
  4. Kill Tor Launcher
  5. A new tor-* process should be spawned which should have different pid than step 2
  6. Enter https://brave5t5rjjg3s6k.onion/ in URL bar
  7. The onion site should be loaded normally
  8. Close Tor window and tor-* process should no longer exist
screencast

Verification passed on

Brave | 1.23.56 Chromium: 89.0.4389.105 (Official Build) dev (64-bit)
-- | --
Revision | 14f44e21a9d539cd49c72468a29bfca4fa43f710-refs/branch-heads/4389_90@{#7}
OS | Windows 10 OS Version 2004 (Build 19041.867)

Case 1: Browser process crashed

  • Opened a TOR window and ensured TOR launcher process is running

image
image

  • In normal window, type chrome://inducebrowsercrashforrealz/ in URL bar and ensured brave crashes, relaunched browser and ensured TOR process isn't running
    image

Case 2: Kill Tor launcher utility process

  • Opened a TOR window and noted TOR process PID 25552
    image

  • End the TOR process via Task manager and ensured new PID 1432 is listed for TOR process
    image

  • Opened https://brave5t5rjjg3s6k.onion/ and ensured onion site loads successfully
    image

  • Closed TOR window and ensured TOR process isn't running


Verified passed with

Brave	1.23.63 Chromium: 89.0.4389.114 (Official Build) beta (64-bit)
Revision	1ea76e193b4fadb723bfea2a19a66c93a1bc0ca6-refs/branch-heads/4389@{#1616}
OS	Linux

Verified test plan from brave/brave-core#7927

Scenario 1 - Browser process crashed
  1. Open Tor window
  2. Make sure tor-* process is running, the process name is platform dependent.
1a
  1. In normal window, type chrome://inducebrowsercrashforrealz/ in URL bar
  2. Browser should crash and tor-* process should no longer exist
1b
Scenario 2 - Kill Tor launcher utility process
  1. Open Tor window
  2. Make sure tor-* process is running, the process name is platform dependent and remember its pid
Screen Shot 2021-04-06 at 1 30 42 PM
  1. Open Task Manager in Brave
Screen Shot 2021-04-06 at 1 30 30 PM
  1. Kill Tor Launcher
  2. A new tor-* process should be spawned which should have different pid than step 2
Example Example
Screen Shot 2021-04-06 at 1 31 34 PM Screen Shot 2021-04-06 at 1 31 43 PM
  1. Enter https://brave5t5rjjg3s6k.onion/ in URL bar
  2. Confirmed the onion site should be loaded normally
  3. Closed Tor window and confirmed tor-* process does not show in Brave or System Task Managers

@stephendonner stephendonner added QA Pass-macOS and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Mar 9, 2021
@stephendonner
Copy link

Hi @darkdh - do you think this should be tested on all supported desktop platforms? If so, we can add the QA/Test-all-platforms tag; thanks!

@darkdh
Copy link
Member Author

darkdh commented Mar 18, 2021

yep, we should test it on all desktop platforms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment