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

Tor Bug Tracker & Wiki blackhole at torproject.org
Tue Jul 17 19:36:30 UTC 2018

#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 sysrqb):

 Replying to [comment:17 gk]:
 > I started my review based on `tor-browser-60.1.0esr-8.0-1+26401` which
 is what I stuck to. Here comes the first batch of comments:

 Thanks, no problem.

 > commit 970c95dff0b30eaf0597e43c442b375fe8a68da0 -- not okay
 > "We exclude the all testStumbler*.java files" (one of "the" and "all"
 should be
 > enough :) )
 > Maybe use `''` instead of `""` as all exclude rules are using the former


 > commit 66ecd900c106de3bb84ba9f5aa1cdc59ab28cdd7 -- not okay
 > .mozconfig-android: are we really still using ndk r10e for esr60;

 No, I'll update the comment (Fennec is build using NDK r15c now)

 > 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.

 dnl ========================================================
 dnl = Enable stripping of libs & executables
 dnl ========================================================
 [  --enable-strip          Enable stripping of libs & executables ],

 dnl ========================================================
 dnl = Enable stripping of libs & executables when packaging
 dnl ========================================================
 [  --enable-install-strip  Enable stripping of libs & executables when
 packaging ],

 $ grep -e ENABLE_STRIP -e PKG_SKIP_STRIP obj-arm-linux-
     'ENABLE_STRIP': '',
     'PKG_SKIP_STRIP': '',

 > mozconfig.common.override: why do we have all of those changes here?
 Shouldn't the .mozconfig-android file already take care of them? If there
 are some options missing in the former let's add them there. Looking
 closer at that file it seems this is intended for Mozilla's try infra?
 While I think that's worthwhile I think we should have a separate bug for
 that and thinking about a strategy including all the other platforms we
 support as well.

 Okay, that's fair, I'll factor out these changes.

 > "When using --disable-crashreporter the symbole file" -> "When using
 --disable-crashreporter the symbols file"


 > commit 9d8303c145317d067b130855eb0b17c70c614d76 -- okay (should we file
 a bug in Mozilla's bug tracker for the unused defines?)

 Yes, I'll open a ticket for that.

 > commit ae36b1d20547358c49ee2206af5e28767ea7d48b -- not okay
 > Indentation:
 > {{{
 > +        if (!AppConstants.isTorBrowser()) {
 >          getApplicationContext().sendBroadcast(
 >                  new Intent(INTENT_REGISTER_STUMBLER_LISTENER)
 >          );
 > +        } // !isTorBrowser()
 > }}}

 I did this so the diff is only the new lines instead of the entire
 statement. This seemed easier for reviewing and future rebase. I can
 indent the conditional if that is more readable.

 > 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.

 > commit 97ca08c7c8bc6d58cbeac4838cf2587dc8603050 -- not okay
 > I think we can skip the whole UA override thing as it does not play any
 role anymore once we set the resistfingerprinting pref (which is done for
 mobile as well)

 Okay, I'll delete it.

 > 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.

 > s/URLS/URLs/


 > no need for dom.battery.enabled anymore (see: #22554)

 Okay, I agree. I'll delete that.

 > `network.manage-offline-status` is already set in 000-tor-browser.js
 > `geo.enabled` is already set in 000-tor-browser.js

 I see! Deleted.

 > Do we need `network.tickle-wifi.enabled` given #18799?

 Okay, no, that looks good.

 > I think we should exclude the VR related pref and fix that in the
 desktop pref
 > file instead (#21607).

 I'm okay with this.

 > It's not clear to me why we have some of the prefs set in mobile but not
 desktop (e.g. the clearOnShutdown ones). I guess we could look over it in
 a new ticket to make sure we really only include android specific prefs in
 the new prefs file?

 Yes. I think we should discuss this, in particular I'm not sure if
 autoplay should be enabled or disabled. This may be one example where it
 provides a better user experience if it is disabled on mobile but the
 remains enabled on desktop.

 > 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.

 > commit 908c2f542a82df40f5757f0439a054e8067d8da8 -- probably okay (Is the
 additional, empty line intentional?
 > {{{
 >      <uses-permission
 >  #endif
 > +
 >      <uses-permission android:name="android.permission.INTERNET"/>
 > }}}
 > )

 No, I'll revert that new line.

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

More information about the tbb-bugs mailing list