[tor-bugs] #13252 [Tor Browser]: Tor Browser on OS X should not store data into the application bundle

[Tor Bug Tracker & Wiki]

#13252: Tor Browser on OS X should not store data into the application bundle
----------------------------------+--------------------------------
 Reporter:  torosx                |          Owner:  mcs
     Type:  defect                |         Status:  needs_revision
 Priority:  Medium                |      Milestone:
Component:  Tor Browser           |        Version:
 Severity:  Normal                |     Resolution:
 Keywords:  TorBrowserTeam201603  |  Actual Points:
Parent ID:  #6540                 |         Points:
 Reviewer:                        |        Sponsor:  None
----------------------------------+--------------------------------

Comment (by mcs):

 Replying to [comment:41 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
 `migrateOneTorBrowserDataDir` function 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!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/13252#comment:44>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online