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

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

#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:
--------------------------------------------+------------------------------

Comment (by sysrqb):

 Replying to [comment:18 mcs]:
 > Replying to [comment:17 sysrqb]:
 [snip]
 >
 > 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.

 All done, thanks for catching those.

 >
 [snip]
 > > 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.

 The problem I see with relying on the default value passed into
 get...Pref() is they aren't always the same as the default value in
 prefs.js. One obvious answer is "then we should make sure they stay
 synchronized", but that comes with other problems.

 And, indeed, TorLauncher throwing the exception (and nothing catching it)
 has exactly the result we want. By this, I mean it "disables Tor Launcher"
 and Tor Browser fails closed (assuming there aren't any proxy-bypass
 vulns, etc). We should have a better user experience in this situation,
 but I'm not sure how we should do that yet.

 I deleted the exception throw in the `get...Pref()` methods, as you
 suggested.

 > A bonus is that we should be able to use the logging module inside
 `loadDefaultPreferences()` and related code.
 >

 Yes, that would be a bonus, but that is difficult to do correctly without
 moving around more logic because there's a cyclic dependency here. Within
 `TLLoggerInternal._init()` we get the default prefs for `LogLevel` and
 `LogMethod`. If we want logging capability while we load the default prefs
 then we must initialize `TLLoggerInternal` first.

 I decided against moving around the code, and instead I left the
 `getIntPref()` calls as-is. This means the log level and log method are
 not set by the prefs.js value.

 >
 > > > 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`
 >

 Ah, good catch. I...missed that. The problem here is Firefox now formats
 the date/time according to the configured locale. We do have the option of
 hard coding the date/time format, but that is not the direction Mozilla
 are going in with respect to handling date/time format.

 > 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!

 Getting closer, but just a few more lose ends.

 I pushed `bug25750_2`.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25750#comment:21>
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