[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #31010 [Applications/Tor Browser]: Rebase Tor Browser mobile/ patches for Firefox ESR 68



#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs