Remove/synchronize Torbutton SOCKS pref logic
In 4.5 (#14630 (moved)), we removed the Torbutton settings window because I assumed that users could use the Firefox proxy setting instead. Unfortunately, Torbutton still has code to check if its SOCKS settings match the browser proxy settings, and to disable itself if these are out of sync.
We should remove this old code so that it is easier for people to point TBB at a different Tor instance. The code is a toggle-era relic, and we have a control port check anyway.
We should also ensure that if the Tor check fails, Torbutton's features aren't actually disabled, and that the difference is only a cosmetic warning. Right now, I fear that features are actually disabled, due to other toggle-era checks.
Activity
Slightly related: #14864 (moved)
I filed a related ticket: #19929 (moved)
-
Sometimes when you pull on a thread, an entire sweater begins to unravel. Kathy and I removed the code that dealt with the long gone proxy preferences, and that led us to to clean up more Torbutton code. The following branch contains 4 commits that are ready for review: https://gitweb.torproject.org/user/brade/torbutton.git/log/?h=bug15852-01
These commits can be smooshed together before merging into master, but it seemed better to keep them separate for review purposes.
Please let us know if we removed something that should be kept. In our judgment, all of the code that we removed was either not being used or it could have confusing (and possible harmful) effects.
Trac:
Keywords: TorBrowserTeam201608 deleted, TorBrowserTeam201608R added
Status: assigned to needs_review 
commit 573bb7fb3b22cfc28d3dce01e2378d1b181e6d43 looks good.
re commit 6244aadf7cfd944e8b53aabd1da52fe40df89f96:
- It seems we don't need to add/remove an observer for
network.proxyanymore in torbutton,js? - Maybe add
3. Reset Security Slider settings" intorbutton_prefs_reset_defaults()`? - I have the feeling that
// XXX: Hack for TBB people who alternate between transproxy and nonin
startup-observer.jscan go as well or did I miss something?commit ab40a9b20bab2d2608ad528830338c24fcf98e32 looks good. commit aa5cceae245efca766d5287d6754fac9c589b89e looks good.
So, just the nits above + squashing the commits into a single one and we are good here, thanks!
Trac:
Status: needs_review to needs_revision- It seems we don't need to add/remove an observer for
-
Replying to gk:
commit 573bb7fb3b22cfc28d3dce01e2378d1b181e6d43 looks good.
re commit 6244aadf7cfd944e8b53aabd1da52fe40df89f96:
- It seems we don't need to add/remove an observer for
network.proxyanymore in torbutton,js? You are right. We will remove that observer.
- Maybe add
3. Reset Security Slider settings" intorbutton_prefs_reset_defaults()`? OK.
- I have the feeling that
{{{
// XXX: Hack for TBB people who alternate between transproxy and non
}}}
in
startup-observer.jscan go as well or did I miss something? Do you mean just remove the comment or do you want the code to be removed? Kathy and I thought we should keep the code so that existing behavior is not modified for users who rely on setting the env variables.
Thanks for reviewing all of this!
- It seems we don't need to add/remove an observer for
-
Replying to mcs:
Replying to gk:
- I have the feeling that
{{{
// XXX: Hack for TBB people who alternate between transproxy and non
}}}
in
startup-observer.jscan go as well or did I miss something? Do you mean just remove the comment or do you want the code to be removed? Kathy and I thought we should keep the code so that existing behavior is not modified for users who rely on setting the env variables.
Just the comment. It seemed to me, if at all, then it only applied to the lines you removed.
Thanks for reviewing all of this!
Sure. :)
- I have the feeling that
{{{
// XXX: Hack for TBB people who alternate between transproxy and non
}}}
in
-
Here is a squashed commit that includes the changes gk requested in comment:12: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug15852-02&id=1b41636c9351b46bbcb2094a65c65a4407b60a37
Trac:
Status: needs_revision to needs_review -
While testing some other things, Kathy and I noticed a change in behavior compared to older versions of Torbutton: prior to the commit that fixed this ticket, the network.proxy.socks and network.proxy.socks_port prefs were not reset in the absence of env vars. We should probably restore the old behavior.
Here is a fixup patch: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug15852-03&id=1648da8461dfed9975ac83fdb03aa745b6c8d49b
Trac:
Status: reopened to needs_review -
Replying to legind:
Is there any reason why the config options
{{{ extensions.torbutton.socks_host extensions.torbutton.socks_port }}}
were not removed with this update? Are they still being used anywhere?
Looking at Torbutton master it seems to me they are gone as well? Grepping in different ways does not show instances of them anymore. Where do you still see them?
-
Some outdated and unneeded Torbutton preferences are still included with default values via the extension-overrides.js file that is part of the Tor Browser profile. See: https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/Bundle-Data/linux/Data/Browser/profile.default/preferences/extension-overrides.js
Kathy and I have prepared a patch to remove all of them, which we will post to #20111 (moved) very soon.
-
Replying to mcs:
Kathy and I have prepared a patch to remove all of them, which we will post to #20111 (moved) very soon.
On second thought, the patch belongs in this ticket. Here it is: https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug15852-01&id=5a7d9c36cd6178fb87f3ea5598b400392120f589
Georg, please let me know if you would rather have a new ticket for this followup fix.
Trac:
Resolution: fixed to N/A
Status: closed to reopened mentioned in issue #16220 (moved)
mentioned in issue #18093 (moved)
mentioned in issue #19929 (moved)
mentioned in issue #19996 (moved)
mentioned in issue tpo/applications/https-everywhere#19996 (closed)
mentioned in issue tpo/applications/torbutton#19929 (closed)
