[tor-bugs] #25750 [Applications/Tor Launcher]: update Tor Launcher for ESR 60

Wed May 9 21:42:08 UTC 2018

#25750: update Tor Launcher for ESR 60
--------------------------------------------+------------------------------
 Reporter:  mcs                             |          Owner:  brade
     Type:  defect                          |         Status:
                                            |  needs_revision
 Priority:  Very High                       |      Milestone:
Component:  Applications/Tor Launcher       |        Version:
 Severity:  Normal                          |     Resolution:
 Keywords:  ff60-esr, TorBrowserTeam201805  |  Actual Points:
Parent ID:                                  |         Points:
 Reviewer:                                  |        Sponsor:
--------------------------------------------+------------------------------
Changes (by mcs):

 * status:  needs_review => needs_revision

Comment:

 Replying to [comment:17 sysrqb]:
 > > 1) I am surprised that XPCOM extensions still work as well as they do.
 We should set `extensions.legacy.enabled = true` and add `tor-
 launcher at torproject.org` to `extensions.legacy.exceptions` so the
 about:addons UI does not label Tor Launcher as LEGACY (and also add
 Torbutton's extension ID if we keep them both as regular add-ons).
 >
 > Yes, I was surprised, too. I think FF61 brings a lot more breakage - but
 I think these should be set in/by tor-browser instead of tor-launcher.

 Agreed.

 Here are a few general comments I should have made earlier (sorry!):
 * Please wrap lines before 80 columns when possible.
 * Please follow the brace style used in the files (different than what
 Mozilla uses, but mixing at this point could cause confusion).
 * Please update the copyright year near the top of the files that you
 modify.

 > > 5) For 374b64f3057ab4b703e3e79473cd4cfc8abc295e:
 > > * How can we guarantee that the call to `_loadPreferences()` occurs
 before any code which might rely on the preference values? Kathy and I
 don't have an answer, but maybe we should add an explicit initialization
 call inside `components/tl-process.js` (either in the `"profile-after-
 change"` case within the `observe()` implementation or at the end of `tl-
 process.js`.
 >
 > This was a little tricky because torlauncher uses preferences very early
 in its initialization (such as initializing the TorLauncherLogger module).
 As a result, I moved loading the prefs into the TorProcessService
 constructor. In addition, now an exception is thrown if certain methods
 are called before we load the default prefs (get{Bool,Int,Char}Pref(),
 TorStartAndControlTor(), etc).
 >
 > I think we want Tor Browser to fail closed in these situations. Further,
 I think we want Tor Browser failing closed if any of the TorLauncher
 default prefs are invalid or aren't set for any reason.

 You are right — this is tricky.  Thanks for your work on it. Kathy and I
 think what you did is good, but we would rather not throw exceptions from
 the get...Pref() functions. Callers all pass a default default anyway, and
 may not handle exceptions well. Plus, in our testing, the exception you
 added to the `TorProcessService` constructor effectively disables Tor
 Launcher. A bonus is that we should be able to use the logging module
 inside `loadDefaultPreferences()` and related code.

 > > * You can use `this.mPrefsSvc` inside `_getPrefDefaultBranch()`; if
 you do that, it might not be worthwhile to have a
 `_getPrefDefaultBranch()` function.
 >
 > I don't see how we can access the DefaultBranch from `this.mPrefsSvc`
 because mPrefsSvc is a `Ci.nsIPrefBranch` but we need `Ci.nsIPrefService`
 for retrieving the default branch. If there's a way to get the default
 branch from nsIPrefBranch, then I'll do that.

 Ah, you are correct. Kathy and I think you should just keep things the way
 you have them.

 > > 6) For d1efcd33ca6389479337f70a603c73c8264888b8:
 > > * Mozilla now uses Services.intl.DateTimeFormat(). We should too. See:
 https://hg.mozilla.org/releases/mozilla-beta/rev/650bd331efd0
 > > * I don't think we should add a blank line after this one:
 > >   let fracSecsStr = ...
 > > (that declaration is tied to the `for` loop that follows immediately
 after).

 While testing that code inside a browser built with Arthur's `25543+10`
 branch from #25543, we found that we had to make two changes:
 * Add `Cu.import("resource://gre/modules/Services.jsm");`
 * Change to `const dateTimeFormatter = new
 Services.intl.DateTimeFormat(undefined, {`
 (the second change is necessary because of https://hg.mozilla.org/releases
 /mozilla-beta/diff/94788650b26b/browser/base/content/pageinfo/pageInfo.js)

 BUT: the date format generated by the new code is a lot different than
 what we had before. Can you see if we can maintain the same format?
 Old: `5/9/18, 21:06:51.000 [NOTICE] Bootstrapped 100%: Done`
 New: `May 9, 2018 at 9:06:17 PM UTC.323 [NOTICE] Bootstrapped 100%: Done`

 Finally, please let us know if you want Kathy and me to work on any of the
 loose ends. I think we are very close!

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