[tor-bugs] #26401 [Applications/Tor Browser]: Rebase Orfox patches onto Tor Browser 8.0 for TBA

[Tor Bug Tracker & Wiki]

#26401: Rebase Orfox patches onto Tor Browser 8.0 for TBA
-----------------------------------------------+---------------------------
 Reporter:  sysrqb                             |          Owner:  tbb-team
     Type:  task                               |         Status:
                                               |  needs_review
 Priority:  Very High                          |      Milestone:
Component:  Applications/Tor Browser           |        Version:
 Severity:  Normal                             |     Resolution:
 Keywords:  tbb-mobile, TorBrowserTeam201807R  |  Actual Points:
Parent ID:  #26531                             |         Points:
 Reviewer:                                     |        Sponsor:
-----------------------------------------------+---------------------------

Comment (by gk):

 Replying to [comment:20 sysrqb]:
 > Replying to [comment:17 gk]:

 [snip]

 > > I guess the comments before the *strip options are essentially
 enabling stripping? if so, we should be explicit about it an use
 `--enable-strip` like on other platforms as well
 > >
 >
 > The defaults are effectively `--disable-strip` and `--enable-install-
 strip` but we should test these options and understand what improves and
 what fails when we enable/disable them. I'm still not sure.

 Well, that makes it so that we build with symbols but then strip them
 during the packaging step (that can be confusing alone in case you want to
 get debug symbols after packaging up everything). It seems to be cleaner
 to me to indicate upfront in the .mozconfig file that we don't have
 symbols available by specifying `--enable-strip`.

 [snip]

 > > commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why
 do we need the duplicated entries we already have in confvars.sh?)
 >
 > I added those in torbrowser.configure because they look like configure
 flags but they are environment variables. They are still in
 torbrowser.configure only for documentation purposes. I can delete them if
 it's confusing or you think they aren't helpful.

 I don't feel that strongly about it. Let's keep the commit as-is if you
 want.

 [snip]

 > >
 > > Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we
 don't want to point our users to the aus5 URL. Maybe I am missing
 something here.
 >
 > Hm! Those `//` were only supposed to be visual and not affect the
 inclusion. I thought the preprocessor preserves and enforces the ifdef
 when it scans the file - but I see this did not happen.
 `app.update.url.android` is still set. I'll look into this more.

 I don't think that's the case for the .js file. At any rate, I would
 follow the style Mozilla has in this file which is plain `#ifdef`'s.

 [snip]

 > >
 > > commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What
 is the threat here?)
 >
 > I think this provides a safe default. When this is enabled, the user is
 provided an icon in the awesome bar for scanning a QRCode that
 (presumably) contains a URL the user wants to visit. When the icon is
 pressed, another app is opened (if it is installed) where a picture is
 taken and it is scanned for a QRCode and then decoded. I believe the
 scanning/decoding is performed on a remote server. Disabling this by
 default protects against accidentally revealing their location to third
 parties. The user could tap the icon by mistake and launch the third party
 app, and we don't have any control over what that app does or what
 information that app sends.

 Good point. I was not aware that the QRCode treatment is actually done
 remotely, so, yes, let's keep the commit.

 [snip]

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