[tor-bugs] #31010 [Applications/Tor Browser]: Rebase Tor Browser mobile/ patches for Firefox ESR 68
Tor Bug Tracker & Wiki
blackhole at torproject.org
Fri Aug 23 13:34:58 UTC 2019
#31010: Rebase Tor Browser mobile/ patches for Firefox ESR 68
-------------------------------------------------+-------------------------
Reporter: sysrqb | Owner: tbb-
| team
Type: defect | Status:
| needs_revision
Priority: Medium | Milestone:
Component: Applications/Tor Browser | Version:
Severity: Normal | Resolution:
Keywords: tbb-mobile, tbb-9.0-must-nightly, | Actual Points:
TorBrowserTeam201908 |
Parent ID: #30429 | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Comment (by sysrqb):
Replying to [comment:11 acat]:
> Some comments on acat30429+5_tor-browser_android_68esr_39:
Thanks for review!
>
> d0386613a4884393ff3324e43f8d06d30f9d6564 -
> this is not a patch in the original branch, does the build fail
without this?
Yes, the build fails without this, I still must open a bugzilla ticket and
uplift it.
> 546cab59acc827d7cf50ae227c2aa84f01c23c2d - ok
> d37580f4e17b6f3709d6906ef13441e29b61d86e - ok
> 4b1f9425adf546aecb094dc9a501c61e32fe3394 -
> In .mozconfig-android, should we delete instead of commenting out
lines, to be consistent with what we did in desktop .mozconfigs?
>
> +CC="/home/android/.mozbuild/clang/bin/clang"
> +CXX="/home/android/.mozbuild/clang/bin/clang++"
> Shouldn't this be removed?
>
Yes, I'll delete the commented-out lines. CC and CXX must be defined now
(I believe), so I'd like to keep these. I'll try making them more general,
so we can use them for troubleshooting in the future.
> 6041d5df003aed06f8dfe0349f53d2821777331b - ok
> f5c33693baf1d386c8bccedaee7cc13ce218a4b6 - ok
> 3c070da60f5d9b851d8c3d85d14e5dabf108d068 - ok
> 9fa528e6853c0538b8968d5980ab7e0fdbe9be1f - ok
> 1fd0ea742bdb74e99c484cbda24f4942aea2663c -
> - `browser.mirroring.enabled` pref seems not to be used anymore
> - app.update.enabled same (see #29611, but not sure how relevant is
that for Android)
> - In the code now I only see `media.autoplay.enabled.user-gestures-
needed` instead of `media.autoplay.enabled`, do we need to change this?
>
Thanks for looking at these. I haven't reviewed the prefs yet, so it is
likely we can delete some of them, and we probably should add new ones.
> 805a0b25be322d44138bae0d3a862070c9348fea
> {{{
> + // Avoid throwing an error because Ci.nsIPushService isn't
implemented
> + // All other clearing actions should succeed if we arrive
here.
> + Promise.resolve();
> }}}
> Promise.resolve(); is not really doing anything I think.
I don't actually know. I'll try testing this without that line and see if
it still works.
>
> cdf51822421ad608cee5a2733a5eb37928da9484 - ok
> cd35aebe2de754c3fbc221ee4864cd5ffa162b16 - ok
> d8f702013cab81db385316f0c5bbd191574ef9ac - ok
> cab59d99f96bf37aaa016e987cb8729e1d551f1c -
> {{{
> +<!--#endif-->
> <uses-feature
> - android:name="android.hardware.microphone"
> + android:name="android.hardware.audio.low_latency"
> android:required="false"/>
> <uses-feature
> - android:name="android.hardware.camera.any"
> + android:name="android.hardware.microphone"
> android:required="false"/>
> }}}
> These last two are outside of the ifdef, which is not the case in the
original patch, why?
I thought we needed them for a feature (the QRCode reader and voice
dictation), but I looked at them again and you are correct that we don't
need them. I moved them within the ifdef again.
> {{{
> ++#ifdef MOZ_ANDROID_LOCATION
> + <uses-permission
android:name="com.android.browser.permission.READ_HISTORY_BOOKMARKS"/>
> + <uses-permission
android:name="android.permission.FOREGROUND_SERVICE"/>
> ++#endif
> }}}
> This was originally in `Bug 25741 - TBA: Only include Firefox Account
permissions if we want them`, why was it moved here?
> Besides, FOREGROUND_SERVICE is new, not present in esr60. how do we
know we also need to put it under MOZ_ANDROID_LOCATION?
>
I squashed all of the permissions-related commits into a single commit.
For FOREGROUND_SERVICE, I wasn't sure if we needed it, so I excluded it
for initial testing, but that results in a crash. I moved
FOREGROUND_SERVICE out of the ifdef in a newer branch.
> 17db91c073521555d5d515a11a75348bde44b52a - ok
> 69401455d5e8ce26b69c6c8d033e5645775c157c - ok
> 47e7d5c8bc94039d3b240faa630e3186c649c49e -
> private.data.passwords was removed, do we need to do something to make
sure they are cleared now?
> a7f568a6cd7c078a749a97fc8510e8fd962a46f1 - ok
> 61b94b2e7d92a816742a0babf0de92465f927c97 - ok
> e4f639671ce4b5c5f108e9fa3c841a2445434a19 - ok
> f7d637ab2f80dc54a3da69e6183676d7aaf0821d -
> Regarding the search engines, I guess it's fine to leave as it is for
now.
> But as GeKo commented in #30429, it would be good at some point to
solve #30017 and #30606.
> That could happen as a fixup of the `Omnibox: Add DDG, Startpage,
Disconnect, Youtube...` commit, maybe, and remove the changes from this
one.
Sounds good.
>
> 0c62b14f6dc334ebef796f771f8b2e3bf623b989 - ok
> 8d56195c1bc6e15b5139d05606876da2736928d5 - ok
> eb0f60e48235872c141288e8bf3be44ac7c55a27 - ok
> 42c9bb7856f1f65b208ceb294ad881e9f4dfd66e - ok
> 998e9eb14df9691f9c1ae041ea085474a4379222 - ok
> b3262f7ef872b534b9f9804ab3fe7649a0e0a79e - ok
> f5ce3d9f078c4796f9f6e84e27970c82944c3859 - ok
> 7f42fb42206b5924dd4b769e1c2a3e22fa76eb56 - ok
> 4aaccecd9afeeac49990aaf45bb7e3c9a4c5613e - ok
> 8ee68edc2342c64ba629f40c8564664da261a3c8 -
> Don't we need
`mobile/android/base/java/org/mozilla/gecko/CrashReporterActivity.java`
and `mobile/android/base/java/org/mozilla/gecko/updater/Updater.java` too?
`CrashReporterActivity` is excluded because we disable crash-reporter at
compile-time: https://searchfox.org/mozilla-
esr68/source/mobile/android/app/build.gradle#119
`Updater` is a little trickier, but that should never be used because we
don't define `MOZ_UPDATER` on Android builds.
>
> aa6eecff7e7e7b3197b436d5ffa5694f19c6c050 - ok
> 5f43ed64c8311c80b1125ac34e7c5d251550e759 - ok
>
> b7abcc2f4062fd56869827ecc181e93782ea8127 -
> A couple of changes from the original patch were dropped, why?
> {{{
> + result.isOnionHost = this.isOnionHost();
> + result.hasCert = !!this._lastStatus;
>
> // Don't show identity data for pages with an unknown identity or
if any
> // mixed content is loaded (mixed display content is loaded by
default).
> @@ -5757,7 +5769,7 @@ var IdentityHandler = {
> // hasMatchingOverride does not handle that, so avoid calling it.
> // Updating the tooltip value in those cases isn't critical.
> // FIXME: Fixing bug 646690 would probably makes this check
unnecessary
> - if (this._lastLocation.hostname &&
> + if (this._lastLocation.hostname && iData.cert &&
> }}}
These were due to merge conflict. The first diff did not change, but I did
miss the additional iData.cert check in the second. Good catch, thanks.
}}}
> 8b7e055d36246e6ff8aa66e1aa5554dba676f3cc - ok
> 139207e5efb48b308b1fe37489299aa8d7e9c432 - ok
> 1ab5a30000edd23dd452c77922e4a20c124583ba - ok
> 394b735e8821cd41861b381ee95ff93299bded12 - ok
> feb7ccdd6b03c0c476ce5201833e942e33297db5 - ok
> 23a394180b896579d8254d158d523ee27a448bca - ok
> 9dadbe00e257efa034c3c2323449c7d138b9b427 - ok
> 3330fc0b90144b9fccef43aebdfd9e635ed17ee8 -
> Is this patch ready? I see it as need_revision.
It needs some revisions, but it is currently shipping in alpha, so I think
we should be consistent and rebase this patch onto 68esr, too. I need to
look at that ticket again.
> 8424156c8cb3c39953a1bd8d40cd384e2153c7d8 - ok
>
> These two patches still need to be rebased, right?
> 41b333e3a9d3 Orfox: NetCipher enabled, checks if orbot is installed ??
> 433766efba25 Orfox: add BroadcastReceiver to receive Tor status from
Orbot
Not anymore. Those were original patches we used when Tor Browser used
Orbot instead of a built-in tor. Now we ship our own integrated in the
app, so we don't need those patches anymore.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31010#comment:14>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list