Opened 4 months ago

Last modified 2 days ago

#31010 needs_review defect

Rebase Tor Browser mobile/ patches for Firefox ESR 68

Reported by: sysrqb Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-9.0-must-alpha, TorBrowserTeam201910R
Cc: mcs, brade, acat Actual Points:
Parent ID: #30429 Points:
Reviewer: Sponsor:

Description

This is the Android-specific patches for #30429.

Child Tickets

Attachments (1)

0001-Bug-13252-Do-not-store-data-in-the-app-bundle.patch (22.1 KB) - added by mcs 2 months ago.
ESR68 rebased patch for #13252

Download all attachments as: .zip

Change History (39)

comment:1 Changed 4 months ago by sysrqb

Copying from the parent ticket:

Replying to acat:

43582f5809cf Bug 29859: Disable HLS support for now                                                                                
34e52459b150 Bug 29982 - Force single-pane UI on Tor Preferences                                                                                
6cba968b5a7e Bug 30136: Use 'Tor Browser' as brand name on mobile, too
321bba05d203 Bug 30239 - Render Fragments after crash                                        
22e72c1e7716 Bug 30214 - Kill background thread when Activity is null
75dcaffedcee Bug 30086 - Prevent Sync-related crashes on Android
545db499b0c7 Bug 28622: Update Tor Browser icon for mobile                                   
b563b207c38c Bug 29238 - Prevent crash on Android after update
323ebe2ca2d4 Bug 28329 - Part 4. Add new Tor Bootstrapping and configuration screens         
79c9b8acba0b Bug 28329 - Part 3. Remove OrbotActivity dependency                             
dd3f22090e52 Bug 28329 - Part 2. Implement checking if the Tor service is running            
fe784a8284e5 Bug 28329 - Part 1. Add new Tor resources
a195c7180350 Bug 28507 - Don't call Push and Sync services during Sanitize
f6f8859b3891 Bug 26690: Port padlock states for .onion services to mobile
722afbb6c5c6 Bug 28051 - Use Orbot's notification-builder wrapper class                      
8436a9443fde Bug 28051 - Stop the background service when we're quitting                     
464c03696814 Bug 28051 - Use our Orbot for proxying our connections                          
a0b886f177d8 Bug 28051 - Launch Orbot if it isn't running in the background                  
a5e24cd02c8e Bug 28051 - Integrate Orbot and add dependencies
39316ccaedbb Bug 28507: Implement fallback to delete private data in the browser startup     
b0b54ec7c7a0 Bug 28507: Add prefs that allow the browser to delete browsing history by defa..
894c64799812 Bug 28507: Parse a set of strings in Android Set Preferences
e860dd77665d Bug 27125 - Move localized Tor Browser for Android strings into separate file
06f9c75d7725 Bug 27111: Configure tor browser for mobile to load about:tor
28a2ea997ba0 Bug 28125 - Prevent non-Necko network connections
878c9536c888 Bug 27472 - Export MOZILLA_OFFICIAL during Android build                        
826e0934adb4 Bug 27400 - Target Android API 26
cb63b9e84081 Bug 27473 - Change app name on Android for Alpha version
6177eef07093 Bug 25696 - Design of alpha onboarding for Tor Browser for Android
8e272a5204e2 Bug 25696 - Implement alpha onboarding for Tor Browser for Android 
2c08bd6d5df5 Bug 24796 - Comment out excess permissions from GeckoView
1bd41f99acd1 Bug 26825 - Delete RECEIVE_BOOT_COMPLETED permission
9023dea11376 Bug 26826 - Disable tab queue and delete SYSTEM_ALERT_WINDOW permission         
350766eecb63 Bug 25906 - Imply false both Adjust and Leanplum configure options              
9312fdf1ac53 Bug 27016 - Create proxy connection during image download
bff13bd887c3 Bug 26528 - Don't allow Fennec to use UpdateService when installed through the..
9b218c05cecc Orfox: hook up default panic trigger to "quit and clear"                        
80b18b10f092 Orfox: quit button added                                                        
f1c13fc12685 Orfox: disable screenshots and prevent page from being in "recent apps"         
e96b86f7a6f6 Bug 25741 - TBA: Disable GeckoNetworkManager                                    
ffcc8cefd762 Bug 25741 - TBA: Adjust the User Agent String so it doesn't leak Android version
0e4ae4cbd11a Bug 25741 - TBA: top sites changed, used bookmarks icon temporarily.            
6e806c3cb9ed Orfox: Centralized proxy applied to AbstractCommunicator and BaseResources.     
433766efba25 Orfox: add BroadcastReceiver to receive Tor status from Orbot                   
41b333e3a9d3 Orfox: NetCipher enabled, checks if orbot is installed                          
d073b92b20ba Bug 25741 - TBA: Neuter Firefox Accounts                                        
f96dc4c9ae4d Bug 25741 - TBA: Only include GCM permissions if we want them                   
e8f22d0b795e Bug 25741 - TBA: Only include Firefox Account permissions if we want them       
6299c89034d3 Bug 25741 - TBA: Always Quit, do not restore the last session                   
a66bd47fd601 Bug 25741 - TBA: Disable all data reporting by default                          
eebbebcb7010 Bug 25741 - TBA: Clear state when the app exits, by default                     
0820ddf07093 Bug 25741 - TBA: Do not import bookmarks and history from native browser by de..
8839bac75801 Bug 25741 - TBA: Do not save browsing history by default                        
3531fb88ab73 Bug 25741 - TBA: Move CAMERA permission within MOZ_WEBRTC                       
c910edef7833 Bug 25741 - TBA: Conditionally require *_LOCATION permissions                   
d2fa5a6928bf Bug 25741 - TBA: Conditionally require WIFI and NETWORK permissions             
f90df9c52d2d Bug 25741 - TBA: Disable QR Code reader by default                              
b6584da608bf Bug 25741 - TBA: Disable the microphone by default                              
92959f9de719 Bug 25741 - TBA: Disable telemetry and experiments                              
e4ea4bf5fbfd Bug 25741 - TBA: Remove sync option from preferences                            
7122131c88d9 Bug 25741 - TBA: Add mobile-override of 000-tor-browser prefs                   
39979b254f2c Bug 25741 - TBA: Add default configure options in dedicated file                
d9b6b1116a67 Bug 25741 - TBA: Do not register Stumbler listener at start up                  
765b0fb6cdee Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION                     
de5d9f3c4f83 Bug 25741 - TBA: Disable features at compile-time                               
2e75b845a93f Bug 25741 - TBA: Add mozconfig for Android and pertinent branding files.        
b4aef2103fef Bug 25741 - TBA: Move GCM Push prefs within preprocessor guard                  
db59d0ef5a5b Bug 25741 - TBA: Exclude unwanted Stumbler tests

comment:2 Changed 4 months ago by sysrqb

I successfully built a branch based on acat's 30429+1 branch. The branch with the Android patches is in my repo as branch acat30429+1_tor-browser_android_68esr_19. This is very much a work in progress, but I'll rebase and squash in different branches.

This branch contains many squashed commits.

comment:3 Changed 3 months ago by gk

I've not checked the rebased commits yet but while looking at #28672 I realized we still have the tor-browser-build patch for #29575, which I think we should move into tor-browser. Could you move android-dependencies.patch over to that repo and integrate it into your rebased patches?

comment:4 Changed 3 months ago by gk

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201906R removed

No reviews in June 2019 anymore, moving them.

comment:5 Changed 3 months ago by gk

Keywords: tbb-9.0-must-nightly added

Starting with 9.0 keywords

comment:6 Changed 2 months ago by gk

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201907R removed

No July any longer.

comment:7 Changed 2 months ago by sysrqb

I'm hitting a problem at startup, and I haven't found the cause yet.

08-06 21:08:44.062 4117-4141/org.torproject.torbrowser D/GeckoThread: State changed to LIBS_READY
08-06 21:08:44.062 4117-4141/org.torproject.torbrowser W/GeckoThread: zerdatime 25373549 - runGecko
08-06 21:08:44.062 4117-4141/org.torproject.torbrowser D/GeckoProfile: Found profile dir.
08-06 21:08:44.096 4117-4141/org.torproject.torbrowser I/Gecko:DumpUtils: Fifo watcher disabled via pref.
08-06 21:08:44.163 4117-4141/org.torproject.torbrowser D/GeckoThread: State changed to JNI_READY
08-06 21:08:45.266 4117-4141/org.torproject.torbrowser I/fennec: XRE_main returned 1
08-06 21:08:45.266 4117-4141/org.torproject.torbrowser D/GeckoThread: State changed to EXITED

Because it is setting JNI_READY, it seems like this is failing after XPCom initialization.

comment:8 Changed 2 months ago by sysrqb

But in the meantime, the current branch I'm using is acat30429+5_tor-browser_android_68esr_39 in my public repo. There are a few fixup commits on that branch, but I think most of the commits are stable.

comment:9 Changed 2 months ago by sysrqb

Cc: mcs brade added

Bisecting leads to this commit (commit d03d360bd773c8d8a97ef1f69e565afcc19796e5 on acat's 30429+5 branch). This'll require some digging. mcs/brade, if you have any suggestions about where I should look, they'll be very much appreciated.

Specifically, I'm hitting this result:

08-06 21:08:45.266 4117-4141/org.torproject.torbrowser I/fennec: XRE_main returned 1

I'm having trouble setting breakpoints in the code, so I'm working through the code manually right now. I think this failure is triggered after mScopedXPCOM->Initialize() because I'm getting the log message:

08-06 21:08:44.163 4117-4141/org.torproject.torbrowser D/GeckoThread: State changed to JNI_READY

and I believe that is set at the end of the Initialize() call (after a few layers of indirection, assuming I traced this correctly).

comment:10 in reply to:  9 ; Changed 2 months ago by mcs

Replying to sysrqb:

Bisecting leads to this commit (commit d03d360bd773c8d8a97ef1f69e565afcc19796e5 on acat's 30429+5 branch). This'll require some digging. mcs/brade, if you have any suggestions about where I should look, they'll be very much appreciated.
...

Without knowing a lot about the Android-specific code, my guess is that the #9173 patch is just not correct by itself for Android. What is missing from acat's 30429+5 branch is a patch for #13252, which Kathy and I have been working on along with the updater patches. The #13252 patch adds the "side-by-side" TorBrowser-Data magic for macOS and also changes how the app data directory is determined on all platforms. As of yesterday, we have a "hot off the press" patch for #13252 which we plan to get to acat soon. I will attach a copy to this ticket so you can try it now.

One caveat: the ESR68 #13252 patch is intended to be inserted into the commit stream immediately after the #11641 patch (69c8b59f9cdc792ee533187723286fdd75dbabd0 on acat's 30429+5 branch). You may get conflicts if you tack it on the end.

Changed 2 months ago by mcs

ESR68 rebased patch for #13252

comment:11 Changed 2 months ago by acat

Some comments on acat30429+5_tor-browser_android_68esr_39:

d0386613a4884393ff3324e43f8d06d30f9d6564 -

this is not a patch in the original branch, does the build fail without this?

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?

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?

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.

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?

  ++#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?

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.

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?

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

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.

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

comment:12 Changed 2 months ago by acat

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201908R removed
Status: newneeds_revision

comment:13 in reply to:  10 Changed 8 weeks ago by sysrqb

Replying to mcs:

Replying to sysrqb:

Bisecting leads to this commit (commit d03d360bd773c8d8a97ef1f69e565afcc19796e5 on acat's 30429+5 branch). This'll require some digging. mcs/brade, if you have any suggestions about where I should look, they'll be very much appreciated.
...

Without knowing a lot about the Android-specific code, my guess is that the #9173 patch is just not correct by itself for Android. What is missing from acat's 30429+5 branch is a patch for #13252, which Kathy and I have been working on along with the updater patches. The #13252 patch adds the "side-by-side" TorBrowser-Data magic for macOS and also changes how the app data directory is determined on all platforms. As of yesterday, we have a "hot off the press" patch for #13252 which we plan to get to acat soon. I will attach a copy to this ticket so you can try it now.

One caveat: the ESR68 #13252 patch is intended to be inserted into the commit stream immediately after the #11641 patch (69c8b59f9cdc792ee533187723286fdd75dbabd0 on acat's 30429+5 branch). You may get conflicts if you tack it on the end.

Thanks, that patch did help.

comment:14 in reply to:  11 ; Changed 7 weeks ago by sysrqb

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

comment:15 Changed 7 weeks ago by sysrqb

Status: needs_revisionneeds_review

I'll cherry-pick all of these commits onto a branch based on tor-browser-68.0esr-9.0-3 for the next round of reviews, but this branch is based on acat's 30429+5 branch, like the previous one for consistency.

The new branch for review is: acat30429+5_tor-browser_android_68esr_54.

comment:16 Changed 7 weeks ago by gk

Cc: acat added
Keywords: TorBrowserTeam201908R added; TorBrowserTeam201908 removed

comment:17 in reply to:  15 Changed 7 weeks ago by sysrqb

Replying to sysrqb:

The new branch for review is: acat30429+5_tor-browser_android_68esr_54.

whoops, that should be acat30429+5_tor-browser_android_68esr_53

comment:18 in reply to:  14 ; Changed 7 weeks ago by sysrqb

Replying to sysrqb:

Replying to acat:

Some comments on acat30429+5_tor-browser_android_68esr_39:

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.

It seems like you're correct, we don't need this. However, I don't know if we should keep this such that the code is more readable/easier to understand? Alternatively, I can delete the resolve() line and modify the comment.

comment:19 Changed 7 weeks ago by gk

One thing I noticed while testing is that the about:tor page is not showing up for me anymore. It just stays blank. That's on an older S5 mini device (armv7) That's with pointing my firefox project to 2dbb7aefeaeb0499ac69c67df470e2ed6df8cb71.

comment:20 in reply to:  18 Changed 6 weeks ago by acat

acat30429+5_tor-browser_android_68esr_53 looks like it addresses the review comments (except the obsolete prefs, which were not reviewed yet according to sysrqb).

Replying to sysrqb:

Replying to sysrqb:

Replying to acat:

Some comments on acat30429+5_tor-browser_android_68esr_39:

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.

It seems like you're correct, we don't need this. However, I don't know if we should keep this such that the code is more readable/easier to understand? Alternatively, I can delete the resolve() line and modify the comment.

I'm not sure it's more readable, but that's subjective I guess :) I read is as a noop: it's a statement without side-effects and the value is not used nor returned.

comment:21 Changed 6 weeks ago by sysrqb

Okay, great! I rebased the commits (except the last one) on top of tor-browser-68.1.0esr-9.0-1 (adb1614b652a5be21e7f025d15a8ea48401eeb02). You can reproduce this with: git cherry-pick acat/30429+5..acat30429+5_tor-browser_android_68esr_54~ (where acat/30429+5 is 677a3a505f714a9379735a189a43349c4097231a).

In any case, after cherry-picking all of the commits, I made a few modifications - including deleting the Promise.resolve(), as it was previously mentioned :)

I have branch bug31010_4. There is an additional change needed in torbutton because addTrustedTab is not defined, so we need to revert the recent change that introduced this for mobile.

diff --git a/chrome/content/torbutton.js b/chrome/content/torbutton.js
index 756c2c7c..c97913bc 100644
--- a/chrome/content/torbutton.js
+++ b/chrome/content/torbutton.js
@@ -1861,9 +1861,10 @@ function showSecurityPreferencesPanel(chromeWindow) {
 
   if (settingsTab === null) {
       // Open up the settings panel in a new tab.
-      tabBrowser.addTrustedTab(SECURITY_PREFERENCES_URI, {
+      tabBrowser.addTab(SECURITY_PREFERENCES_URI, {
           "selected": true,
           "parentId": tabBrowser.selectedTab.id,
+          triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
       });
   } else {
       // Activate an existing settings panel tab.

I pushed this to:
https://github.com/sysrqb/torbutton.git
branch bug31010_00

comment:22 in reply to:  21 Changed 6 weeks ago by sysrqb

Replying to sysrqb:

Okay, great! I rebased the commits (except the last one) on top of tor-browser-68.1.0esr-9.0-1 (adb1614b652a5be21e7f025d15a8ea48401eeb02). You can reproduce this with: git cherry-pick acat/30429+5..acat30429+5_tor-browser_android_68esr_54~ (where acat/30429+5 is 677a3a505f714a9379735a189a43349c4097231a).

In any case, after cherry-picking all of the commits, I made a few modifications - including deleting the Promise.resolve(), as it was previously mentioned :)

I have branch bug31010_4. There is an additional change needed in torbutton because

Okay, let's try branch bug31010_5 instead. The torbutton scripts are included directly in <src> tags, and torbutton_init() is called during BrowserApp.startup() which is called onLoad from browser.xul. I think this should have the same affect as what happens on desktop.

comment:23 Changed 6 weeks ago by gk

The fixups look good to me. I've reverted the previous patch (commit 805ff051656dc22d11de026e01223352f39e7380) and cherry-picked the respective one from bug31010_5 onto tor-browser-68.1.0esr-9.0-1 (commit fe6da38319ca39a62dc8cfffe615ec0335ba3ddf).

comment:24 Changed 6 weeks ago by gk

Keywords: tbb-9.0-must-alpha added; tbb-9.0-must-nightly removed

Move must-nightly items to must-alpha ones.

comment:25 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201909 added

Moving must-alpha tickets to September.

comment:26 Changed 6 weeks ago by gk

Moving tickets to September

comment:27 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201908R removed

No August anymore.

comment:28 in reply to:  11 Changed 3 weeks ago by sysrqb

Replying to acat:

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?

I pushed bug31010_6 for these last few prefs, plus disabling ServiceWorkers (as mentioned in #15563), and disabling webauthn on Android.

comment:29 Changed 2 weeks ago by acat

I think that's all (although I was not aware of webauthn being enabled on Android 😊).

comment:30 in reply to:  29 ; Changed 2 weeks ago by sysrqb

Replying to acat:

I think that's all (although I was not aware of webauthn being enabled on Android 😊).

Thanks!

I chatted with GeKo and I have a new branch that moves disabling ServiceWorkers into the desktop prefs override file. This will protect us in the case where Mozilla enable ServiceWorkers on Desktop and we forget about this or miss the change.

Branch bug31010_7

comment:31 in reply to:  30 Changed 2 weeks ago by gk

Replying to sysrqb:

Replying to acat:

I think that's all (although I was not aware of webauthn being enabled on Android 😊).

Thanks!

I chatted with GeKo and I have a new branch that moves disabling ServiceWorkers into the desktop prefs override file. This will protect us in the case where Mozilla enable ServiceWorkers on Desktop and we forget about this or miss the change.

Branch bug31010_7

Looks good to me. Cherry-picked both commits onto tor-browser-68.1.0esr-9.0-2 (commit 726047a459acf9d8c26fcfdd72584f0196dd60ce and 8fe3191e67d007cf35969d888453096bd823af76). FWIW, the bug showing Google Play Services involvement in WebAuthn is https://bugzilla.mozilla.org/show_bug.cgi?id=1391438.

comment:32 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201909 removed

comment:33 in reply to:  14 Changed 2 weeks ago by acat

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.

}}}

I think I missed to list the other change from the original patch that was dropped, which is https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/chrome/content/browser.js?h=tor-browser-60.8.0esr-9.0-1&id=260599b2d1da147f420421c217f413f9bc50e23d#n5562.

I also missed the fact that this._lastStatus is not there anymore. I think replacing this._lastStatus by this._lastSecInfo should work in both cases. I suspect this might be causing the issue of the onion icon with the fixup in https://gitweb.torproject.org/user/gk/tor-browser.git/log/?h=30429_test, so I'll try to do a build and check if this fixes it.

comment:34 Changed 13 days ago by pili

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201909R removed

We're now in October, moving September outstanding reviews to October

comment:35 Changed 10 days ago by acat

A fixup for review in: https://github.com/acatarineu/tor-browser/commit/31010. I could verify that this indeed fixes the issue with https://trac.torproject.org/projects/tor/ticket/30429#comment:77.

comment:36 Changed 6 days ago by sysrqb

Thanks for writing the patch! I remember seeing _lastSecInfo replaced _lastStatus, but I don't know why I didn't update the patch (or maybe I updated the patch and then I accidentally reverted it).

In any case, this looks good. I haven't tested it yet.

comment:37 Changed 6 days ago by sysrqb

Status: needs_reviewmerge_ready

lgtm.

comment:38 Changed 2 days ago by gk

Status: merge_readyneeds_review

Looks good and works for me. Cherry-picked to tor-browser-68.1.0esr-9.0-2 (commit 89acb810aa0f622316b43812bb07cb56eb9c9377).

Note: See TracTickets for help on using tickets.