Opened 8 weeks ago

Closed 12 days ago

Last modified 12 days ago

#28051 closed enhancement (fixed)

Build Orbot into TBA

Reported by: sysrqb Owned by: sysrqb
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tba-a2, TorBrowserTeam201811R
Cc: tbb-team, gk, igt0, sisbell, eighthave Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor8

Description

While we're waiting for #27609, we'll try building Orbot directly into TBA (this flows into #27977).

Child Tickets

Change History (61)

comment:1 Changed 8 weeks ago by gk

Keywords: TorBrowserTeam201810 added
Priority: MediumVery High

comment:2 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:3 Changed 5 weeks ago by sysrqb

Owner: changed from tbb-team to sysrqb
Status: newassigned

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 an applicationId :) ).

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 keep directive 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.

comment:4 Changed 5 weeks ago by sysrqb

Maybe 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 28051 for 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/ and tor-browser/ share the same parent directory, then you can use this shell script for copying the three .aar files from orbot/ into mobile/android/app/
      $ 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/
    done
    

When 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.

Changed 5 weeks ago by sysrqb

Attachment: 28051_orbot_v0.patch added

Orbot patch v0

comment:5 Changed 4 weeks ago by 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.

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:

1) We can downgrade the version of the support library Orbot depends on, and lose Notifications on Android O (and later version)
2) 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.

comment:6 in reply to:  5 Changed 4 weeks ago by sysrqb

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.

1) We can downgrade the version of the support library Orbot depends on, and lose Notifications on Android O (and later version)
2) 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. 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.

Last edited 4 weeks ago by sysrqb (previous) (diff)

Changed 4 weeks ago by sysrqb

Attachment: 28051_orbot_v1.diff added

comment:7 Changed 4 weeks ago by sysrqb

Status: assignedneeds_review

Okay, new Orbot patch, and I pushed a new tor browser branch - 28051_1. I'll set this to review, but I know there is more testing needed and at least one remaining bug (Orbot's Settings menu doesn't work).

comment:8 Changed 4 weeks ago by 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).

https://trac.torproject.org/projects/tor/attachment/ticket/28051/0001-Bug-28051-Build-Orbot-as-a-Library.patch

comment:9 in reply to:  8 ; Changed 4 weeks ago by sisbell

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).

https://trac.torproject.org/projects/tor/attachment/ticket/28051/0001-Bug-28051-Build-Orbot-as-a-Library.patch

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

comment:10 in reply to:  9 Changed 4 weeks ago by sysrqb

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.

comment:11 Changed 4 weeks ago by sysrqb

Okay, I rebased the changes on top of tag 16.0.5-RC-1-tor-0.3.4.9 (which is current HEAD of master).

comment:12 Changed 4 weeks ago by sisbell

jsocksAndroid is still targeting sdk 27

comment:13 in reply to:  12 Changed 4 weeks ago by sysrqb

Replying to sisbell:

jsocksAndroid is still targeting sdk 27

Yep, I missed that. Good catch.

comment:14 Changed 4 weeks ago by targetSdkVersion

"As always, you must ensure compatibility between the major version of Android Support Library and your app’s compileSdkVersion.

We recommend that you choose a targetSdkVersion smaller than or equal to the Support Library’s major version."

comment:15 Changed 3 weeks ago by sysrqb

Okay, here's a new patch against 16.0.5-RC-1-tor-0.3.4.9. There's a new tor-browser branch, as well: 28051_2.

comment:16 Changed 3 weeks ago by sysrqb

Ugh. This one should be correct.

comment:17 Changed 3 weeks ago by gk

Okay, starting to review I guess 28051_2?

1) 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?

2) 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 :)

3) 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 with checkStartOrbot2()?

comment:18 in reply to:  17 Changed 3 weeks ago by sysrqb

Replying to gk:

Okay, starting to review I guess 28051_2?

1) 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?

2) 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 :)

3) 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 with checkStartOrbot2()?

Thanks for starting the review. I pushed 28051_3 with the changes.

comment:19 Changed 3 weeks ago by igt0

About the commit e9967a17a4d8eadcd35f45a81023620a93c72633 what 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?

comment:20 Changed 3 weeks ago by gk

