Re: [tor-bugs] #10760 [Applications/Tor Browser]: Integrate TorButton to TorBrowser core to prevent users from disabling it

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#10760: Integrate TorButton to TorBrowser core to prevent users from disabling it
-------------------------------------------------+-------------------------
 Reporter:  Rezonansowy                          |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  High                                 |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  AffectsTails, tbb-parity, ux-team,   |  Actual Points:
  TorBrowserTeam201906, GeorgKoppen201906        |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by acat):

 Thanks for the review :)

 I addressed your comments (and some other small things I found while
 looking at it) in https://github.com/acatarineu/torbutton/commits/10760+1.
 I also squashed the previous 9cf79bcf222cfb1ca47f2cd6f119ac1f19b2f54d,
 ce346d069afa5f1a42dfa13eb8fce2a23cfbf679 and
 4dd346e2b81527785d7f8b68a5cff14972dd645c (overlays removal) into the new
 0140cfca33be68fedd895672ab7e88f55b023803. The other commits are in the
 same place, with some changes addressing the comments.

 Some of the inconsistencies (but not all) were because initially I had
 more style fixes (all ESLint ones), but I decided to undo most of them
 (and keep the important ones) to reduce the number of changes. I think in
 the process I undid too much (e.g. some of the short-form Services.obs,
 Services.strings, Services.cookies...).


 >1) What is the reason for commenting out code in the patch? If we don't
 need it then we should remove >it. Additionally, the torbutton-new-
 identity part is even disabled only sometimes but it's not obvious >why
 not every time.

 I'm sorry, I forgot to remove these commented lines. Even though there is
 one that should not have been commented (torbutton_toggle_plugins). For
 torbutton-new-identity, I removed all of them, since the UI element is not
 there anymore.

 >2) In torbutton_utils.js there is Services used, but // let { Services }

 Yes, currently the util functions in this file are used as globals from
 other files. For that, this is loaded via `Services.scriptloader` in
 browser.xul and via <script> tag in
 `torbutton/chrome/content/preferences.xhtml`. And with that loader there
 is a problem if you try to redefine the Services variable (since it's
 already defined in the global scope). I removed the commented Services
 line (hoping that for mobile, `torbutton/chrome/content/preferences.xhtml`
 already has Services in the globals). I also filed #30888 to deal with the
 torbutton global variables and functions that are "leaked" in global scope
 of the browser window (this also happens in current browser, see all
 m_tb_* variables in browser console for example). I think we should try to
 minimize these, if I'm not wrong I think we only need as globals the
 functions that need to be called from some menu button (e.g.
 torbutton_new_circuit, torbutton_new_identity...).


 >3) In aboutTor.js (and cookie-jar-selector.js + domain-isolator.js +
 dragDropFilter.js + external-app-blocker.js + torCheckService.js +
 torbutton-logger.js + partly tor-control-port.js + utils.js)
 >we have
 >const {XPCOMUtils}
 >const {Services}
 >while having spaces after and before the curly braces elsewhere in the
 same changes, please stick to >one style.

 Ok, I changed everything to the style with spaces.

 >5) + QueryInterface: ChromeUtils.generateQI([Ci.nsISupports,
 Ci.nsIClassInfo]),
 >Why suddenly here Ci.nsISupports but a lot of other generateQI() calls
 don't have it? There is more of >this in other components, like domain-
 isolator.js, dragDropFilter.js, external-app-blocker.js,
 >torCheckService.js, torbutton-logger.js

 I removed nsISupports from all GenerateQI, since they seem to be added
 anyway in MozQueryInterface.cpp ChromeUtils::GenerateQI.

 >7) Why do we need to resort to broadCastAsyncMessage and sendAsyncMessage
 would not be enough?

 I think I tried Services.cpmm.sendAsyncMessage and for some reason it was
 not working, although clearly I must have done something wrong as it is
 working now (as expected). So fixed that too. My understanding is that
 this will work as long as the webextension runs in content process (as I
 think it does now in all platforms).

 >8) There is an addTab call in menu-overlay.xul that needs to get adjusted

 Ok, after looking more, I saw `addWebTab` and `addTrustedTab` that call
 addTab with nullprincipal and systemprincipal respectively, and used this
 accordingly now.

 >8) It seems you missed to adapt startup-observer.js and doing all the
 clean-up there, too?

 Yes, not really sure why :(

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/10760#comment:60>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs