update Tor Launcher for ESR 60

Tor Launcher will need some updates for compatibility with the Firefox ESR 60 code. There is one issue we know of so far:

Replace nsIScriptableDateFormat with Intl.DateTimeFormat(). See https://bugzilla.mozilla.org/show_bug.cgi?id=1313625

added TorBrowserTeam201805R component::applications/tor launcher ff60-esr owner::brade priority::very high resolution::fixed severity::normal status::closed type::defect labels

Trac:
Priority: Medium to Very High

Trac:
Keywords: N/A deleted, TorBrowserTeam201804, ff60-esr added

Trac:
Description: Tor Launcher will need some updstes for compatibility with the Firefox ESR 60 code. There is one issue we know of so far:

Replace nsIScriptableDateFormat with Intl.DateTimeFormat(). See https://bugzilla.mozilla.org/show_bug.cgi?id=1313625

to

Tor Launcher will need some updates for compatibility with the Firefox ESR 60 code. There is one issue we know of so far:

Replace nsIScriptableDateFormat with Intl.DateTimeFormat(). See https://bugzilla.mozilla.org/show_bug.cgi?id=1313625

Trac:
Cc: N/A to anonym, intrigeri

As a prerequisite for #24856 (moved) and #25260 (moved), I think we can concentrate on getting this working. I have a branch with some small changes, and it works - except firefox does not read the default prefs (defaults/preferences/prefs.js). I'm investigating that now.

We may not need #25260 (moved) - The directory structure we currently use in Tor Browser works with FF60, too. But I'll keep #25260 (moved) open until we are certain we don't need it for the Android build. For testing I built Arthur's 25543+7 branch (./mach configure/build/package):

• Created a new xpi (make package) from the below branch
• Untarred the newly built firefox built from Arthur's branch
• Copied the TorBrowser/ dir from a 7.5.3 release
• Copied new version of libstdc++.so.6 into TorBrowser/Tor/ (I copied the system install I have /lib64/libstdc++.so.6 - libxul needs a newer version than the version included with 7.5.3)
• Copied the new TorLauncher xpi into the profile (firefox/TorBrowser/Data/Browser/profile.default/extensions/tor-launcher@torproject.org.xpi)
• Copied start-tor-browser from the 7.5.3 release, commented out FONTCONFIG_PATH and FONTCONFIG_FILE
• Run ./firefox/start-tor-browser -v -l

There are a few errors from torbutton, but I think igt0 has patches for (some of) them already. I haven't see any problems with nsIScriptableDateFormat, but it's worth making the change mcs mentioned.

Branch 25750 on git.tp.o/user/sysrqb/tor-launcher.git

Trac:
Cc: anonym, intrigeri to anonym, intrigeri, arthuredelstein

Replying to sysrqb:

As a prerequisite for #24856 (moved) and #25260 (moved), I think we can concentrate on getting this working. I have a branch with some small changes, and it works - except firefox does not read the default prefs (defaults/preferences/prefs.js). I'm investigating that now.

I guess [1413413] explains this. "Remove support for extensions having their own prefs file"

I'll try reverting the changeset - hopefully it applies cleanly.

This is "update Tor Launcher for ESR 60", not the opposite ;)

Replying to sysrqb:

I'll try reverting the changeset - hopefully it applies cleanly.

It doesn't. Instead, I added another commit on branch bug25750 that loads the prefs.js file at initialization and sets the prefs on the default prefs branch. We'll need the same in torbutton, too.

I'll test the other functionality now - bridges, fascistfirewall, etc.

Replying to sysrqb:

Replying to sysrqb:

I'll try reverting the changeset - hopefully it applies cleanly.

It doesn't. Instead, I added another commit on branch bug25750 that loads the prefs.js file at initialization and sets the prefs on the default prefs branch. We'll need the same in torbutton, too.

I'll test the other functionality now - bridges, fascistfirewall, etc.

This is looking good. I tested with igt0's torbutton patches from #25013 (moved) [0] and set/getconf updates with tor are working.

So far tor-launcher is looking good when run using Arthur's patched FF60.

I'll set this to needs-review for the tor-launcher patches I have: Branch bug25750 on git.tp.o/user/sysrqb/tor-launcher.git

[0] https://github.com/igortoliveira/torbutton/tree/25013

Trac:
Status: new to needs_review