Okay some more comments (I did not finish reviewing yet as I am fighting with #27977):

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?

comment:21 Changed 3 weeks ago by 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?

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?

comment:22 in reply to:  21 ; Changed 3 weeks ago by 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?

I forgot one nit: s/mean all/means all/

comment:23 Changed 3 weeks ago by gk

Status: needs_reviewneeds_revision

comment:24 in reply to:  19 Changed 2 weeks ago by sysrqb

Replying to igt0:

About the commit e9967a17a4d8eadcd35f45a81023620a93c72633 what 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() in onDestroy() for this.

comment:25 in reply to:  20 Changed 2 weeks ago by sysrqb

Replying to gk:

Okay some more comments (I did not finish reviewing yet as I am fighting with #27977):

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.

comment:26 in reply to:  22 Changed 2 weeks ago by sysrqb

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/

comment:27 Changed 2 weeks ago by sysrqb

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed
Status: needs_revisionneeds_review

I now have branch 28051_5 for 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_3

Changed 2 weeks ago by sysrqb

comment:28 Changed 2 weeks ago by sysrqb

Okay, I can't add all the Orbot patches (*mumble* *mumble* spam captcha *mumble *mumble*). Please use the git branch for review. The patches can be created locally using git format-patch 16.0.5-RC-1-tor-0.3.4.9.

comment:29 Changed 2 weeks ago by pili

Sponsor: Sponsor8

comment:30 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201811R removed
Status: needs_reviewneeds_revision

28051_5 looks 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.xml as 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.

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

Replying to gk:

28051_5 looks 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.xml as 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.

comment:32 in reply to:  31 ; Changed 2 weeks ago by 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.

comment:33 in reply to:  32 Changed 2 weeks ago by gk

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!

comment:34 Changed 2 weeks ago by targetSdkVersion

Without actual support of API 26 you'll get only the runtime crashes on 24, 25, 26 API Android. And you'll be forced to API 28 and 64-bit in August 2019.
(servo is a mess with its classpath 'com.android.tools.build:gradle:2.2.3')

comment:35 Changed 2 weeks ago by eighthave

Cc: eighthave added

comment:36 Changed 2 weeks ago by gk

Okay, I gave the whole thing some testing and it works, nice! The two main points I have so far:

1) I start the app and it says it's Tor Browser but I still need to start that Orbot thing which might confuse users ("Hey, I want to run Tor Browser and I already started it!1! What do I need to do now with that Orbot piece?). But I think that's okay for the alpha.

2) More worriesome, though: I start Orbot and after it has bootstrapped nothing happens. It's essentially sitting there and no browser window pops up. In fact, it's not even clear how one could manually open one. I pressed by accident the button I usually press if I want to go back a website in my history and suddenly the window is open an I can use Tor Browser. Pressing that "going back one website"-button "works" as well if one has not started the Orbot part yet. The huge downside to that scenario is that it's not obvious how to go back to start Orbot now and the user is just receiving proxy connection errors (so far I needed to get rid of the current Tor Browser instance and start over to work around that).

comment:37 Changed 2 weeks ago by sysrqb

Status: needs_revisionneeds_review

Okay. I have two new branches, one for tor-browser and another for orbot.

The orbot branch (28051_orbot_4) includes the changes in comment:31. It also includes a new proguard rule for keeping org.torproject.android.settings.Languages.setup(Class,int) that it was stipping due to it not being used. Unfortunately, this method is called by the Application when the app starts, so I added it in GeckoApplication in Fennec.

I have mixed feelings about adding this, because on the one hand this is needed for opening Orbot's Settings menu - otherwise Orbot crashes and we're left with Tor Browser without Tor. However, on the other hand, we'll likely never use Orbot's settings menu. But, we're already renaming the preferences layout xml so it doesn't conflict with Fennec, so if we do that then I think it makes sense that we prevent this crash.

The new tor-browser branch adds the necessary initialization calls - 28051_6.

comment:38 in reply to:  36 Changed 2 weeks ago by sysrqb

Replying to gk:

Okay, I gave the whole thing some testing and it works, nice! The two main points I have so far:

1) I start the app and it says it's Tor Browser but I still need to start that Orbot thing which might confuse users ("Hey, I want to run Tor Browser and I already started it!1! What do I need to do now with that Orbot piece?). But I think that's okay for the alpha.

2) More worriesome, though: I start Orbot and after it has bootstrapped nothing happens. It's essentially sitting there and no browser window pops up. In fact, it's not even clear how one could manually open one. I pressed by accident the button I usually press if I want to go back a website in my history and suddenly the window is open an I can use Tor Browser. Pressing that "going back one website"-button "works" as well if one has not started the Orbot part yet. The huge downside to that scenario is that it's not obvious how to go back to start Orbot now and the user is just receiving proxy connection errors (so far I needed to get rid of the current Tor Browser instance and start over to work around that).

Indeed, I agree with these concerns. They should be taken care of in #28329

comment:39 Changed 2 weeks ago by sysrqb

Also, note to self, seeing this in the logs. Maybe related to #28507, but should investigate.

