Opened 19 months ago

Closed 19 months ago

Last modified 17 months ago

#21723 closed defect (fixed)

Fix inconsistent generation of MOZ_MACBUNDLE_ID

Reported by: teor Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201704R
Cc: mcs, brade Actual Points:
Parent ID: #17670 Points:
Reviewer: Sponsor:

Description

Please see my branch fix-macos-bundle-id on https://github.com/teor2345/tor-browser.git

This appears to be a Mozilla bug, we should upstream it.

Child Tickets

Attachments (1)

0001-Bug-21723-Fix-inconsistent-generation-of-MOZ_MACBUND.patch (1.1 KB) - added by mcs 19 months ago.
patch for ESR52-based browser

Download all attachments as: .zip

Change History (12)

comment:1 Changed 19 months ago by gk

Cc: mcs brade added
Keywords: TorBrowserTeam201703R added
Status: newneeds_review

comment:2 Changed 19 months ago by mcs

Thanks for working on this. I have not tested this, but a quick glance at the uses of MOZ_MACBUNDLE_NAME within the browser code tells me we may not want to force it to lowercase because doing so will change our .app bundle from TorBrowser.app to torbrowser.app which is is less OSX-like and may also require changes to our packaging scripts, etc.

MOZ_MACBUNDLE_ID should definitely be fixed though, and I like org.torproject.torbrowser better than org.mozilla.torbrowser (but that will require a larger patch or scarier build config changes).

comment:3 in reply to:  2 Changed 19 months ago by teor

Replying to mcs:

Thanks for working on this. I have not tested this, but a quick glance at the uses of MOZ_MACBUNDLE_NAME within the browser code tells me we may not want to force it to lowercase because doing so will change our .app bundle from TorBrowser.app to torbrowser.app which is is less OSX-like and may also require changes to our packaging scripts, etc.

You're right, I didn't notice that this was a different variable.
I reverted this change.

MOZ_MACBUNDLE_ID should definitely be fixed though,

I have revised this patch in my branch fix-macos-bundle-id-v2.
It now removes all invalid characters, not just spaces.

I tested it using:
echo "Tor Browser" | tr 'A-Z' 'a-z' | tr -dc 'a-z-'
on macOS and Ubuntu.

The [] are unnecessary on modern tr, and lead to unexpected results when used with tr -d.

a-z might not work when configure is run in a non-English locale, we can replace them with octal if this is a concern. (I expect this is not a practical issue, because otherwise the existing tr [A-Z] [a-z] would have failed.)

and I like org.torproject.torbrowser better than org.mozilla.torbrowser (but that will require a larger patch or scarier build config changes).

This can be implemented by passing the following argument to Firefox's configure:
--with-distribution-id=org.torproject

Please see my branch distribution-torproject on https://github.com/teor2345/tor-browser-build.git for this change.

comment:4 Changed 19 months ago by mcs

Kathy and I think we should adopt these changes for our first Firefox ESR52-based alpha release of Tor Browser (which is to say, we should test these kinds of changes in our alpha before we release them to everyone).

We rebased your configure.in patch for ESR52; I will attach the rebased patch to this ticket in a few minutes. The general rebasing work is being done in #20680 but the code is not yet in a torproject.org git repo.

We also created a patch for our gitian-based build system that sets the distribution ID:
https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug21723-01&id=a0bc7b6d29ec7b0b46d669de92ff171ce4af188e

You already did that work for our new RBM-based build system:
https://github.com/teor2345/tor-browser-build/commit/8cda511cc0c89aec33d8ca8761c16552cff36cd3

We will want all of these patches.

Changed 19 months ago by mcs

patch for ESR52-based browser

comment:5 Changed 19 months ago by gk

In the commit message you write "This results in an OSX bundle ID of org.torproject.torbrowser." but we adapt the descriptors for Linux and Windows as well. I am fine doing that if it fixes a more generic issue (does it?) but then we should amend the commit message.

comment:6 in reply to:  5 ; Changed 19 months ago by teor

Replying to gk:

In the commit message you write "This results in an OSX bundle ID of org.torproject.torbrowser." but we adapt the descriptors for Linux and Windows as well. I am fine doing that if it fixes a more generic issue (does it?) but then we should amend the commit message.

The build change --with-distribution-id=org.torproject changes the distribution ID on all platforms. I'm not sure how the distribution ID is used on Linux or Windows, but we should be consistent.

The configure change is to MOZ_MACBUNDLE_ID, I assume it is only used on macOS.

comment:7 in reply to:  6 Changed 19 months ago by mcs

Replying to teor:

The build change --with-distribution-id=org.torproject changes the distribution ID on all platforms. I'm not sure how the distribution ID is used on Linux or Windows, but we should be consistent.

I agree that we should be consistent (unless we find a good reason not to be). Here are the references to MOZ_DISTRIBUTION_ID (which is what --with-distribution-id sets):
https://dxr.mozilla.org/mozilla-esr52/search?q=MOZ_DISTRIBUTION_ID

Files such as browser/branding/aurora/locales/moz.build use MOZ_DISTRIBUTION_ID to define MOZ_DISTRIBUTION_ID_UNQUOTED, but I do not see where that is used.

In old-configure.in, MOZ_DISTRIBUTION_ID is used to define MOZ_MACBUNDLE_ID, as teor pointed out.

The code in toolkit/xre/nsAppRunner.cpp makes the MOZ_DISTRIBUTION_ID value available via nsIXULRuntime.distributionID. Chasing that via:
https://dxr.mozilla.org/mozilla-esr52/search?q=distributionID
shows that it is used in telemetry and in the search engine code. We don't care about telemetry, but the search engine code will insert the distribution ID into search parameters via {moz:distributionID}. But the only place I see that technique used is in browser/locales/searchplugins/yahoo-jp-auctions.xml, which I assume we will not ship with Tor Browser.

So... I think it is okay to change the distribution ID on all platforms.

comment:8 Changed 19 months ago by gk

The changes to tor-browser will appear in commit 86bf10d413f08592ecbf72c80061e43c8a8e3b73 on tor-browser-52.0.2esr-7.0-1 once it is pushed.

comment:9 Changed 19 months ago by gk

Keywords: TorBrowserTeam201704R added; TorBrowserTeam201703R removed

Moving review tickets to April.

comment:10 Changed 19 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

The tor-browser-bundle part is on master and hardened-builds (commit 50b6e6fff4a535044f9b506854535df757125267 and 84f386e0783e1972db72b94df755766a5eb6e9ce).

comment:11 Changed 17 months ago by teor

If Tor Browser is the default browser in macOS, this change resets it. That's what we expected. See https://trac.torproject.org/projects/tor/ticket/21724#comment:7 for details.

Note: See TracTickets for help on using tickets.