I also pushed another commit on that branch for replacing nsIScriptableDateFormat. I think we can use toLocaleString() directly (instead of using Intl.DateTimeFormat()), but I'll update the branch and use Intl.DateTimeFormat() if there's a reason.

Trac:
Cc: anonym, intrigeri, arthuredelstein to anonym, intrigeri, arthuredelstein, brade

Trac:
Keywords: TorBrowserTeam201804 deleted, TorBrowserTeam201805R added

Trac:
Cc: anonym, intrigeri, arthuredelstein, brade to anonym, intrigeri, arthuredelstein, mcs

Replying to sysrqb:

Replying to sysrqb:

I'll try reverting the changeset - hopefully it applies cleanly.

It doesn't. Instead, I added another commit on branch bug25750 that loads the prefs.js file at initialization and sets the prefs on the default prefs branch. We'll need the same in torbutton, too.

I filed #26039 (moved) to track a related problem.

Replying to sysrqb:

I'll set this to needs-review for the tor-launcher patches I have: Branch bug25750 on git.tp.o/user/sysrqb/tor-launcher.git

Kathy and I reviewed and ran the revised Tor Launcher in a ESR 60-ish browser. It is working pretty well – good work! We have a few comments:

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@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).

2. For 5e5fba0951b0837948bd9c074a3710afe137d0eb, please omit the first change.

3. For 6105ea64e74cf65e09755d917234fe63b49b84ba, you could use a shorter commit message, e.g., Mozilla dropped support for Ci.nsIProgrammingLanguage.JAVASCRIPT See https://bugzilla.mozilla.org/show_bug.cgi?id=1149830#c18

4. For 068b9b5e8bb0408879b984c7c7ebd3726f25dd6c you could shorten the commit message and reference the entire bugzilla.mozilla.org URL. Maybe just use: Gecko now requries "0o"-prefixed octal literals See https://bugzilla.mozilla.org/show_bug.cgi?id=1263074

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.
• Please use let instead of var in new code when possible (we are migrating in that direction).
• Please use the Cc and Ci constants.
• Please log a message when ignoring invalid pref names (near the top of the pref() function).
• You can use this.mPrefsSvc inside _getPrefDefaultBranch(); if you do that, it might not be worthwhile to have a _getPrefDefaultBranch() function.

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).

Trac:
Keywords: TorBrowserTeam201805R deleted, TorBrowserTeam201805 added
Status: needs_review to needs_revision

Trac:
Cc: anonym, intrigeri, arthuredelstein, mcs to anonym, intrigeri, arthuredelstein, mcs, igt0

Replying to mcs:

Replying to sysrqb:

I'll set this to needs-review for the tor-launcher patches I have: Branch bug25750 on git.tp.o/user/sysrqb/tor-launcher.git

Kathy and I reviewed and ran the revised Tor Launcher in a ESR 60-ish browser. It is working pretty well – good work! We have a few comments:

Thanks!

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@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.

2. For 5e5fba0951b0837948bd9c074a3710afe137d0eb, please omit the first change.

Done.

3. For 6105ea64e74cf65e09755d917234fe63b49b84ba, you could use a shorter commit message, e.g., Mozilla dropped support for Ci.nsIProgrammingLanguage.JAVASCRIPT See https://bugzilla.mozilla.org/show_bug.cgi?id=1149830#c18

Done.

4. For 068b9b5e8bb0408879b984c7c7ebd3726f25dd6c you could shorten the commit message and reference the entire bugzilla.mozilla.org URL. Maybe just use: Gecko now requries "0o"-prefixed octal literals See https://bugzilla.mozilla.org/show_bug.cgi?id=1263074

Done.

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.

• Please use let instead of var in new code when possible (we are migrating in that direction).
• Please use the Cc and Ci constants.
• Please log a message when ignoring invalid pref names (near the top of the pref() function).

All done.

• 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.

One option, which requires more changes, is changing mPrefsSvc a nsIPrefBranch. Then we can use both getBranch() and getDefaultBranch() from that.

https://developer.mozilla.org/en-US/docs/Archive/Add-ons/Code_snippets/Preferences#Default_preferences

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).

Done.

I have pushed a new branch: bug25750_1

Trac:
Status: needs_revision to needs_review

Replying to 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@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 (moved), we found that we had to make two changes:

• Add Cu.import("re[/gre/modules/Services.jsm");](/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!

Trac:
Status: needs_review to needs_revision

I took a quick look in the code and I have a question about d104e7ecd35b2dbd38cdc9988fbd5924857d857d, does it load all the default properties every time the browser is restarted?

Replying to igt0:

I took a quick look in the code and I have a question about d104e7ecd35b2dbd38cdc9988fbd5924857d857d, does it load all the default properties every time the browser is restarted?

Yes, I think so. Do you think that is a problem? I suspect that is what the code that Mozilla removed did too.

If addressing this for Torbutton is really messy, maybe we should put more effort into reverting the Mozilla patch that removed support for default preferences (doing so would fix #26039 (moved) as well). But I defer to Matt who already tried that approach.

Replying to mcs:

Replying to 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 (moved), we found that we had to make two changes:

• Add Cu.import("re[/gre/modules/Services.jsm");](/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.

Replying to mcs:

Replying to igt0:

I took a quick look in the code and I have a question about d104e7ecd35b2dbd38cdc9988fbd5924857d857d, does it load all the default properties every time the browser is restarted?

Yes, I think so. Do you think that is a problem? I suspect that is what the code that Mozilla removed did too.

Yes, that was my understanding.

If addressing this for Torbutton is really messy, maybe we should put more effort into reverting the Mozilla patch that removed support for default preferences (doing so would fix #26039 (moved) as well). But I defer to Matt who already tried that approach.

Basically, after Mozilla removed this functionality, they simplified the remaining code (inlining other functions because now they only have a single call site, deleting and refactoring other functions). From what I saw, as I began reverting the commit and integrating the code into the current logic, it was a bit complicated - so I decided implementing the loading in the extension was much, much easier.

Replying to sysrqb:

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.

Although I agree with what I said, it's a terrible UX for users, so we should tell the user (somehow) that Tor Browser is in an undefined state. The only other option I see is we get rid of prefs.js entirely and somehow make sure all get...Prefs() calls use the correct default prefs.

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.

Similar to what was mentioned above. I worry about this situation, but as long as we're careful about keeping prefs.js and the getIntPrefs() calls synchronized, this shouldn't be a problem.

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.

Right, so we have a few options and available formats, from what I see. The built-in formats do not play nicely with appending the milliseconds at the end, because some locales use 12-hour clocks and have AM/PM suffix, plus some add the timezone. We'd need additional logic for detecting this. The four built-in formats for en-US are (with milliseconds appended):

dateStyle: "long", timeStyle: "long"
May 11, 2018, 8:45:58 PM UTC.436

dateStyle: "short", timeStyle: "long"
5/11/18, 8:45:58 PM UTC.436

dateStyle: "long", timeStyle: "short"
May 11, 2018, 8:45 PM.436

dateStyle: "short", timeStyle: "short"
5/11/18, 8:45 PM.436

I'd like feedback about the best direction we should go with these.

Trac:
Status: needs_revision to needs_review

Replying to sysrqb:

Although I agree with what I said, it's a terrible UX for users, so we should tell the user (somehow) that Tor Browser is in an undefined state. The only other option I see is we get rid of prefs.js entirely and somehow make sure all get...Prefs() calls use the correct default prefs.

I think this situation is rare enough that we should not spend a lot of time trying to make it better from a UX perspective. Users should not edit the prefs.js file that is embedded in Tor Launcher (which is the most likely reason that loading the default prefs might fail).

Similar to what was mentioned above. I worry about this situation, but as long as we're careful about keeping prefs.js and the getIntPrefs() calls synchronized, this shouldn't be a problem.

Kathy and I agree.

Right, so we have a few options and available formats, from what I see. The built-in formats do not play nicely with appending the milliseconds at the end, because some locales use 12-hour clocks and have AM/PM suffix, plus some add the timezone. We'd need additional logic for detecting this. The four built-in formats for en-US are (with milliseconds appended):

dateStyle: "long", timeStyle: "long"
May 11, 2018, 8:45:58 PM UTC.436

dateStyle: "short", timeStyle: "long"
5/11/18, 8:45:58 PM UTC.436

dateStyle: "long", timeStyle: "short"
May 11, 2018, 8:45 PM.436

dateStyle: "short", timeStyle: "short"
5/11/18, 8:45 PM.436
}}}

I'd like feedback about the best direction we should go with these.

Kathy and I spent a little time looking for an alternative. It seems we have more control if we use JavaScript's built-in Intl.DateTimeFormat object, i.e., {{{ let options = { year: '2-digit', month: 'numeric', day: 'numeric', hour: 'numeric', minute: 'numeric', second: 'numeric', hour12: false }; let timeStr = new Intl.DateTimeFormat('en-US', options) .format(logObj.date, options);

