Opened 12 months ago

Last modified 45 hours ago

#28745 needs_revision defect

THE Torbutton clean-up

Reported by: gk Owned by: acat
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton, TorBrowserTeam202001
Cc: igt0, tbb-team Actual Points:
Parent ID: #30506 Points:
Reviewer: gk Sponsor:

Description

That is the parent ticket for all things Torbutton clean-up, now that we included it into tor-browser. It's not clear yet how we'll be restructuring it but it's clear that a lot of old cruft has to go. This will be done in child tickets.

Child Tickets

TicketStatusOwnerSummaryComponent
#28746closedtbb-teamRemove torbutton_update_isolation_prefs() and torbutton_update_fingerprinting_prefs()Applications/Tor Browser
#28747closedtbb-teamRemove old NoScript related code (needed for dealing with the XPCOM version)Applications/Tor Browser
#28894closedtbb-teamRemove Torbutton parts of fix for #20244Applications/Tor Browser
#30850newtbb-teamMove not UI-related torbutton.js code to XPCOM component/serviceApplications/Tor Browser
#30851closedtbb-teamMove torbutton default prefs to 000-tor-browser.jsApplications/Tor Browser
#30888closedtbb-teamMinimize torbutton globals exposed in browser windowApplications/Tor Browser

Change History (28)

comment:1 Changed 12 months ago by gk

Cc: igt0 added

comment:5 Changed 5 months ago by acat

Keywords: TorBrowserTeam201907R added
Status: newneeds_review

Some changes for review here: https://github.com/acatarineu/torbutton/commits/28745 (starting from 6023e0875c8085a58e6269801113a2a4600cb6d3, which is the last commit from #10760 patch). Corresponding tor-browser changes here: https://github.com/acatarineu/tor-browser/commits/28745 (last 4 commits).

Some comments:

Remove cookie-jar-selector component

  • For browser.privatebrowsing.autostart = true, cookie-jar-selector does nothing currently, so for new identity we just clearCookies().
  • For browser.privatebrowsing.autostart = false, some cookies json files are stored in the browser profile folder, which we clear on startup in the patch.

Bug 30851: Move default preferences to 000-tor-browser.js

  • This has a corresponding patch in tor-browser.

Bug 30888: move torbutton_util.js to modules/utils.js

  • This also has a corresponding patch in tor-browser, to stop including torbutton_util.js in browser.xul.

Make torbutton_open_network_settings global

  • This will have to be called somewhere in UI at some point, for now we just expose it.

Refactor code to always use tor-control-port.js

  • Trying to use a single implementation of tor controller. Perhaps next step would be to do the same with tor-launcher.
  • The code had to be adapted a bit, since tor-control-port.js is async while previous impl. was sync.

Remove versioncheck from torbutton.js

  • I'm assuming we don't need this, with the browser updater and having removed the blinking notification in torbutton UI. But perhaps we still want as telemetry, not sure.

comment:6 Changed 5 months ago by cypherpunks

Refactor code to always use tor-control-port.js

It's a rewriting, not only refactoring. Should we do it inside the general clean-up?

comment:7 Changed 4 months ago by gk

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201907R removed

No July any longer.

comment:8 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201908R removed

No August anymore.

comment:9 Changed 3 months ago by cypherpunks

comment:10 Changed 3 months ago by cypherpunks

comment:11 Changed 2 months ago by pili

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201909R removed

We're now in October, moving September outstanding reviews to October

comment:12 Changed 8 weeks ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed
Parent ID: #30506
Status: needs_reviewneeds_revision

We'd want to have this rebased I think (sorry for the delay).

comment:13 Changed 7 weeks ago by acat

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

Rebased in https://github.com/acatarineu/torbutton/commits/28745+1 and https://github.com/acatarineu/tor-browser/commits/28745+1.

