[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
>
Changed.
> 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 ========================================================
MOZ_ARG_ENABLE_BOOL(strip,
[ --enable-strip Enable stripping of libs & executables ],
ENABLE_STRIP=1,
ENABLE_STRIP= )
dnl ========================================================
dnl = Enable stripping of libs & executables when packaging
dnl ========================================================
MOZ_ARG_ENABLE_BOOL(install-strip,
[ --enable-install-strip Enable stripping of libs & executables when
packaging ],
PKG_SKIP_STRIP= ,
PKG_SKIP_STRIP=1)
}}}
{{{
$ grep -e ENABLE_STRIP -e PKG_SKIP_STRIP obj-arm-linux-
androideabi/config.status
'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"
ack.
>
> 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/
>
ack.
> 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
android:name="android.permission.ACCESS_NETWORK_STATE"/>
> #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