Build Orbot into TBA
While we're waiting for #27609 (moved), we'll try building Orbot directly into TBA (this flows into #27977 (moved)).
Activity
I have this building and working (mostly).
It requires modifying Orbot so it is built as a library (producing an AAR, Android Archive, instead of an APK). Most of the modifications delete application-specific information, such as the
applicationId(libraries aren't allowed anapplicationId:) ).Following the Orbot modifications, building tor-browser now depends on Orbot's dependencies, as well - so Orbot's dependencies are now explicitly included in tor-browser's Gradle build configuration, too.
ProGuard needed some modification when building Orbot, too. Its code minimization algorithm stripped a unused method, but that method is used within tor-browser, so I added an explicit
keepdirective for it.Last, tor-browser depends on an older version of the Android support libraries than Orbot. Unfortunately the newer versions of the support libraries are not backwards-compatible, and Orbot now uses some of the newer features, so we can't easily rollback to the older version. Luckily, we can compile Orbot against the newer version, and we can compile tor-browser against the older version using a Gradle hack.
Trac:
Owner: tbb-team to sysrqb
Status: new to assignedMaybe the easiest solution (short-term) is maintaining a patchset on top of a specific Orbot git tag (instead of forking Orbot and maintaining it). I'll attach a patch on top of
16.0.3-BETA-2-tor-0.3.4.8.I have a branch
28051for tor-browser in my repo. It's not ready for review yet, but it mostly works.The assumption is:
- Build Orbot using the provided patch
- Copy the resulting aar file into
mobile/android/app/- If
orbot/andtor-browser/share the same parent directory, then you can use this shell script for copying the three .aar files fromorbot/intomobile/android/app/
- If
$ for aar in orbotservice/build/outputs/aar/orbotservice-release.aar jsocksAndroid/build/outputs/aar/jsocksAndroid-release.aar app/build/outputs/aar/Orbot-16.0.3-BETA-2-tor-0.3.4.8-fullperm-release.aar; do cp $aar ../tor-browser/mobile/android/app/; done- or, as multiple lines:
$ for aar in orbotservice/build/outputs/aar/orbotservice-release.aar jsocksAndroid/build/outputs/aar/jsocksAndroid-release.aar app/build/outputs/aar/Orbot-16.0.3-BETA-2-tor-0.3.4.8-fullperm-release.aar do cp $aar ../tor-browser/mobile/android/app/ doneWhen the app first starts, it immediately opens Orbot and goes through Orbot's onboarding screens. At the end, the user is shown Orbot's main screen (as usual). If the user presses the phone's back button, then they return to TBA. There isn't currently a mechanism for re-opening Orbot within TBA. Orbot only opens when the app is restarted, and it is the first screen they see.
Note, tor doesn't start, so this doesn't work yet.
Trac:
28051_orbot_v0.patchOkay. I spent a more than a few days troubleshooting a bug in Fennec this patch introduced. There is one more issue that remains and I'm not sure which direction we go with this right now (see ticket:27977#comment:12), I'll paste it here, as well for reference:
Right, this is a problem. We have two choices, and I don't particularly like either of them. Orbot doesn't need API 27 support, API 26 is sufficient. However, we have a problem with the Android Support Library version it uses.
Orbot uses notifications (the message in the drop-down menu at the top) for showing information about Orbot's current state. We can take advantage of this by giving TBA users a way to open TBA's Orbot and configure it. The UX-flow is a little weird, and we can improve on it, but I think the notification is very helpful. However, this is only available beginning with the Android support library version 26.1.0. Orbot compiles and runs without a problem when its compiled against version 26.1.0, so we can make this change easily. However, Fennec uses version 23.4.0. Unfortunately, version 23.4.0 can't create notifications on new versions of Android (Android O added a new "Channel ID" attribute, and setting this is a requirement of all notifications, but there isn't a way to set it when we use an Android Support Library less than 26.1.0). It gets better because between version 23.4.0 and 26.1.0, Android changed the name of some built-in images (Material Design resources), so Fennec doesn't compile against Support library 26.1.0 (without patching Fennec for the name change). With all this being said:
- We can downgrade the version of the support library Orbot depends on, and lose Notifications on Android O (and later version)
- We can upgrade the version Fennec depends on and add a patch that changes the resource names.
The bug I tracked was a result of the way Fennec recovers from Android killing part of the app but not the entire thing. Specifically, but for simplicity, we can think of Android apps as being divided into three parts: the Application, the Activity, and the Service. When Fennec is initialized (either first-run, or the device was rebooted, or the app was force stopped), Fennec's Application is created which instantiates numerous Java classes and keeps the state of the overall app. Then Fennec's Activity is created (the browser part) and that statically loads the Gecko library in addition to creating the browser UI and restoring previous saved state, and creating a new Gecko session with the Chrome and nsWindow and various other parts. Usually, if Android kills Fennec's Activity (because it needs to recover some memory or because the user "swipped" away the app), then the Activity is destroyed as well as the Application state. This means when the app is next run, everything is initialized again. However, now we have Orbot running as a background service, and it isn't destroyed when the Activity is destroyed...this also means the underlying Application isn't destroyed (because the Application encompasses all Activities and Services). This resulted in following a new code path within Fennec when the App was next launched. Now, instead of re-initializing everything, it only re-instantiated and initialized the Activity pieces. The end result is that the underlying application remembered some information about the previous time the app was run (in particular, the tabs that were open), but Gecko didn't have any information about them because that is created and tracked by the Activity (and it was lost when the Activity was destroyed). The fix is simply iterating over all of the tabs the application knows again, and "reloading" them - this instantiates new tabs within the chrome and brings the chrome in sync with the Android UI.
I'm going to clean up the patches a little more, and then I'll post them.
Replying to sysrqb:
Okay. I spent a more than a few days troubleshooting a bug in Fennec this patch introduced. There is one more issue that remains and I'm not sure which direction we go with this right now (see ticket:27977#comment:12), I'll paste it here, as well for reference:
Right, this is a problem. We have two choices, and I don't particularly like either of them. Orbot doesn't need API 27 support, API 26 is sufficient. However, we have a problem with the Android Support Library version it uses.
- We can downgrade the version of the support library Orbot depends on, and lose Notifications on Android O (and later version)
- We can upgrade the version Fennec depends on and add a patch that changes the resource names.
Oh, and there is another problem I forgot to mention. Orbot's Onboarding screens are created using a third-party library (AppIntro), and that library requires a the newest version of the Android support library (thus it causes the Material Design resource renaming issue). I think this is okay, and we can delete Orbot's Onboarding stuff and implement our own, depending on #28329 (moved). I'm assuming this is what we will do, for now.
I also removed Orbot's dependency on AppUpdater for the same reason, but this is a much less critical (and less invasive) component, so I'm not concerned about removing it.
Edit: Also, Orbot includes a LocaleHelper which allows the user to switch locales at run-time. This relies on a newer version of the support library, too. I removed this (temporarily), and we can think about how we want to provide this in the future.
Trac:
28051_orbot_v1.diffOkay, this patch now includes renaming Orbot's "preferences" file so it doesn't conflict with Fennec's (they were both names preferences.xml).
Replying to sysrqb:
Okay, this patch now includes renaming Orbot's "preferences" file so it doesn't conflict with Fennec's (they were both names preferences.xml).
I was going through the patch and it looks like its setting targetSdkVersion to 16. From Nov 1, the targetSdkVersion needs to 26 or higher to upload to Google Play.
https://developer.android.com/distribute/best-practices/develop/target-sdk
Replying to sisbell:
I was going through the patch and it looks like its setting targetSdkVersion to 16. From Nov 1, the targetSdkVersion needs to 26 or higher to upload to Google Play.
https://developer.android.com/distribute/best-practices/develop/target-sdk
Oh, right, good catch. I forgot we already bumped Tor Browser's target. I'll change the patch for Orbot.
Okay, starting to review I guess
28051_2?-
Can you squash those fixups you made (that is commits 43936a111d23a89d509e9de75ecf2f28c17dc3c0 and c7fa82b84eceacf49d7d9436bfeb530630c9fd03) and base the branch on the latest
tor-browser-60.3.0esr-8.5-1? -
commit ca62a15e7e8f6781c61e82c4ec64bc4fcfbfe97c
+ //implementation 'com.github.apl-devs:appintro:v4.2.2' + //implementation 'com.github.javiersantos:AppUpdater:2.6.4'Let's omit those if we don't need them. The Orbot integration is already complex enough :)
- commit 5a755dad3b549f5e01b87016f6f4bfbfbd3654cc
I doubt we'll ever get back to a model where Tor Browser for Android ships without Orbot or something similarly integrated and needs to ask the user about installing Orbot first. Thus, can we clean up the
checkStartOrbot()related logic while we are at it instead of working around withcheckStartOrbot2()?-
Replying to gk:
Okay, starting to review I guess
28051_2?-
Can you squash those fixups you made (that is commits 43936a111d23a89d509e9de75ecf2f28c17dc3c0 and c7fa82b84eceacf49d7d9436bfeb530630c9fd03) and base the branch on the latest
tor-browser-60.3.0esr-8.5-1? -
commit ca62a15e7e8f6781c61e82c4ec64bc4fcfbfe97c
{{{
- //implementation 'com.github.apl-devs:appintro:v4.2.2'
- //implementation 'com.github.javiersantos:AppUpdater:2.6.4' }}} Let's omit those if we don't need them. The Orbot integration is already complex enough :)
- commit 5a755dad3b549f5e01b87016f6f4bfbfbd3654cc
I doubt we'll ever get back to a model where Tor Browser for Android ships without Orbot or something similarly integrated and needs to ask the user about installing Orbot first. Thus, can we clean up the
checkStartOrbot()related logic while we are at it instead of working around withcheckStartOrbot2()?Thanks for starting the review. I pushed
28051_3with the changes.-
-
Okay some more comments (I did not finish reviewing yet as I am fighting with #27977 (moved)):
a97dedfcb4830e13b1dea8625568ee1234ad9ac9 - why do we have the okhttp3.pro stuff and
-keepnames class org.torproject.*is commented out? The orbot blame does not specify the proguard rules update either (where okhttp3.* etc. got added the first time). Is that due to a newer SDK supported?2da1ac0e6d324000373742473ed8359168310564 - why do we have
+ final SafeIntent intent = new SafeIntent(getIntent());given that we don't seem to use that intent in the following code?
-
Here come the remaining bits:
fc856ca71ab3220a4a2dabc1bce32f56821e54a9 - okay
e9967a17a4d8eadcd35f45a81023620a93c72633 - okay
50bc8a44d1730cb561d2cccc13a75a13f1ce3795 - okay
d19f80ee1afcc9b5ed08e63f57d56ccfdd302e41 - okay, but why do we suddenly need this when orbot is included in the app?
e20ffcfdd47fa8fe2acd1cfff73931101c52b3f1 - that's currently busting my builds with
tor-browser-build, so not sure if that patch is what we want. Why do we need it? -
Replying to gk:
Here come the remaining bits:
fc856ca71ab3220a4a2dabc1bce32f56821e54a9 - okay
e9967a17a4d8eadcd35f45a81023620a93c72633 - okay
50bc8a44d1730cb561d2cccc13a75a13f1ce3795 - okay
d19f80ee1afcc9b5ed08e63f57d56ccfdd302e41 - okay, but why do we suddenly need this when orbot is included in the app?
I forgot one nit: s/mean all/means all/
Replying to igt0:
About the commit
e9967a17a4d8eadcd35f45a81023620a93c72633what happens if the the user closes the app using the swipe gesture? Does TBA stop the background services? Or should we move this logic to the onDestroy method?Yeah, that was a good question. We must call
quitAndClear()inonDestroy()for this.Replying to gk:
Okay some more comments (I did not finish reviewing yet as I am fighting with #27977 (moved)):
a97dedfcb4830e13b1dea8625568ee1234ad9ac9 - why do we have the okhttp3.pro stuff and
-keepnames class org.torproject.*is commented out? The orbot blame does not specify the proguard rules update either (where okhttp3.* etc. got added the first time). Is that due to a newer SDK supported?The okhttp3 lines are needed because of how the libraries are linked and ProGuard throws errors. I followed their official doc for this - https://github.com/square/okhttp/blob/master/okhttp/src/main/resources/META-INF/proguard/okhttp3.pro
I'll remove the
org.torproject.*entry, it's not needed anymore.2da1ac0e6d324000373742473ed8359168310564 - why do we have {{{
-
final SafeIntent intent = new SafeIntent(getIntent());
}}} given that we don't seem to use that intent in the following code?
Indeed. I'll delete that.
-
Replying to gk:
Replying to gk:
Here come the remaining bits:
fc856ca71ab3220a4a2dabc1bce32f56821e54a9 - okay
e9967a17a4d8eadcd35f45a81023620a93c72633 - okay
50bc8a44d1730cb561d2cccc13a75a13f1ce3795 - okay
d19f80ee1afcc9b5ed08e63f57d56ccfdd302e41 - okay, but why do we suddenly need this when orbot is included in the app?
We don't need this anymore. I'll delete it. We would need this if "swipping away the app" didn't mean "Quit app", but now we are using this definition so there should never be saved state in this scenario.
I forgot one nit: s/mean all/means all/
I now have branch
28051_5for tor browser ready for review. I also split the Orbot patch into many patches, so it's easy to review. Also, I pushed an Orbot branch, so it is easier to review.http://oniongit.eu/sysrqb/orbot.git branch
28051_3Trac:
Keywords: TorBrowserTeam201811 deleted, TorBrowserTeam201811R added
Status: needs_revision to needs_review-
28051_5looks good to me now. Here comes the review of the Orbot patches:42ce2f3be79078a780f80aa6b3d5bdcf7d685737 - not okay "that that" (one "that" is enough); I guess we could ignore the minimalperm
AndroidManifest.xmlas there is no such productFlavor anymore anyway? (I was confused as you seemed to not adjust the targetSdkVersion/mcSdkVersion)?9ee57d5e158ec0f8c5dc71decfa4eb5d7656e70d - okay
108a70748ce6a2406bc780fca752c24801b777a3 - okay
60e599b9fdbca45ff2dababf5fcaa791aabcf654 - do we need minimalperm changes?
653a7f6f119b26dd7920ea802435d3b3163ec19d - okay (what's the reason we suddenly need to cast them?)
b82c86f18775d55fe60ace7bdb1a7b2f5d1f70c0 - if you grep for "LocaleHelper" there are a bunch of other cases you did not touch. Are we good with leaving them as-is?
c487b13bfc21a2e5ececdf0f5ad151afef011cf5 - "// Rename preferences do it" -> "//Rename preferences so it"; you could think about patching the WALKTROUGH file as well to change "preferences.xml" there, too (but I don't feel strongly about that)
c3e4361f4b8b6366aefd14dc3dfcb8e1f41badc9 - could you add a comment why you commented the
setChannelId()call? (in case we really need to comment it out); actually after looking at patch 12 it seems this part of the patch should get deleted. It's already commented out in patch 12 (that is b529abde51aac1a0a7fa6ca2500313a2e13286fc, where it fits better).ab2e00148dd7b522d34c0a38bd2f2efaefe62683 - "swipped away" -> "swiped away"
b29a86b6b8d0ec9020ff96592bdd84ae6a899d30 - okay
0effce6c167ad8ccc03ca476b6e1db6ddecbea7d - okay
b529abde51aac1a0a7fa6ca2500313a2e13286fc - "This was add" -> "This was added"; how did you chose the methods to implement in your wrapper class? There seem to be other methods that got not implemented and not used and others implemented and not used in Orbot's code.
Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201811R deleted, TorBrowserTeam201811 added Replying to gk:
28051_5looks good to me now. Here comes the review of the Orbot patches:Thanks! I'll clean these up and prepare a new branch.
42ce2f3be79078a780f80aa6b3d5bdcf7d685737 - not okay "that that" (one "that" is enough); I guess we could ignore the minimalperm
AndroidManifest.xmlas there is no such productFlavor anymore anyway? (I was confused as you seemed to not adjust the targetSdkVersion/mcSdkVersion)?Yes, I agree. I didn't dig into the history of the minimalperm manifest, but now I see why it is there. I agree we can ignore it because it isn't being used anymore.
9ee57d5e158ec0f8c5dc71decfa4eb5d7656e70d - okay
108a70748ce6a2406bc780fca752c24801b777a3 - okay
60e599b9fdbca45ff2dababf5fcaa791aabcf654 - do we need minimalperm changes?
Nope.
653a7f6f119b26dd7920ea802435d3b3163ec19d - okay (what's the reason we suddenly need to cast them?)
I get an error like:
> Task :app:compileFullpermReleaseJavaWithJavac FAILED /home/android/orbot/app/src/main/java/org/torproject/android/ui/AppManagerActivity.java:62: error: incompatible types: View cannot be converted to GridView listApps = findViewById(R.id.applistview);I don't know the exact reason, but this is either directly or indirectly caused by changing the support library from version 27.1.1 to 23.4.0. I thought this was related to the API/SDK version we were targeting or compiling, but changing those doesn't affect whether or not we receive a compile-time error. I only see this when changing the version of
implementation 'com.android.support:design:23.4.0'in app/build.gradle.b82c86f18775d55fe60ace7bdb1a7b2f5d1f70c0 - if you grep for "LocaleHelper" there are a bunch of other cases you did not touch. Are we good with leaving them as-is?
It would be safer deleting the other cases, as well.
c487b13bfc21a2e5ececdf0f5ad151afef011cf5 - "// Rename preferences do it" -> "//Rename preferences so it"; you could think about patching the WALKTROUGH file as well to change "preferences.xml" there, too (but I don't feel strongly about that)
Keeping the documentation consistent is a good idea.
c3e4361f4b8b6366aefd14dc3dfcb8e1f41badc9 - could you add a comment why you commented the
setChannelId()call? (in case we really need to comment it out); actually after looking at patch 12 it seems this part of the patch should get deleted. It's already commented out in patch 12 (that is b529abde51aac1a0a7fa6ca2500313a2e13286fc, where it fits better).Agreed.
ab2e00148dd7b522d34c0a38bd2f2efaefe62683 - "swipped away" -> "swiped away"
Ack.
b29a86b6b8d0ec9020ff96592bdd84ae6a899d30 - okay
0effce6c167ad8ccc03ca476b6e1db6ddecbea7d - okay
b529abde51aac1a0a7fa6ca2500313a2e13286fc - "This was add" -> "This was added"; how did you chose the methods to implement in your wrapper class? There seem to be other methods that got not implemented and not used and others implemented and not used in Orbot's code.
Ack. I chose those methods because those should be all the methods used by used by Orbot and Fennec when they create notifications. My hope is this will provide us with notifications on all supported platforms. If I missed any methods expected by the app, then we should implement them.
Replying to sysrqb:
Replying to gk:
653a7f6f119b26dd7920ea802435d3b3163ec19d - okay (what's the reason we suddenly need to cast them?)
I get an error like:
> Task :app:compileFullpermReleaseJavaWithJavac FAILED /home/android/orbot/app/src/main/java/org/torproject/android/ui/AppManagerActivity.java:62: error: incompatible types: View cannot be converted to GridView listApps = findViewById(R.id.applistview); }}} I don't know the exact reason, but this is either directly or indirectly caused by changing the support library from version 27.1.1 to 23.4.0. I thought this was related to the API/SDK version we were targeting or compiling, but changing those doesn't affect whether or not we receive a compile-time error. I only see this when changing the version of `implementation 'com.android.support:design:23.4.0'` in app/build.gradle.Also, just so it's clear, the Android documentation says:
{{{ Note: In most cases -- depending on compiler support -- the resulting view is automatically cast to the target class type. If the target class type is unconstrained, an explicit cast may be necessary.
I'm guessing additional compiler support is enabled when a newer support library is used - but that's only a guess.-
Replying to sysrqb:
Replying to sysrqb:
Replying to gk:
653a7f6f119b26dd7920ea802435d3b3163ec19d - okay (what's the reason we suddenly need to cast them?)
I get an error like:
{{{
Task :app:compileFullpermReleaseJavaWithJavac FAILED /home/android/orbot/app/src/main/java/org/torproject/android/ui/AppManagerActivity.java:62: error: incompatible types: View cannot be converted to GridView listApps = findViewById(R.id.applistview);
}}}
I don't know the exact reason, but this is either directly or indirectly caused by changing the support library from version 27.1.1 to 23.4.0. I thought this was related to the API/SDK version we were targeting or compiling, but changing those doesn't affect whether or not we receive a compile-time error. I only see this when changing the version of
implementation 'com.android.support:design:23.4.0'in app/build.gradle.Also, just so it's clear, the Android documentation says:
{{{ Note: In most cases -- depending on compiler support -- the resulting view is automatically cast to the target class type. If the target class type is unconstrained, an explicit cast may be necessary. }}}
I'm guessing additional compiler support is enabled when a newer support library is used - but that's only a guess.
Makes sense, thanks!