I dropped some commits wrt the original torbutton changes (Refactor code to always use tor-control-port.js, Tor controller promises should fail when there is an error, Move torbutton_do_new_identity to separate module, Move some code from torbutton to torCheckService) as I was not sure whether they fit in this ticket or they added much value, and the changes were quite big already. Maybe we can continue on these on later torbutton refactors?

I think #13198 would be fixed with these changes, and perhaps #19256 too, although there might be more dead code that I missed.

comment:14 Changed 5 weeks ago by gk

Alright I started reviewing and applying patches. The first two landed on master (the latter closing #28746): commit 2dfa0e0c9cff7cfad93664e0b0b6cdc05b24b7f2 and 938997fa6de418e423186a5fe5c3e8adf4d82a38.

comment:15 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201910R removed

comment:16 Changed 4 weeks ago by gk

Could you remove the extensions.torbutton.prompt_torbrowser pref in the commit where you are removing all the m_tb_tbb parts? It seems that pref removal belongs there.

comment:17 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

Commit 46b550c5550d3ad4a7d526a402a33bbb40dce5e2 on your torbutton branch has

-      // Disable the External App Blocker on Android
-      if ((aPrefName === "extensions.torbutton.launch_warning") &&
-          (Services.appinfo.OS === "Android")) {
-        defaultPrefBranch.setBoolPref(aPrefName, false);
-      }

It seems we want to keep that part, though, and I don't see a matching counterpart in any of your related tor-browser commits.

Commit f797d8ba19c5838032abff6da3c4b5e8cabc518f on your tor-browser branch:

+pref("extensions.torbutton.cookie_protections",true);
+pref("extensions.torbutton.cookie_auto_protect",false);

That's already gone, I think.

+pref("extensions.torbutton.prompt_torbrowser", true);

No need to add this anymore, see my previous comment.

+// TODO: This is just part of a stopgap until #14429 gets properly implemented.
+// See #7255 for details. We display the warning three times to make sure the
+// user did not click on it by accident.

I think we should update the comment at least. We still have this mechanism for users that disable letterboxing, I think.

Have you tested that moving browser.startup.homepage is fine now and that we can ditch non-localized.properties in torbutton? The code mentioned in the comment is still there. But I guess we don't hit the codepath as we are not setting any default prefs in the extension anymore?

comment:18 Changed 4 weeks ago by acat

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: needs_revisionneeds_review

Revised branches: https://github.com/acatarineu/tor-browser/commits/28745+2 and https://github.com/acatarineu/torbutton/commits/28745+2.

I moved the extensions.torbutton.launch_warning = false mobile override to 000-tor-browser-android.js.

Have you tested that moving browser.startup.homepage is fine now and that we can ditch non-localized.properties in torbutton? The code mentioned in the comment is still there. But I guess we don't hit the codepath as we are not setting any default prefs in the extension anymore?

Yes, I tested that it works fine.

comment:19 Changed 3 weeks ago by gk

Thanks. I closed #30851 as the updated changes look good to me. And I applied the m_tb_tbb one to torbutton's master (comit 6b1a5ded2cab7e51aeb504483fa0d8fbf0cae957). Getting closer...

Last edited 3 weeks ago by gk (previous) (diff)

comment:20 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

commit a12088fdc0d343d81b6ecf53faf06c91f8da27d5 looks good. I've applied it tor torbutton's master (commit 65e1e8ba67ba865e7b3ded74b7e4bbc5b86c721c)

Some nits and a question for commit 7e23b3cbf66717c1b9b3a107daf3147b54061a96:

+/* global ,

I think you don't want to have a , + whitespace there?

Are you sure we want to do

-  let firstPartyDomain = getDomainForBrowser(gBrowser.selectedBrowser);
+  let firstPartyDomain = getDomainForBrowser(gBrowser);

? In utils.js it says

// Assuming this is called with gBrowser.selectedBrowser
var getDomainForBrowser = (browser) => {

Please remove the superfluous whitespaces in

+    // So we can use it in boolean expressions to determine where the 

and

+function torbutton_get_property_string(propertyname)
+{
+    try { 

(the try line)

commit 38720bfe8f88248534d4d211a40274c613a6ade6:

1) Do we really need the remaining k_tb_browser_update_needed_pref parts (including the pref) now that the pref observer is gone (and the code acting upon pref changes)? What about the respective button CSS parts? Can't we get rid of them as well in this commit? For instance, if we don't set tbStatus anymore it seems to me we don't need the [tb-status="on"] and [tb-status="off"] parts anymore either etc.

2) Please adapt the commit message to what has happened since you wrote it. :)

Please base your new branch on current master.

comment:21 Changed 3 weeks ago by gk

Cc: tbb-team added
Owner: changed from tbb-team to acat
Status: needs_revisionassigned

comment:22 Changed 3 weeks ago by acat

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: assignedneeds_review

Thanks for the review.

Revised branch: https://github.com/acatarineu/torbutton/commits/28745+3.

With respect to the k_tb_browser_update_needed_pref comment, I decided to squash Remove versioncheck from torbutton.js and Remove code dealing with torbutton UI button in toolbar, as some of the requested changes were already included in the other commit, and I think these are related (we can remove versioncheck because there is not torbutton UI anymore).

There were some conflicts with the last commit of 28745+2 (2d318efde8faccf3980c6d7da163c32103202b26) and the 46efc92348dbed06fc31ddfb0a5ac2e4e8554de2 commit in master (#30237). I think these are not straightforward to solve, as in that commit I moved m_tb_control_ipc_file, m_tb_control_host, m_tb_control_port, m_tb_control_pass, m_tb_control_desc to a service while in the master commit those were moved to a different module and initialized via configureControlPortModule. I would suggest dropping that commit for now, and perhaps do it later in #30850 (some of the previous dropped commits are also related to that one).

comment:23 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

Thanks. While I am preparing a patch for #30787 I realized that it seems we forgot to clean up the respective locale files, too, while removing the code. E.g. I am right now still seeing torbutton.cookieDialog.* entries. Could you go over the locale files and get rid of the strings we don't need anymore due to this clean-up?

Last edited 6 days ago by gk (previous) (diff)

comment:24 Changed 2 weeks ago by gk

I closed #30888 as the changes look good to me now.

comment:25 Changed 2 weeks ago by acat

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: needs_revisionneeds_review

comment:26 Changed 9 days ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201911R removed

We are in December now.

comment:27 Changed 9 days ago by pili

Reviewer: gk

gk to review this tickets

comment:28 Changed 6 days ago by gk

Okay, let's move on with the review process, commit 8269902a22357697c46c7052c9d946bd874c3f55:

Looks mostly good. It misses a tor-browser change to remove the extensions.torbutton.updateNeeded and extensions.torbutton.versioncheck_enabled pref, now that we moved the preferences. Additionally, please remove the torbutton-update-needed.svg as well. It seems to be obsolete now, too.

For 8c0c18a09a30f14dd0b4a99fe67238fec0ad3bac what command did you run/tool did you use to check for those errors (and fix them)?

comment:29 Changed 6 days ago by gk

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201912R removed
Status: needs_reviewneeds_revision

comment:30 in reply to:  22 Changed 6 days ago by gk

Replying to acat:

[snip]

There were some conflicts with the last commit of 28745+2 (2d318efde8faccf3980c6d7da163c32103202b26) and the 46efc92348dbed06fc31ddfb0a5ac2e4e8554de2 commit in master (#30237). I think these are not straightforward to solve, as in that commit I moved m_tb_control_ipc_file, m_tb_control_host, m_tb_control_port, m_tb_control_pass, m_tb_control_desc to a service while in the master commit those were moved to a different module and initialized via configureControlPortModule. I would suggest dropping that commit for now, and perhaps do it later in #30850 (some of the previous dropped commits are also related to that one).

Sounds good to me.

comment:31 Changed 45 hours ago by pili

Keywords: TorBrowserTeam202001 added; TorBrowserTeam201912 removed

acat is afk in December

Note: See TracTickets for help on using tickets.