11-27 14:47:48.676 20747 20768 I Gecko   : *************************
11-27 14:47:48.676 20747 20768 I Gecko   : A coding exception was thrown and uncaught in a Task.
11-27 14:47:48.676 20747 20768 I Gecko   : 
11-27 14:47:48.676 20747 20768 I Gecko   : Full message: TypeError: Cc['@mozilla.org/push/Service;1'] is undefined
11-27 14:47:48.676 20747 20768 I Gecko   : Full stack: Sanitizer.prototype.items.siteSettings.clear</<@resource://gre/modules/Sanitizer.jsm:128:15
11-27 14:47:48.676 20747 20768 I Gecko   : Sanitizer.prototype.items.siteSettings.clear<@resource://gre/modules/Sanitizer.jsm:127:15
11-27 14:47:48.676 20747 20768 I Gecko   : TaskImpl_run@resource://gre/modules/Task.jsm:326:42
11-27 14:47:48.676 20747 20768 I Gecko   : TaskImpl@resource://gre/modules/Task.jsm:275:3
11-27 14:47:48.676 20747 20768 I Gecko   : asyncFunction@resource://gre/modules/Task.jsm:247:14
11-27 14:47:48.676 20747 20768 I Gecko   : _clear@resource://gre/modules/Sanitizer.jsm:59:14
11-27 14:47:48.676 20747 20768 I Gecko   : clearItem@resource://gre/modules/Sanitizer.jsm:40:14
11-27 14:47:48.676 20747 20768 I Gecko   : sanitize@chrome://browser/content/browser.js:1566:25
11-27 14:47:48.676 20747 20768 I Gecko   : onEvent@chrome://browser/content/browser.js:1787:9
11-27 14:47:48.676 20747 20768 I Gecko   : 
11-27 14:47:48.676 20747 20768 I Gecko   : *************************

comment:40 in reply to:  37 ; Changed 12 days ago by gk

Replying to sysrqb:

Okay. I have two new branches, one for tor-browser and another for orbot.

The orbot branch (28051_orbot_4) includes the changes in comment:31. It also includes a new proguard rule for keeping org.torproject.android.settings.Languages.setup(Class,int) that it was stipping due to it not being used. Unfortunately, this method is called by the Application when the app starts, so I added it in GeckoApplication in Fennec.

I have mixed feelings about adding this, because on the one hand this is needed for opening Orbot's Settings menu - otherwise Orbot crashes and we're left with Tor Browser without Tor. However, on the other hand, we'll likely never use Orbot's settings menu. But, we're already renaming the preferences layout xml so it doesn't conflict with Fennec, so if we do that then I think it makes sense that we prevent this crash.

I think that's okay and I agree with that.

The new tor-browser branch adds the necessary initialization calls - 28051_6.

Okay, both branches look good to me. I'll commit the tor-browser patches once we are good with all the other stuff.

comment:41 in reply to:  40 ; Changed 12 days ago by sysrqb

Replying to gk:

Replying to sysrqb:

The new tor-browser branch adds the necessary initialization calls - 28051_6.

Okay, both branches look good to me. I'll commit the tor-browser patches once we are good with all the other stuff.

Great. I pushed a new orbot branch - 28051_orbot_5. This only changes the commit "Change Orbot's behavior for Tor Browser" and calls "finish()" in the Activity when Tor's bootstrapping reaches 100%. This returns the user to Fennec automatically. This should only happen when first starts TBA. If the user opens our Orbot using the notification message, then Orbot remains open until they hit the back button or select Exit from Orbot's menu.

comment:42 in reply to:  41 ; Changed 12 days ago by sysrqb

Replying to sysrqb:

If the user opens our Orbot using the notification message, then Orbot remains open until they hit the back button or select Exit from Orbot's menu.

Hrm. Yeah, selecting Exit is actually bad. Now Exit actually exits and kills orbot, so the user is provided a Tor Browser without a Tor. I think we should change this, too.

comment:43 in reply to:  42 ; Changed 12 days ago by sysrqb

Replying to sysrqb:

Replying to sysrqb:

If the user opens our Orbot using the notification message, then Orbot remains open until they hit the back button or select Exit from Orbot's menu.

Hrm. Yeah, selecting Exit is actually bad. Now Exit actually exits and kills orbot, so the user is provided a Tor Browser without a Tor. I think we should change this, too.

I pushed 28051_orbot_6 where the Exit menu option is renamed as "Close", and Orbot doesn't stop tor when Close is selected.

comment:44 in reply to:  43 Changed 12 days ago by gk

Replying to sysrqb:

Replying to sysrqb:

Replying to sysrqb:

If the user opens our Orbot using the notification message, then Orbot remains open until they hit the back button or select Exit from Orbot's menu.

Hrm. Yeah, selecting Exit is actually bad. Now Exit actually exits and kills orbot, so the user is provided a Tor Browser without a Tor. I think we should change this, too.

I pushed 28051_orbot_6 where the Exit menu option is renamed as "Close", and Orbot doesn't stop tor when Close is selected.

Looks good to me.

comment:45 Changed 12 days ago by gk

Resolution: fixed
Status: needs_reviewclosed

I merged the tor-browser patches into tor-browser-60.3.0esr-8.5-1 (commits 0bb30b64417fa0789edc6b8d9d8ff6c684a42a78, 078d1f89036764b9486cd4de841ad2bfcd748ac0, 2ffe1e523e67c62a5bc160cfa8d09030cbdf5267, 4514386c95b57153ca550d710c117efe5a3acf93, 8e5290a7ff9c70dde87e055256717feb342a8645, and e6c7c8b6022a3d9e5c1dab2966ab45b467d84630). The orbot patches will be dealt with in #27977, closing. Thanks!

comment:46 Changed 12 days ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed
Note: See TracTickets for help on using tickets.