Opened 3 years ago

Closed 17 months ago

Last modified 11 months ago

#19675 closed task (fixed)

Merge Orfox patches into tor-browser

Reported by: gk Owned by: igt0
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, ff52-esr, TorBrowserTeam201803
Cc: amoghbl1, brade, mcs, arthuredelstein, fdsfgs@…, igt0 Actual Points:
Parent ID: #5709 Points:
Reviewer: Sponsor:

Description

We want to have Tor Browser mobile patches in our tree to make it easier keeping track on all things Tor Browser.

Child Tickets

TicketStatusOwnerSummaryComponent
#23144closedn8fr8Orfox UserAgent is very old. Please upgrade to modern version (52 or above).Applications/Orbot

Change History (24)

comment:1 Changed 3 years ago by mcs

Cc: brade mcs added

comment:2 Changed 3 years ago by arthuredelstein

Cc: arthuredelstein added

comment:3 Changed 3 years ago by arthuredelstein

Parent ID: #20680

comment:4 Changed 2 years ago by tokotoko

Cc: fdsfgs@… added

comment:5 Changed 2 years ago by arthuredelstein

Parent ID: #20680

Removing parent ID because this shouldn't block TBB/ESR52.

comment:6 Changed 2 years ago by arthuredelstein

Keywords: ff52-esr added

comment:7 Changed 21 months ago by gk

Keywords: TorBrowserTeam201712 added

comment:8 Changed 21 months ago by igt0

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

comment:9 Changed 20 months ago by sysrqb

Cc: igt0 added
Status: assignedneeds_revision

I rebased the Orfox commits on tor-browser-52.5.2esr-7.5-2. This includes all commits from orfox-tor-browser-52.2.0esr-7.0-1, but with some modifications.

1) Rename .mozconfig-orfox as .mozconfig-android
2) Align .mozconfig-android configuration with .mozconfig configuration
3) Add comments on non-obvious configuration changes
4) Bump Java JDK version from openjdk-7 to openjdk-8 (openjdk-7 is EOL)
5) Bump Android NDK version from r11b to r11c (hash of r11c is published on the Android website, r11b's hash is not available)
6) Verify sha256sum hash of downloaded SDK and NDK files match expected/published value

There are a few outstanding changes that are needed:
1) Compiling with tests enabled fails
2) App crashes when running in an Android emulator (it does not crash when running on a device)
3) Change the app name from Orfox to Tor Browser(?)
4) We need more unit tests
5) c823adc2da01ee47d50e03f1a5a4f14e661e8a2c adds a third-party extension, we need integrate its changes instead
6) Audit mobile.js and confirm it matches browser.js
7) Some of the commits add whitespace, we should clean before merging

branch is tor-browser-52.5.2esr-7.5-2_attempt0_1 at https://github.com/sysrqb/tor-browser/tree/tor-browser-52.5.2esr-7.5-2_attempt0_1

I have some more notes from my review, I will add them here after I think about them some more.

comment:10 Changed 20 months ago by igt0

Nice stuff! I am making comments straight in the github.

comment:11 in reply to:  9 Changed 20 months ago by igt0

I am not sure if it is related to this merge however we need to sync the:
https://github.com/sysrqb/tor-browser/blob/tor-browser-52.5.2esr-7.5-2_attempt0_1/mobile/android/app/mobile.js

with the

https://gitweb.torproject.org/tor-browser.git/tree/modules/libpref/init/all.js?h=tor-browser-52.5.2esr-7.5-2

Replying to sysrqb:

I rebased the Orfox commits on tor-browser-52.5.2esr-7.5-2. This includes all commits from orfox-tor-browser-52.2.0esr-7.0-1, but with some modifications.

1) Rename .mozconfig-orfox as .mozconfig-android
2) Align .mozconfig-android configuration with .mozconfig configuration
3) Add comments on non-obvious configuration changes
4) Bump Java JDK version from openjdk-7 to openjdk-8 (openjdk-7 is EOL)
5) Bump Android NDK version from r11b to r11c (hash of r11c is published on the Android website, r11b's hash is not available)
6) Verify sha256sum hash of downloaded SDK and NDK files match expected/published value

There are a few outstanding changes that are needed:
1) Compiling with tests enabled fails
2) App crashes when running in an Android emulator (it does not crash when running on a device)
3) Change the app name from Orfox to Tor Browser(?)
4) We need more unit tests
5) c823adc2da01ee47d50e03f1a5a4f14e661e8a2c adds a third-party extension, we need integrate its changes instead
6) Audit mobile.js and confirm it matches browser.js
7) Some of the commits add whitespace, we should clean before merging

branch is tor-browser-52.5.2esr-7.5-2_attempt0_1 at https://github.com/sysrqb/tor-browser/tree/tor-browser-52.5.2esr-7.5-2_attempt0_1

