Tor Browser on OS X should not store data into the application bundle
The Tor application on OS X stores user data into its bundle (TorBrowser.app/Data/). This is bad. This causes various issues:
-
the Tor application can't be code sign which decreases the security. See Ticket #13251 (moved): CodeSign Tor for OS X
-
when installing a new version of Tor, all previous user data (bookmarks) are deleted.
Trac:
Username: torosx
Activity
Trac:
Summary: Tor on OS X should not stored data into the application bundle to Tor Browser on OS X should not store data into the application bundle
Parent: N/A to #6540 (moved)
Mozilla hacked around this by placing stuff they needed to modify in MozResources: https://bugzilla.mozilla.org/show_bug.cgi?id=1047584.
-
OK, I attempted to sign our app dir via the command line using codesign as per: https://stackoverflow.com/questions/9245149/jenkins-on-os-x-xcodebuild-gives-code-sign-error and then https://lists.torproject.org/pipermail/tor-dev/2015-October/009780.html
I had to add a --keychain to codesign to specify a non-default keychain, but otherwise it was pretty much that verbatim.
Unfortunately, I did indeed hit issues. codesign bailed with this error: ./TBBDir/TorBrowser.app/: unsealed contents present in the bundle root
Some searching revealed that this likely means there are non-executable resources mixed in with directories that contain binaries. I think we may indeed have to reorganize the Mac bundle dir again for this.
mcs+brade: This seems like something you might be able to help with, since you reorganized our Mac bundles before? I can get you a Mac "developer" certificate if you give me a CSR using those jenkins instructions, if you need that. (Though tbh, I don't get what the difference is between the developer certs and the app store certs.. if there is no difference from our users' point of view, maybe we should find another way to test this.. Do we even need to involve Apple's official cert process at all to get past these errors?)
Trac:
Sponsor: N/A to N/A
Severity: N/A to Normal
Cc: N/A to mcs, brade 
Replying to mikeperry:
mcs+brade: This seems like something you might be able to help with, since you reorganized our Mac bundles before? I can get you a Mac "developer" certificate if you give me a CSR using those jenkins instructions, if you need that. (Though tbh, I don't get what the difference is between the developer certs and the app store certs.. if there is no difference from our users' point of view, maybe we should find another way to test this.. Do we even need to involve Apple's official cert process at all to get past these errors?)
Kathy and I will take a look. I think what we ultimately want for signing is a Production "Developer ID" certificate, not an App Store one. There are also "Mac Development" certificates that should work for testing the build process but presumably are not suitable for distribution of the .app bundle to end-users. Kathy and I should be able to generate certs for testing because we have access to an Apple Developer account (we use it for some iOS work and Apple merged all of their programs together, so in theory we can create certs for Mac apps also).
But the showstopper issue with the Tor Browser bundle structure is going to be the Tor and browser profile data that we store inside the bundle. I am almost certain that we will need to move all files that may change out of the .app bundle. Maybe we will have to ship a folder that contains a TorBrowser folder as well as TorBrowser.app (instead of having everything inside TorBrowser.app as we do today). Assuming that works with Gatekeeper, we then have the problem that users can break things by moving one folder without the other :(
-
Replying to mcs:
But the showstopper issue with the Tor Browser bundle structure is going to be the Tor and browser profile data that we store inside the bundle. I am almost certain that we will need to move all files that may change out of the .app bundle.
Digging a little deeper and experimenting on a Mac OS 10.10.5 system, Kathy and I learned some interesting things:
- It is possible to add a valid signature to Tor Browser if we remove the TorBrowser directory.
- It appears that the signature is only checked when the app bundle has a quarantine attribute in the file system. See http://www.cocoabuilder.com/archive/xcode/319946-revoking-gatekeeper-exceptions.html#320000
- After an application is opened once, the quarantine attribute is modified by the system so that the app bundle is no longer subjected to signature checks.
- It is possible to add a valid signature to Tor Browser if we move the TorBrowser directory under TorBrowser.app/Contents/.
Because of 2-4 above, we might be able to cheat a little and just relocate the TorBrowser directory. This will mean that our app bundle's signature will be broken as soon as Tor Browser is opened for the first time (this is because we make changes under TorBrowser/Data and Apple's signature "seals" everything under Contents/ -- nothing can be modified without invalidating the signature).
It is possible Apple will be even more strict in a future release of their Gatekeeper technology, so our other option is to keep our data outside TorBrowser.app (either in a side-by-side folder like Ricochet does or in the standard location under ~/Library/Application Support/).
What do other people think?
Trac:
Status: assigned to needs_information
Cc: mcs, brade to mcs, brade, gk, mikeperry, teor Replying to mcs:
Because of 2-4 above, we might be able to cheat a little and just relocate the TorBrowser directory. This will mean that our app bundle's signature will be broken as soon as Tor Browser is opened for the first time (this is because we make changes under TorBrowser/Data and Apple's signature "seals" everything under Contents/ -- nothing can be modified without invalidating the signature).
It is possible Apple will be even more strict in a future release of their Gatekeeper technology, so our other option is to keep our data outside TorBrowser.app (either in a side-by-side folder like Ricochet does or in the standard location under ~/Library/Application Support/).
What do other people think?
If it works for now, then that's a much better user experience. Except for users who pass around copies of Tor Browser, who will see the "gatekeeper" check on every new machine.
I think we should avoid storing data in Application Support, because it violates our "leave no disk traces" goal. Storing the data beside the app is ugly, but more obvious to the user. (And it makes it easier for them to share just Tor Browser with others, or reset their copy of Tor Browser.)
(I wish Apple had thought of apps that do not want to leave any traces when it designed Application Support and code signing etc.)
-
I think the app sharing case isn't that bad to break, because people should be sharing the .dmg rather than .app dirs. However, we may also be breaking the USB drive usecase with this change, since when you take the drive to a new computer Gatekeeper will barf at the modified app.
One option might be to do the hack only if we're in /Applications, but do side-by-side directory otherwise? But if that is too complicated, I think it's not too much to tell people that if they want to do the USB drive thing, they need to keep the .dmg on their USB drive and erase the TBB app every time. It would be better than status quo, where many people can't even figure out how to get the thing to run in the first place.
It is looking like long term for MacOS we're going to want an uninstaller/wipe button, though. Probably for Windows also, esp if we can clean up the registry at that point.
-
Replying to mikeperry:
I think the app sharing case isn't that bad to break, because people should be sharing the .dmg rather than .app dirs. However, we may also be breaking the USB drive usecase with this change, since when you take the drive to a new computer Gatekeeper will barf at the modified app.
I don't think the quarantine bit gets set when app bundles are copied to or from a Flash drive (but I do not have a computer that is running Mac OS 10.11 El Capitan, so it is possible Apple made changes in that OS release).
-
19:14 < GeKo> re 13252: i think i am fine with the cheating approach to get the signing going asap on the alphas to look for other issues 19:14 < GeKo> but i am actually worried about apply restricting their policy further with an update breaking that short-cut 19:14 < GeKo> leaving users with a scary warning again 19:15 < GeKo> which is especially bad as it worked until then 19:16 < GeKo> s/apply/apple/
I am not sure whether that would mean we should work on both things in parallel or just try The Real Thing. Especially if the latter turns out to be time-consuming, resulting in not having signed alpha bundles in our next alpha release on Jan 26.
-
If we implement the "side-by-side" solution where we have a TorBrowser-Data folder next to TorBrowser.app, then we will probably end up solving the profile creation problem as well (#14981 (moved)).
-
As we are proceeding with this work, Kathy and I are wondering whether we want to just move to a side-by-side data model on all platforms in the Tor Browser 6.0 timeframe.
Con: it will add some extra risk on Linux and Windows and some people may not like the change.
Pro: better cross-platform consistency and all platforms will get the benefits that go along with moving user data out of the area where the binaries live. Examples: easier and safer sharing of Tor Browser with other users, annoyances like #18179 (moved) will be eliminated, support for multiple profiles and new profile creation, it will be easy to "reset" and start over (just delete the TorBrowser-Data directory), etc.
Opinions?
Trac:
Status: assigned to needs_information -
I think that idea sounds like a good one generally. But I fear we won't have the time for 6.0 for this (given the lack of testing time alone). I think filing two additional bugs where the works gets done and getting that ready in the 6.5 timeframe seems like the better plan to me.
Trac:
Status: needs_information to assigned -
Replying to gk:
I think that idea sounds like a good one generally. But I fear we won't have the time for 6.0 for this (given the lack of testing time alone). I think filing two additional bugs where the works gets done and getting that ready in the 6.5 timeframe seems like the better plan to me.
I filed #18367 (moved) for Windows and #18369 (moved) for Linux.
-
See #18371 (moved) for a child ticket related to meek.
-
We have now completed most of the work for this ticket. With our changes, all user data is stored in a directory named TorBrowser-Data which is created next to the app bundle (the TorBrowser-Data directory does not exist initially; it is created by the browser the first time Tor Browser is opened). The content of TorBrowser-Data looks like this:
Browser/ Caches/ profiles.ini qf0enfzt.default/ (user's browser profile) Tor/ (Tor data directory) ... PluggableTransports/ profile.meek-http-helper/ (meek browser profile) ... torrc UpdateInfo/ (update history and temporary space)There are two major loose ends:
- FTE needs to be repackaged to separate the Python scripts from the compiled libraries.
- Kathy and I need to write code to relocate the user profile files during an upgrade from a version of Tor Browser that uses the old "embedded user data" layout to one that uses the new layout.
See #18371 (moved) for our meek changes.
Our tor-browser patches (two fixup commits plus another one) are on bug13252-01 branch, here: https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug13252-01 There is a new --enable-tor-browser-data-outside-app-dir configure option which is used to enable the new disk layout.
Our Tor Launcher patch is here: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13252-01&id=fe41b6d6032ffde989b860dd94a7f53301ef97ed With these changes, Tor Launcher should automatically handle both the old and new layout.
Our tor-browser-bundle (builder) patch is here: https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug13252-01&id=1c2485467efc31b5539585f39fc1a861d421fb28 The new packaging is triggered by setting DATA_OUTSIDE_APP_DIR=1 in the versions file (our patch sets it in versions.alpha).
Early reviews are welcome; just keep in mind that additional changes are needed to address the two loose ends I mentioned above.
Trac:
Cc: mcs, brade, gk, mikeperry, teor, mrphs, arlolra, arthuredelstein to mcs, brade, gk, mikeperry, teor, mrphs, arlolra, arthuredelstein, dcf
Keywords: TorBrowserTeam201602 deleted, TorBrowserTeam201603 added -
Replying to teor:
What happens when the user doesn't have permission to write to /Applications (or the directory Tor Browser is in?)
Bad things will happen, which is also true today if TB cannot write inside its .app bundle. I just tried running the new Tor Browser in a folder that I do not have write access to and this message was displayed:
Profile Missing Your Tor Browser profile cannot be loaded. It may be missing or inaccessible.I think we may want to add a new, more accurate error message that is displayed when the browser is unable to start up because the profile directory is not writable/cannot be created.
In the long run we should probably support two modes like Ricochet does: if TorBrowser.app is installed into /Applications, store user data inside ~/Library/Application\ Support like all non-portable apps do; otherwise, use the TorBrowser-Data side-by-side/portable approach.
-
Kathy and I finished the migration code and added it as an additional commit on this branch: https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug13252-01 (we plan to combine the two commits that are labeled with "Bug 13252" after we receive some review feedback).
Since in that commit we added a localized string which is retrieved from Torbutton, we now have a tiny Torbutton commit as well: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug13252-01&id=9be4fe9beb087d6a421bab705b4d1a1d60fd5f90
All of this is now ready for review. Please look at the two commits I just mentioned as well as those mentioned in comment:22. Another approach is to look at the bug13252-01 branch in each of the following user/brade repos: tor-browser, torbutton, tor-launcher, and tor-browser-bundle.
Trac:
Status: assigned to needs_review
Keywords: TorBrowserTeam201603 deleted, TorBrowserTeam201603R added -
See #18495 (closed) for a child ticket related to FTE.
-
Okay, let's start with the easiest one: the Torbutton patch looks good to me. :)
Comments to the tor-browser-bundle changes:
-
It seems to me we can get rid of creating
Cachesaltogether without the need for treating that step specially in the non-signed bundles? I tested it a bit andCachesseems to get created anyway. -
Could you add the FTE ticket number in the FTE TODO comment?
-
The second
rm -rf $SKELETON_TMPseems wrong to me as we are still in the directory we want to delete. And we already delete it before we begin with our skeleton preparation. Maybe a copy/paste fail? -
Do we have some alternative for handling the bookmarks file? Putting it into the
omni.jafile seems a bit weird to me.
-
-
Replying to gk:
Comments to the tor-browser-bundle changes:
- It seems to me we can get rid of creating
Cachesaltogether without the need for treating that step specially in the non-signed bundles? I tested it a bit andCachesseems to get created anyway.
Yes, you are correct that we can just let the browser create it when it is opened for the first time. Do you want us to make that change (skip creation of the Caches directory) for all platforms at this time or just for Mac OS?
- Could you add the FTE ticket number in the FTE TODO comment?
Sure; good idea.
- The second
rm -rf $SKELETON_TMPseems wrong to me as we are still in the directory we want to delete. And we already delete it before we begin with our skeleton preparation. Maybe a copy/paste fail?
I think it should just be moved after the cd -, not removed. The idea is to clean up the directory now that its contents have been captured inside inputs/mac-skeleton.zip.
- Do we have some alternative for handling the bookmarks file? Putting it into the
omni.jafile seems a bit weird to me.
When Firefox creates a new profile, it copies the default bookmarks from defaults/profile/bookmarks.html within browser/omni.ja. In other words, we are using the standard mechanism that Firefox uses to get our bookmarks into new profiles. We could have replaced browser/locales/generic/profile/bookmarks.html.in with our own content inside the tor-browser repo (via a Firefox patch) but since (1) our bookmarks are already part of tor-browser-bundle and (2) we already touch browser/omni.ja to modify the 000-tor-browser.js, we thought it best to handle the bookmarks in a similar way.
Trac:
Reviewer: N/A to N/A - It seems to me we can get rid of creating
-
Replying to mcs:
Replying to gk:
Comments to the tor-browser-bundle changes:
- It seems to me we can get rid of creating
Cachesaltogether without the need for treating that step specially in the non-signed bundles? I tested it a bit andCachesseems to get created anyway.
Yes, you are correct that we can just let the browser create it when it is opened for the first time. Do you want us to make that change (skip creation of the Caches directory) for all platforms at this time or just for Mac OS?
I think we can get rid of it for all platforms.
[snip]
- The second
rm -rf $SKELETON_TMPseems wrong to me as we are still in the directory we want to delete. And we already delete it before we begin with our skeleton preparation. Maybe a copy/paste fail?
I think it should just be moved after the cd -, not removed. The idea is to clean up the directory now that its contents have been captured inside inputs/mac-skeleton.zip.
Sounds good. But what is the purpose of the first
rm -rf $SKELETON_TMPthen? I thought that one was there for making sure everything is in a clean state before starting with the skeletion extraction? Do we really need a clean-up twice in that code snippet?- Do we have some alternative for handling the bookmarks file? Putting it into the
omni.jafile seems a bit weird to me.
When Firefox creates a new profile, it copies the default bookmarks from defaults/profile/bookmarks.html within browser/omni.ja. In other words, we are using the standard mechanism that Firefox uses to get our bookmarks into new profiles. We could have replaced browser/locales/generic/profile/bookmarks.html.in with our own content inside the tor-browser repo (via a Firefox patch) but since (1) our bookmarks are already part of tor-browser-bundle and (2) we already touch browser/omni.ja to modify the 000-tor-browser.js, we thought it best to handle the bookmarks in a similar way.
Makes sense to me, thanks.
- It seems to me we can get rid of creating
-
The tor-launcher changes look good to me. Some minor things:
-
regarding the changes in the prefs.js: s/tor_path pref./tor_path/ like you did for the other paths?
-
I wonder whether
// In FF21+, CurProcD is the "browser" directory that is next to // the firefox binary, e.g., <TorFileBaseDir>/Browser/browsershould be in the Tor Browser code path (as it basically was before). I was confused to read that comment and then the code continued to be concerned with Thunderbird/Instantbird
- What is the reason for treating
torrc-defaultsdifferently here, i.e. moving that one out of the data dir?
-
-
Replying to gk:
Replying to mcs:
Replying to gk:
Comments to the tor-browser-bundle changes:
- It seems to me we can get rid of creating
Cachesaltogether without the need for treating that step specially in the non-signed bundles? I tested it a bit andCachesseems to get created anyway.
Yes, you are correct that we can just let the browser create it when it is opened for the first time. Do you want us to make that change (skip creation of the Caches directory) for all platforms at this time or just for Mac OS?
I think we can get rid of it for all platforms.
Ah, sorry. I think I meant the opposite. Let's not touch the other bundle descriptors in this bug. We can do this later in the respective bugs for Linux and Windows or separately.
- It seems to me we can get rid of creating
-
Replying to gk:
... Sounds good. But what is the purpose of the first
rm -rf $SKELETON_TMPthen? I thought that one was there for making sure everything is in a clean state before starting with the skeletion extraction? Do we really need a clean-up twice in that code snippet?We can remove the second one.
-
Replying to gk:
The tor-launcher changes look good to me. Some minor things:
- regarding the changes in the prefs.js: s/tor_path pref./tor_path/ like you did for the other paths?
Sure. The comments are probably easier to read that way.
- I wonder whether {{{ // In FF21+, CurProcD is the "browser" directory that is next to // the firefox binary, e.g., /Browser/browser }}} should be in the Tor Browser code path (as it basically was before). I was confused to read that comment and then the code continued to be concerned with Thunderbird/Instantbird
You are right. We will move or remove that comment. We are already proposing that we move the check for FF >= 21: if (TorLauncherUtil.isAppVersionAtLeast("21.0"))
- What is the reason for treating
torrc-defaultsdifferently here, i.e. moving that one out of the data dir?
The updater will not be able to touch anything inside TorBrowser-Data (at least that is our current plan). Therefore we need to put torrc-defaults somewhere inside the app bundle so it can be updated. Also, we do not really want users to edit torrc-defaults.
-
Replying to mcs:
Replying to gk:
- What is the reason for treating
torrc-defaultsdifferently here, i.e. moving that one out of the data dir?
The updater will not be able to touch anything inside TorBrowser-Data (at least that is our current plan). Therefore we need to put torrc-defaults somewhere inside the app bundle so it can be updated. Also, we do not really want users to edit torrc-defaults.
Sounds good to me.
- What is the reason for treating
-
Replying to gk:
Commits 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e, 4ea6a818614ec50a62e1bf683baed931d33586ff and f5f6dd2b7d6358343917dba64f722896aa6b79a3 in your tor-browser branch look good to me.
Thank you for all the reviews! I think the only remaining patch is b03f511d38631243fec0e6c5427d9a50e602a762 (also on our tor-browser branch). After you have a chance to review that one, Kathy and I will create new tor-launcher, tor-browser-bundle, and tor-browser patches. We will take your feedback into account and probably combine 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e and b03f511d38631243fec0e6c5427d9a50e602a762 into one patch while we are at it.
-
Replying to mcs:
Replying to gk:
Commits 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e, 4ea6a818614ec50a62e1bf683baed931d33586ff and f5f6dd2b7d6358343917dba64f722896aa6b79a3 in your tor-browser branch look good to me.
Thank you for all the reviews! I think the only remaining patch is b03f511d38631243fec0e6c5427d9a50e602a762 (also on our tor-browser branch).
Yes. I just wanted to indicate to you where I was in my review process and that I probably won't get to that commit anymore on Friday. :)
After you have a chance to review that one, Kathy and I will create new tor-launcher, tor-browser-bundle, and tor-browser patches. We will take your feedback into account and probably combine 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e and b03f511d38631243fec0e6c5427d9a50e602a762 into one patch while we are at it.
Sounds good to me. Re the missing commit it looks good to me. I'd like to understand better the following, though:
+ // Display an error alert and continue startup. Since XPCOM was + // initialized in a limited way inside ProfileErrorDialog() and + // because it cannot be reinitialized, use LaunchChild() to start + // the browser.What exactly is happening in this case? What is the user experiencing? I assume no IP leakage? But what else?
arthuredelstein: Could you please have a look at the C++ bits as well (especially commit b03f511d38631243fec0e6c5427d9a50e602a762 but commits 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e, 4ea6a818614ec50a62e1bf683baed931d33586ff as well)
mcs, brade: Thanks for all this, nice job. I plan to test the profile migration stuff a bit (that's the remaining testing bit I have on my ToDo list) this week. The
needs_revisionis for the pieces I found so far which are mentioned in previous comments.Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201603R deleted, TorBrowserTeam201603 added -
Replying to gk:
[snip] Re the missing commit it looks good to me. I'd like to understand better the following, though:
+ // Display an error alert and continue startup. Since XPCOM was + // initialized in a limited way inside ProfileErrorDialog() and + // because it cannot be reinitialized, use LaunchChild() to start + // the browser. }}} What exactly is happening in this case? What is the user experiencing? I assume no IP leakage? But what else?If this code path is followed (because migration of the old/existing profile failed), the user will see an error alert that looks like this: {{{ Tor Browser Profile Problem
Migration of your existing Tor Browser profile failed. New settings will be used.
After they click OK, the browser will appear to continue to start up, a new profile will be created, and they will see the language prompt or network settings wizard. To the user, it will be as if they downloaded a new copy of Tor Browser, although if they poke inside TorBrowser.app they can find an old TorBrowser/ directory that contains their old profile, their old Tor data directory, and their old UpdateInfo. Above I said "**appear** to continue to start up" because firefox is actually re-exec'd inside LaunchChild(). Doing so ensures a clean startup. This same technique is used by Mozilla whenever a dialog is displayed early on, e.g., there is a LaunchChild() call at the very end of ShowProfileManager() (profile picker dialog). -
Trac:
signit.sh -
We pushed revised patches for tor-browser-bundle and tor-launcher (taking Georg's comments into account):
https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug13252-02&id=b87c52d313977a531703e79711824c3ef1f92f17 https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13252-02&id=64d3139e72587372223d0795ca715991d7423adb
We will wait for Arthur's comments on the tor-browser patches.
I read through each of the three tor-browser patches carefully and I didn't find any problems, although I am pretty unfamiliar with this part of the codebase so I may not understand all of the ramifications. Very nice work! One question I have for patch 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e:
--- a/toolkit/mozapps/extensions/AddonManager.jsm +++ b/toolkit/mozapps/extensions/AddonManager.jsm @@ -42,6 +42,11 @@ const PREF_MATCH_OS_LOCALE = "intl.locale.matchOS"; const PREF_SELECTED_LOCALE = "general.useragent.locale"; const UNKNOWN_XPCOM_ABI = "unknownABI"; +#ifdef TOR_BROWSER_VERSION +#expand const TOR_BROWSER_VERSION = __TOR_BROWSER_VERSION__; +const PREF_EM_LAST_TORBROWSER_VERSION = "extensions.lastTorBrowserVersion"; +#endif + const UPDATE_REQUEST_VERSION = 2; const CATEGORY_UPDATE_PARAMS = "extension-update-params"; @@ -866,6 +871,30 @@ var AddonManagerInternal = { this.validateBlocklist(); } +#ifdef TOR_BROWSER_VERSION + // To ensure that extension override prefs are reinstalled into the + // user's profile after each update, set appChanged = true if the + // Mozilla app version has not changed but the Tor Browser version + // has changed. + let tbChanged = undefined; + try { + tbChanged = TOR_BROWSER_VERSION != + Services.prefs.getCharPref(PREF_EM_LAST_TORBROWSER_VERSION); + } + catch (e) { } + if (tbChanged !== false) { + // Because PREF_EM_LAST_TORBROWSER_VERSION was not present in older + // versions of Tor Browser, an app change is indicated when tbChanged + // is undefined or true. + if (appChanged === false) { + appChanged = true; + } + + Services.prefs.setCharPref(PREF_EM_LAST_TORBROWSER_VERSION, + TOR_BROWSER_VERSION); + } +#endifIs it true here that the TOR_BROWSER_VERSION preprocessor directive already contains quotation marks? I noticed in some other places in a previous patch Kathy and Mark used
+#ifdef TOR_BROWSER_VERSION +# Add double-quotes back on (stripped by JarMaker.py). +#expand const TOR_BROWSER_VERSION = "__TOR_BROWSER_VERSION__"; +#endifbut perhaps the quotes are not stripped on AddonManager.jsm?
One other observation: in b03f511d38631243fec0e6c5427d9a50e602a762 it would be nice to have a doc comment at the head of the
migrateOneTorBrowserDataDirfunction describing what it does. I also have difficulty parsing the comment inside that function:+ // The destination directory exists, which is expected in the case of + // migration of the browser profile. Set the new directory aside and set + // tmpDir to point to the new, temporary location.Could you make it explicit why an existing destination directory is expected in migrating the browser profile? And, should the next sentence be something like
Set the old directory aside and set tmpDir to point to its new temporary location? (Sorry if I'm misunderstanding.)Trac:
Sponsor: N/A to None-
Replying to mcs:
We pushed revised patches for tor-browser-bundle and tor-launcher (taking Georg's comments into account):
https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug13252-02&id=b87c52d313977a531703e79711824c3ef1f92f17 https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13252-02&id=64d3139e72587372223d0795ca715991d7423adb
Looks good to me.
-
I tested the update (a full one, not an incremental one) as good as I could and it works as far as I can see. Nice!
One thing I noticed is that we might want to have a patch for not showing FTE as a PT option for OS X users in case we aren't able to get #18495 (closed) fixed until the alpha with the code signing gets out of the door.
-
Replying to arthuredelstein:
I read through each of the three tor-browser patches carefully and I didn't find any problems, although I am pretty unfamiliar with this part of the codebase so I may not understand all of the ramifications. Very nice work!
Thanks!
One question I have for patch 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e: [snip] Is it true here that the TOR_BROWSER_VERSION preprocessor directive already contains quotation marks? I noticed in some other places in a previous patch Kathy and Mark used
+#ifdef TOR_BROWSER_VERSION +# Add double-quotes back on (stripped by JarMaker.py). +#expand const TOR_BROWSER_VERSION = "__TOR_BROWSER_VERSION__"; +#endif }}} but perhaps the quotes are not stripped on AddonManager.jsm?Yes, that is the case (strange as it is). I checked both toolkit/mozapps/extensions/AddonManager.jsm and toolkit/mozapps/update/content/updates.js (which has a code snippet like you showed above) in a built copy of the browser and the quotes are correct in the processed JavaScript files. I think the difference is that a jar.mn file is used to package update.js but not AddonManager.jsm. But I admit that I do not understand all of the quirks of Mozilla's build system.
One other observation: in b03f511d38631243fec0e6c5427d9a50e602a762 it would be nice to have a doc comment at the head of the
migrateOneTorBrowserDataDirfunction describing what it does.OK. We will add a comment. We tried to write a descriptive comment for migrateInAppTorBrowserProfile() but later we pulled some common code out into migrateOneTorBrowserDataDir() but neglected to document that second function.
I also have difficulty parsing the comment inside that function: {{{
- // The destination directory exists, which is expected in the case of
- // migration of the browser profile. Set the new directory aside and set
- // tmpDir to point to the new, temporary location.
}}}
Could you make it explicit why an existing destination directory is expected in migrating the browser profile? And, should the next sentence be something like
Set the old directory aside and set tmpDir to point to its new temporary location? (Sorry if I'm misunderstanding.)
Sorry for the confusing comment. Here is an attempt at clearer wording: {{{ // The destination directory exists. When we are migrating an old Tor Browser // profile, we expect this to be the case because we first allow the standard // Mozilla startup code to create a new profile as usual, and then later (here) // we set aside that profile directory and replace it with the old Tor Browser // profile that we need to migrate. For now, move the Mozilla profile directory // aside and set tmpDir to point to its new, temporary location in case migration // fails and we need to restore the profile that was created by the Mozilla code.
We will produce a new tor-browser branch that contains updated patches. Thanks again for your review! -
Replying to gk:
One thing I noticed is that we might want to have a patch for not showing FTE as a PT option for OS X users in case we aren't able to get #18495 (closed) fixed until the alpha with the code signing gets out of the door.
Good catch. We revised our tor-browser-bundle patch to includes removal of the FTE-related configuration:
-
Replying to mcs:
We will produce a new tor-browser branch that contains updated patches.
This is done now, on a new branch named bug13252-02: https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug13252-02
There are 3 commits there for this ticket: a9354c824b2435054c3d8d853606a2c7f8622f0b, 38e31a62d48dd8ea8bee02673b7050338fc1c22a, and c1b7d7e2f51f89ab19018e99fe62f8654d3cecb8.
We combined the two #13252 (moved) patches and fixed up the comments as we said we would in #comment:44.
Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam201603 deleted, TorBrowserTeam201603R added
