Opened 4 years ago

Closed 4 years ago

#15510 closed defect (fixed)

The controller for the circuit-display is not stopped on New Identity

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-circuit-display, TorBrowserTeam201503R
Cc: arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

If one hits New Identity n times during then one gets essentially n controller instances talking to tor as they are not stopped on windows unload.

Child Tickets

Change History (5)

comment:1 Changed 4 years ago by gk

Keywords: TorBrowserTeam201503R added
Status: newneeds_review

bug_15510 in my public Torbutton repo (https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_15510) has a fix.

comment:2 Changed 4 years ago by arthuredelstein

That fix isn't in the right place -- bindPrefAndInitis supposed to be a generic utililty function that binds to any pref we choose and calls prefHandler(currentPrefValue) whenever the pref changes. It is not specific to the controller and it's not designed only for boolean prefs.

My intention was that the controller code is set up to create only one instance of the controller, the first time controller(...) is called, and just return an extra pointer to the controller with each subsequent call. Please see the comment and code at https://gitweb.torproject.org/torbutton.git/tree/src/modules/tor-control-port.js#n648
But it's possible I got this code wrong or the code in tor-circuit-display wrong. What was the original symptom you saw?

comment:3 in reply to:  2 Changed 4 years ago by gk

Replying to arthuredelstein:

That fix isn't in the right place -- bindPrefAndInitis supposed to be a generic utililty function that binds to any pref we choose and calls prefHandler(currentPrefValue) whenever the pref changes. It is not specific to the controller and it's not designed only for boolean prefs.

Ugh, yes you are ight. I should have paid more attention while writing the patch.

My intention was that the controller code is set up to create only one instance of the controller, the first time controller(...) is called, and just return an extra pointer to the controller with each subsequent call. Please see the comment and code at https://gitweb.torproject.org/torbutton.git/tree/src/modules/tor-control-port.js#n648
But it's possible I got this code wrong or the code in tor-circuit-display wrong. What was the original symptom you saw?

If you start Tor Browser and load foo.com you see things like

Torbutton INFO: streamEvent.CircuitID: 2

and other control port related things in the log, If you press New Identity, say, 5 times and load foo.com you see

Torbutton INFO: streamEvent.CircuitID: 2
Torbutton INFO: streamEvent.CircuitID: 2
Torbutton INFO: streamEvent.CircuitID: 2
Torbutton INFO: streamEvent.CircuitID: 2
Torbutton INFO: streamEvent.CircuitID: 2 

in the log and so on. In other words stop() is not called properly and shutting the controller explicitly down is needed for some reason.

Last edited 4 years ago by gk (previous) (diff)

comment:4 Changed 4 years ago by arthuredelstein

I've added a call to stop() in the tor-circuit-display, and also added code that detaches "event watchers" inside the stop() function. This seems to fix the problem:

https://github.com/arthuredelstein/torbutton/commit/f0e1d504dbf5

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:5 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Yeah, that's much better now, thanks. Commits f0e1d504dbf57f0c0041ae14f32a5a092a8a1afe and fc0c8ed76346f2bb0b4d139ca444cbc404df5d07 (the latter is just a fix for a small typo) have the changes.

Note: See TracTickets for help on using tickets.