#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- launc...@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
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs