Skip to content

Bug 41647 & 41657 & 41453: macro cleanup

Pier Angelo Vendrame requested to merge pierov/tor-browser:bug_41647 into tor-browser-102.8.0esr-12.5-1

Merge Info

Related Issues

Backport Timeline

  • Immediate - patchsets for critical bug fixes or other major blocker (e.g. fixes for a 0-day exploit) OR patchsets with trivial changes which do not need testing (e.g. fixes for typos or fixes easily verified in a local developer build)
  • Next Minor Stable Release - patchset that needs to be verified in nightly before backport
  • Eventually - patchset that needs to be verified in alpha before backport
  • No Backport - patchset for the next major stable

Upstream Merging

  • Merge to base-browser - typically for !fixups to patches in the base-browser branch, though sometimes new patches as well
    • NOTE: if your changeset includes patches to both base-browser and tor-browser please please make separate merge requests for each part
    • It will probably need another MR
    • It will also probably need another rebase

Issue Tracking

Change Description

Changes

  • --with-tor-browser-version --> --with-base-browser-version, removed --enable-base-browser
    • we need to replace ifdef BASE_BROWSER with ifdef BASE_BROWSER_VERSION, and some TOR_BROWSER_VERSION with BASE_BROWSER_VERSION
    • this macro needs to be available to JS with the AppConstants mechanism
    • for some other occurrences, I changed TOR_BROWSER_VERSION with TOR_BROWSER, which the patch hardcodes as always defined
      • does this make sense? Or should we have a way to enable/disable it?
    • I've removed set_config("TOR_BROWSER_VERSION"), because it seems to me that we never used that with CONFIG["TOR_BROWSER_VERSION"] in any moz.build file
    • what to check:
      • the build must fail when not setting --with-base-browser-version as a failsafe
      • builds must not fail in any of our platforms (Android included), because of configure flags that don't exist anymore
      • wherever we used one of the changed macros to disable a certain mechanism, it needs to continue to work as expected
      • the custom version needs to be displayed wherever needed
        • the about dialog
        • the preferences page
        • mechanisms to detect whether we've just updated (needed in about:tor, but also for something in toolkit/mozapps/extensions/AddonManager.jsm, and used to purge startup cache in nsAppRunner.cpp)
      • we check on this to disable monitoring network changes on Android, therefore this patch should be moved to the Tor Browser part of the patchset in the next rebase! Also, how do we test this?
  • Removed --enable-tor-browser-data-outside-app-dir
    • Rationale: in my previous changes for the profile directory, I've tried not to hardcode the path of the profile directory, and to make it possible to specify a directory outside the Browser.
    • Additional usages of the macro:
      • TorLauncher (partially, instead of using the macro directly, we also checked if the app directory was included in the profile directory). However, we have better ways to find the paths to the files, so this patch also tries to use them
      • Changelogs: if I understand correctly, this wasn't even needed, since the changelogs are always in the program directory
      • Updater: some files that normally are in the profile directory are excluded from the updater. I have added this to a comment, instead.
    • What to test:
      • TorLauncher: tor still launches, doesn't log warnings/errors in the console, all PTs keep working, and Moat requests succeed (Moat.jsm needs the path to obfs4client, to do domain fronting requests)
      • Changelogs: the changelogs are still displayed, in all platforms
      • Updater:
  • Removed x86_64 from the macOS mozconfig
    • What to test:
      • macOS still builds

Relative paths

I've created a utility function to check if a path is relative, rather than copying in more than one place the regex.

While doing so, I've also updated the Windows version of the regex to accept paths starting with \\ as absolute paths.

Is this okay, or is it something we didn't want to do on purpose? In the latter case, we should have added a comment.

New commit at base browser

Should we move the version changes in a commit on its own?

Localization problems

$ourVersion (based on Mozilla Firefox $firefoxVersion) in the about dialog/browser/base/content/aboutDialog.js is not localized.

  • Probably we should localize it
  • We might want to do a strings commit: having a file only for a string isn't great, because it will need a component on its own etc etc etc.
  • However, the about dialog itself will need additional strings. So, we could wait to have a commit on its own also for this reason

Firefox version in the preferences

At a certain point, we've switched to display the Tor Browser version on about:preferences, but we don't show the original Firefox version. We could do it, since we're at it (with the same caveat of localization we have for 2.1).

SKIPLIST_COUNT - 2

In the various changes we do to the updater, we change SKIPLIST_COUNT in CopyInstallDirToDestDir becasue we need to skip a few more files (and, Mozilla, seriously, don't abuse macros, just use a constexpr size_t/constexpr unsigned 😡️).

However, we used to miss a file at SKIPLIST_COUNT - 2. I've updated the code to use it, instead of SKIPLIST_COUNT - 3, and decresed SKIPLIST_COUNT by 1.

I didn't find any obvious reason to skip SKIPLIST_COUNT - 2, but I thought it's worth pointing this out.

tor-browser-build

A tor-browser-build MR will be needed:

  • Change the version argument
  • Change the macOS mozconfig name

Merge request reports