I have some more notes from my review, I will add them here after I think about them some more.

comment:12 Changed 20 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:13 Changed 19 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:14 Changed 19 months ago by sysrqb

Parent ID: 5709

comment:15 Changed 19 months ago by sysrqb

Parent ID: 5709#5709

comment:16 Changed 18 months ago by sysrqb

Okay, we decided we'll take a different path with this. I rebased the current Orfox patches onto tor-browser-52.6.0esr-8.0-2, there weren't any merge conflicts. Instead of fixing the current patches so we're comfortable calling mobile/android "Tor Browser for Android", we'll take the patches as-is and mark this task as complete. We'll fixup these commits as we rebase them onto mozilla-central.

I already have a few changes, but getting these patches in-tree is higher priority. We're keeping the Orfox branding because this is not a Tor Browser for Android, yet. I'll open some tickets that cover the changes I think we should make.

I did fixup one commit where symlinks were used for icons at different screen resolutions. Mach try does not like symlinks anywhere in the commit history, so I scrubbed that before we merged it.

Branch bug19675_orfox_patches on https://git.torproject.org/user/sysrqb/tor-browser.git

comment:17 Changed 18 months ago by sysrqb

Status: needs_revisionneeds_review

comment:18 Changed 18 months ago by gk

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201802 removed

comment:19 Changed 18 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802R removed
Status: needs_reviewneeds_revision

I am not overly thrilled to add .xpi files to our tree in commit 4553e490317622677475d0d63ba169074df2f085, especially as they are outdated (there is HTTPS-E 5.2.21 available instead of 5.2.20 if the requirement is a non-WebExtension Add-On). But I guess we can iterate from that. :)

Could you provide an updated branch without b27b64b540cd81a895a289e09d611a50255de3c1 as it does not seem to do anything?

comment:20 in reply to:  19 Changed 18 months ago by sysrqb

Replying to gk:

I am not overly thrilled to add .xpi files to our tree in commit 4553e490317622677475d0d63ba169074df2f085, especially as they are outdated (there is HTTPS-E 5.2.21 available instead of 5.2.20 if the requirement is a non-WebExtension Add-On). But I guess we can iterate from that. :)

Yes, I can follow up with a new ticket for updating these.

Could you provide an updated branch without b27b64b540cd81a895a289e09d611a50255de3c1 as it does not seem to do anything?

Sure thing, I pushed branch bug19675_orfox_patches_1 with that commit deleted
https://gitweb.torproject.org/user/sysrqb/tor-browser.git/log/?h=bug19675_orfox_patches_1

For reference:

$ git status
On branch bug19675_orfox_patches_1
nothing to commit, working tree clean

$ git diff bug19675_orfox_patches  
diff --git a/mobile/android/base/AndroidManifest.xml.in b/mobile/android/base/AndroidManifest.xml.in
index 1d6730d13ff4..1aa50a08463d 100644
--- a/mobile/android/base/AndroidManifest.xml.in
+++ b/mobile/android/base/AndroidManifest.xml.in
@@ -22,7 +22,6 @@
      fileincluded here, so that they can be referenced by both APKs. -->
 #include FennecManifest_permissions.xml.in
 
-
     <application android:label="@string/moz_app_displayname"
                  android:icon="@drawable/icon"
                  android:logo="@drawable/logo"

comment:21 Changed 17 months ago by gk

Resolution: fixed
Status: needs_revisionclosed

Alright, we are done here, finally. I did:

1) Run a build for all supported platforms/architectures to make sure the patches don't affect the desktop build
2) Reviewed the patches
3) Checked that they are indeed the same patches as in the Orfox repository
4) Built Orfox and ran it successfully on my test phone \o/.

sysrqb: I wonder if we should amend the "Building Orfox" part on our Hacking document a bit to reflect the new situation. FWIW: I needed to spent quite some time to get the Android SDK thing to do what it was supposed to do, even after calling ./mach bootstrap. It seems to be the case that the bootstrap thing is downloading the r24 SDK, however at the end the build-tools r23.0.3 are missing. Trying to install them fails even with the SDK manager because I only got offered r23.0.1 and the build system is insisting that it needs r23.0.3. So, I ended up just sim-linking 23.0.1 -> 23.0.3 and that worked smoothly. :) I wonder though whether there is something more straightforward we can suggest on the doc...

comment:22 Changed 17 months ago by sysrqb

Excellent! Opened #25562 for additional discussion.

comment:23 Changed 17 months ago by gk

Keywords: tbb-backport added

We might want to have this backported to make Orfox releases easier.

comment:24 Changed 11 months ago by gk

Keywords: tbb-backport removed
Note: See TracTickets for help on using tickets.