We have a few review comments for your bug25750_2 branch but we will post those in a separate comment.

For 0219dcd1aaef1a5460bec39cdd95742222ff9c08, a couple of "leftover" functions are defined in modules/tl-util.jsm but they are not used; please remove them:

• _defaultPrefsLoaded()
• loadDefaultPreferencesAndThrow()

Replying to mcs:

Replying to sysrqb:

Although I agree with what I said, it's a terrible UX for users, so we should tell the user (somehow) that Tor Browser is in an undefined state. The only other option I see is we get rid of prefs.js entirely and somehow make sure all get...Prefs() calls use the correct default prefs.

I think this situation is rare enough that we should not spend a lot of time trying to make it better from a UX perspective. Users should not edit the prefs.js file that is embedded in Tor Launcher (which is the most likely reason that loading the default prefs might fail).

Similar to what was mentioned above. I worry about this situation, but as long as we're careful about keeping prefs.js and the getIntPrefs() calls synchronized, this shouldn't be a problem.

Kathy and I agree.

Great, thanks! I added a commit that synchronizes a few get..Pref() calls default values. I don't think we need all of them in sync, but it seems safer if retrieving a pref during initialization uses the same default value as defined in prefs.js.

Right, so we have a few options and available formats, from what I see. The built-in formats do not play nicely with appending the milliseconds at the end, because some locales use 12-hour clocks and have AM/PM suffix, plus some add the timezone. We'd need additional logic for detecting this. The four built-in formats for en-US are (with milliseconds appended):

Kathy and I spent a little time looking for an alternative. It seems we have more control if we use JavaScript's built-in Intl.DateTimeFormat object, i.e.,

let options = { year: '2-digit', month: 'numeric', day: 'numeric',
                hour: 'numeric', minute: 'numeric', second: 'numeric',
                hour12: false };
let timeStr = new Intl.DateTimeFormat('en-US', options)
                      .format(logObj.date, options);
}}}

Thanks, that looks good. The only difference I see is this format inserts a comma between the date and time.

{{{ 5/15/18, 23:20:43.525 [NOTICE] Bootstrapped 100%: Done

Replying to [mcs](#note_25):
> For 0219dcd1aaef1a5460bec39cdd95742222ff9c08, a couple of "leftover" functions are defined in `modules/tl-util.jsm` but they are not used; please remove them:
> * `_defaultPrefsLoaded()`
> * `loadDefaultPreferencesAndThrow()`

Done.

I pushed `bug25750_3` for review. Thanks.

Trac:
Keywords: TorBrowserTeam201805 deleted, TorBrowserTeam201805R added
Cc: anonym, intrigeri, arthuredelstein, mcs, igt0 to anonym, intrigeri, arthuredelstein, mcs, igt0, tbb-team

Replying to sysrqb:

Great, thanks! I added a commit that synchronizes a few get..Pref() calls default values. I don't think we need all of them in sync, but it seems safer if retrieving a pref during initialization uses the same default value as defined in prefs.js.

Thanks for doing that. The patch looks good except for three things:

A. In src/chrome/content/network-settings.js, you do not want to add a second parameter to this line: let prefBranch = TorLauncherUtil.getPrefBranch(kPrefBranchBridgeDBBridge); but rather to this one: let bridgeType = TorLauncherUtil.getCharPref(kPrefBridgeDBType);

B. In src/components/tl-protocol.js, I do not think we should change the code that retrieves network.proxy.socks_port. It was written to treat Firefox's default value of 0 as 9150.

C. In src/modules/tl-logger.jsm, Kathy thinks we should leave the default default for extensions.torlauncher.loglevel as 0 so that if defaults/preferences/prefs.js cannot be read the user will get all logging (which may help with debugging whatever is wrong in their environment).

Thanks, that looks good. The only difference I see is this format inserts a comma between the date and time.

5/15/18, 23:20:43.525 [NOTICE] Bootstrapped 100%: Done
}}}

Hmmm. On macOS at least, I see the comma with 8.0a7 as well: {{{ 5/16/18, 14:49:14.500 [NOTICE] Bootstrapped 100%: Done

Maybe things were different on other platforms though. Anyway, I think we are close enough now to the old format that we won't get many complaints (famous last words!)

**Trac**:  
**Status**: needs_review **to** needs_revision   
**Keywords**: TorBrowserTeam201805R **deleted**,  TorBrowserTeam201805 **added**

Replying to mcs:

Replying to sysrqb:

Great, thanks! I added a commit that synchronizes a few get..Pref() calls default values. I don't think we need all of them in sync, but it seems safer if retrieving a pref during initialization uses the same default value as defined in prefs.js.

Thanks for doing that. The patch looks good except for three things:

A. In src/chrome/content/network-settings.js, you do not want to add a second parameter to this line: let prefBranch = TorLauncherUtil.getPrefBranch(kPrefBranchBridgeDBBridge); but rather to this one: let bridgeType = TorLauncherUtil.getCharPref(kPrefBridgeDBType);

Ugh. Fixed.

B. In src/components/tl-protocol.js, I do not think we should change the code that retrieves network.proxy.socks_port. It was written to treat Firefox's default value of 0 as 9150.

Ah, right. I reverted that and added a comment so hopefully someone else doesn't do the same thing.

C. In src/modules/tl-logger.jsm, Kathy thinks we should leave the default default for extensions.torlauncher.loglevel as 0 so that if defaults/preferences/prefs.js cannot be read the user will get all logging (which may help with debugging whatever is wrong in their environment).

Sounds good.

Thanks, that looks good. The only difference I see is this format inserts a comma between the date and time.

{{{ 5/15/18, 23:20:43.525 [NOTICE] Bootstrapped 100%: Done }}}

Hmmm. On macOS at least, I see the comma with 8.0a7 as well: {{{ 5/16/18, 14:49:14.500 [NOTICE] Bootstrapped 100%: Done }}}

Maybe things were different on other platforms though. Anyway, I think we are close enough now to the old format that we won't get many complaints (famous last words!)

I agree! Okay, I pushed a new branch. bug25750_4. Also, it seems like meek-http-helper needs updating because it imports re[/gre/modules/devtools/Console.jsm](/gre/modules/devtools/Console.jsm) which is a shim for re[/gre/modules/Console.jsm](/gre/modules/Console.jsm) in 52ESR, but the shim was deleted in Bug 1386535. I'll open another ticket for that.

Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam201805 deleted, TorBrowserTeam201805R added

The bug25750_4 branch looks good. There is one small typo in the comment you added: "use 9150 is we get 0" should be "use 9150 if we get 0" Georg, if this looks good to you after Matt fixes the typo please merge these patches into master.

Replying to mcs:

The bug25750_4 branch looks good. There is one small typo in the comment you added: "use 9150 is we get 0" should be "use 9150 if we get 0"

:( Pushed bug25750_5 without a typo in the comment.

Replying to sysrqb:

Also, it seems like meek-http-helper needs updating because it imports re[/gre/modules/devtools/Console.jsm](/gre/modules/devtools/Console.jsm) which is a shim for re[/gre/modules/Console.jsm](/gre/modules/Console.jsm) in 52ESR, but the shim was deleted in Bug 1386535. I'll open another ticket for that.

Opened #26118 (moved) for tracking

Some comments (just nits):

commit 785568d05b12f819a29b90c2dcd4e55b821a2047: Any reason why the minVersion for Fennec is 45 and not 52? (I am fine with leaving the patch you have given that we are not really enforcing 52 for desktop either, just curious)

commit 2e1e760a8393de281318e97ef44b2e89ba67879c "Gecko now requires "0o"-prefixed octal literals" <- Are you sure about that? Yes, the warning shows up in the browser console but the bug you are citing is already fixed in Firefox 48, yet Tor Browser 7 does not show the warning. Fixing the octals is good, though. I hunted a bit but finding the actual bug behind this change seems a bit tricky. I think we could just say "Fix deprecated octal literals" in the commit message and move on.

You are using "Bug XXXXXXX" and "bug XXXXXXX" for referencing Mozilla bugs within a sentence. I think you should stick to one format and the latter is the better one.

commit 039bd44ce1a65bbc7bcacfa7a6b114b744a84b8f

s/var loader/let loader/

+      TorLauncherLogger.log(5,"Ignoring invalid pref ending with a period: '" +

Whitspace between "," and """.

You want to have pairwise "'" but are forgetting sometimes the closing one.

commit 3f2936d1323c36d1882b81ab155bf9fd48e27a37

The indentation of the new code block is off by one.

Trac:
Keywords: TorBrowserTeam201805R deleted, TorBrowserTeam201805 added
Status: needs_review to needs_revision

Replying to sysrqb:

Replying to mcs:

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@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.

I agree, I've filed #26127 (moved) for that.

Replying to gk:

Some comments (just nits):

commit 785568d05b12f819a29b90c2dcd4e55b821a2047: Any reason why the minVersion for Fennec is 45 and not 52? (I am fine with leaving the patch you have given that we are not really enforcing 52 for desktop either, just curious)

No, I don't remember why I chose that. Good question.

commit 2e1e760a8393de281318e97ef44b2e89ba67879c "Gecko now requires "0o"-prefixed octal literals" <- Are you sure about that? Yes, the warning shows up in the browser console but the bug you are citing is already fixed in Firefox 48, yet Tor Browser 7 does not show the warning. Fixing the octals is good, though. I hunted a bit but finding the actual bug behind this change seems a bit tricky. I think we could just say "Fix deprecated octal literals" in the commit message and move on.

The runtime now throws a syntax error:

JavaScript error: jar:file:///home/user/firefox/TorBrowser/Data/Browser/profile.default/extensions
/tor-launcher@torproject.org.xpi!/components/tl-process.js, line 1003: SyntaxError: "0"-prefixed octal
 literals and octal escape sequences are deprecated; for octal literals use the "0o" prefix instead

But, I see 52ESR should throw the same error at runtime, so I'm not sure why TorLauncher and TorButton currently work.

You are using "Bug XXXXXXX" and "bug XXXXXXX" for referencing Mozilla bugs within a sentence. I think you should stick to one format and the latter is the better one.

Agreed, I'll change this.

commit 039bd44ce1a65bbc7bcacfa7a6b114b744a84b8f

s/var loader/let loader/ {{{

•  TorLauncherLogger.log(5,"Ignoring invalid pref ending with a period: '" +

}}} Whitspace between "," and """.

I did this so the line length is less than 80 chars. If you prefer adding the space then we can move the format string onto the next line.

You want to have pairwise "'" but are forgetting sometimes the closing one.

I'll review this and confirm there are matching quotes.

commit 3f2936d1323c36d1882b81ab155bf9fd48e27a37

The indentation of the new code block is off by one.

I'll change this.

Replying to sysrqb:

Replying to gk:

commit 2e1e760a8393de281318e97ef44b2e89ba67879c "Gecko now requires "0o"-prefixed octal literals" <- Are you sure about that? Yes, the warning shows up in the browser console but the bug you are citing is already fixed in Firefox 48, yet Tor Browser 7 does not show the warning. Fixing the octals is good, though. I hunted a bit but finding the actual bug behind this change seems a bit tricky. I think we could just say "Fix deprecated octal literals" in the commit message and move on.

The runtime now throws a syntax error:

{{{ JavaScript error: jar:file:///home/user/firefox/TorBrowser/Data/Browser/profile.default/extensions /tor-launcher@torproject.org.xpi!/components/tl-process.js, line 1003: SyntaxError: "0"-prefixed octal literals and octal escape sequences are deprecated; for octal literals use the "0o" prefix instead }}}

But, I see 52ESR should throw the same error at runtime, so I'm not sure why TorLauncher and TorButton currently work.

I would not worry too much about that as long as the commit message reflects what we know. (Although I can feel the urge to find out what's up) :)

commit 039bd44ce1a65bbc7bcacfa7a6b114b744a84b8f

s/var loader/let loader/ {{{

•  TorLauncherLogger.log(5,"Ignoring invalid pref ending with a period: '" +

}}} Whitspace between "," and """.

I did this so the line length is less than 80 chars. If you prefer adding the space then we can move the format string onto the next line.

Aha, I think we should have the same spacing requirements for all code and use a new line if that's indeed needed.

I think I corrected all of GK's comments. I pushed branch bug25750_6.

Trac:
Status: needs_revision to needs_review

Replying to sysrqb:

I think I corrected all of GK's comments. I pushed branch bug25750_6.

Looks good, thanks.

Trac:
Keywords: TorBrowserTeam201805 deleted, TorBrowserTeam201805R added

The bug25750_6 branch looks good to me too.

Thanks all! I merged this to master with commit b15da273aa1c6de5a1725e24a171ad043864bf69.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

mentioned in issue #26073 (moved)

mentioned in issue #26118 (moved)

moved to tpo/applications/tor-launcher#25750 (closed)