Opened 9 months ago

Last modified 2 weeks ago

#26858 needs_revision defect

TBA: Investigating patching AccountManager

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

Child Tickets

TicketStatusOwnerSummaryComponent
#27065newtbb-teamTBA: Delete AccountManager usage from TabsApplications/Tor Browser
#27069newtbb-teamTBA: Make SendTab SafeApplications/Tor Browser
#27070newtbb-teamTBA: FxAccountStatusActivity shouldn't modify Android AccountsApplications/Tor Browser
#27072newtbb-teamTBA: Delete SyncPreference from tablet layoutApplications/Tor Browser

Change History (20)

comment:1 Changed 9 months ago by gk

Parent ID: #26401#26531

comment:2 in reply to:  description ; Changed 9 months ago by sysrqb

Replying to sysrqb:

Orfox previously patched some of the AccountManager and Sync code. Are these still needed?

https://github.com/guardianproject/tor-browser/commit/45ba8e208975daebda7520ea0f111f475adba967

1) mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java

AccountsHelper is never used because we deleted the only instance in BrowserApp in Bug 25741 - TBA: Neuter Firefox Accounts.

2) ContactService.java

This was deleted in:

commit 9a8b60ffcae1d642096c9aaedfdd3cf37f87fe7f
Author: Makoto Kato <m_kato@ga2.so-net.ne.jp>
Date:   Mon Aug 8 15:40:19 2016 +0900

    Bug 1284283 - Remove MozContact API Android backend. r=sebastian
    
    MozReview-Commit-ID: GLhrA1cc1Rq
    
    --HG--
    extra : rebase_source : 73efab6d9468388a6ba0be489250d8dd3d185cd6

3) Tabs.java

This still exists, and an instance of AccountManager is still retrieved here. We should clean this. I believe the goal here is preventing the caching of current tabs on disk (queuePersistAllTabs()).

4) mobile/android/base/java/org/mozilla/gecko/overlays/service/sharemethods/SendTab.java

We should clean this. It is initialized in OverlayActionService.java, it's a background service for handling sharing the current tab with another device (when FxA/sync are enabled).

5) mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java

Already cleaned in FxA neutering

6) mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusActivity.java

Needs cleaning.

7) mobile/android/services/src/main/java/org/mozilla/gecko/sync/setup/SyncAccounts.java

Deleted in:

commit 4978a3514277d4cc310ee6e15072d15abd1dfe2a
Author: Nick Alexander <nalexander@mozilla.com>
Date:   Mon Jan 18 17:02:56 2016 -0800

    Bug 1220906 - Part 6: Remove Old Sync configuration code. r=rnewman
    
    --HG--
    extra : commitid : 1NbJi3JriIB
    extra : rebase_source : 895092b10bf20b0425e266a1d340e22b81f8daee

8) mobile/android/services/src/main/java/org/mozilla/gecko/sync/setup/activities/SetupSyncActivity.java

Deleted in:

commit 35d2682654031d011b46bb07344490686f748117
Author: Nick Alexander <nalexander@mozilla.com>
Date:   Tue Jan 19 11:31:31 2016 -0800

    Bug 1220906 - Part 5: Remove Old Sync setup and JPAKE code. r=rnewman
    
    --HG--
    extra : commitid : DrheDFD75zQ
    extra : rebase_source : 4249543a2df591bf042b93094a2f112fd8460805


https://github.com/guardianproject/tor-browser/commit/b044089a93bd36ddb454ac11eecc72a2c295229c

Already deleted in:

commit 99f346a73bde893c862bbb6504ff52f63b5b60e0
Author: Amogh Pradeep <amoghbl1@gmail.com>
Date:   Fri Jul 17 13:27:07 2015 -0400

    Bug 25741 - TBA: Remove sync option from preferences
    
    We don't want this while the Sync subsystem is a proxy-bypass risk. We
    can drop this when the feature is patched.
    
    Signed-off-by: Amogh Pradeep <amoghbl1@gmail.com>

But, we should delete in mobile/android/app/src/main/res/xml/preferences_general_tablet.xml, too.

https://github.com/guardianproject/tor-browser/commit/842618571bbe865657836447cbd50fffc027b582

This isn't needed because the firstrun page was changed so it used the RestrictedWelcomePanel, and TBA's onboarding/welcome will be custom-built.

comment:3 in reply to:  2 Changed 9 months ago by sysrqb

Replying to sysrqb:

Replying to sysrqb:
3) Tabs.java

This still exists, and an instance of AccountManager is still retrieved here. We should clean this. I believe the goal here is preventing the caching of current tabs on disk (queuePersistAllTabs()).

#27065

4) mobile/android/base/java/org/mozilla/gecko/overlays/service/sharemethods/SendTab.java

We should clean this. It is initialized in OverlayActionService.java, it's a background service for handling sharing the current tab with another device (when FxA/sync are enabled).

#27069

6) mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusActivity.java

Needs cleaning.

#27070

https://github.com/guardianproject/tor-browser/commit/b044089a93bd36ddb454ac11eecc72a2c295229c

But, we should delete in mobile/android/app/src/main/res/xml/preferences_general_tablet.xml, too.

#27072

comment:4 Changed 9 months ago by sysrqb

But, I wonder if we should accomplish this differently. If we put FxA and Sync behind compile-time build flags and add Stub classes, this may make it easier to maintain (and easier to reason about).

comment:5 Changed 8 months ago by gk

I wonder what Mozilla's stance is here. If that's obsolete with GeckoView coming it seems to me the approach we pick should not matter much (assuming we only carry around the patches for this and the child bugs during this ESR cycle).

comment:6 Changed 8 months ago by sysrqb

I took the approach of using the compile-time stub classes. This should be safer than simply hoping we find all the non-safe code. It seems Mozilla don't care too much about Fennec, in general, so we shouldn't experience many conflicts with these patches in the future.

Branch 26858.

I feel this is a better approach because it only exposes non-functional implementations. The commits in the branch are grouped logically (they don't successfully compile individually, so you need all of them for testing).

comment:7 Changed 8 months ago by gk

Keywords: TorBrowserTeam201808R added
Status: newneeds_review

comment:8 Changed 8 months ago by gk

Keywords: TorBrowserTeam201809R added; TorBrowserTeam201808R removed

Moving review tickets to September

comment:9 Changed 7 months ago by sysrqb

Keywords: TBA-a2 added
Parent ID: #26531

Moving to second-alpha TBA keyword.

comment:10 Changed 7 months ago by gk

Keywords: TorBrowserTeam201810R added; TorBrowserTeam201809R removed

Moving review tickets to October

comment:11 Changed 6 months ago by pili

Sponsor: Sponsor8

comment:12 Changed 6 months ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201810R removed

Moving reviews to November.

comment:13 Changed 5 months ago by gk

Keywords: TorBrowserTeam201812R added; TorBrowserTeam201811R removed

Move review tickets to Decemeber.

comment:14 Changed 4 months ago by gk

Keywords: TBA-a3 added

Setting tag for third Tor Browser for Android alpha milestone.

comment:15 Changed 4 months ago by gk

Keywords: TBA-a2 removed

We are beyond TBA-a2, TBA-a3 is the new black.

comment:16 Changed 3 months ago by gk

Keywords: TorBrowserTeam201901R added; TorBrowserTeam201812R removed

Moving review tickets to 2019.

comment:17 Changed 3 months ago by gk

Keywords: TorBrowserTeam201901 added; TorBrowserTeam201901R removed
Status: needs_reviewneeds_revision

Sorry, for the long delay here. I started looking into the patches today. It seems they do not compile (anymore) I checked with my bug_26858_test branch and get:

 0:31.73 :app:compileOfficialWithoutGeckoBinariesNoMinApiPhotonDebugJavaWithJavac/var/tmp/build/firefox-cbe1939d23ce/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:153: error: cannot find symbol
 0:31.73 import org.mozilla.gecko.telemetry.TelemetryCorePingDelegate;
 0:31.73                                   ^
 0:31.73   symbol:   class TelemetryCorePingDelegate
 0:31.73   location: package org.mozilla.gecko.telemetry
 0:31.77 /var/tmp/build/firefox-cbe1939d23ce/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:427: error: cannot find symbol
 0:31.77     private final TelemetryCorePingDelegate mTelemetryCorePingDelegate = new TelemetryCorePingDelegate();
 0:31.78                   ^
 0:31.78   symbol:   class TelemetryCorePingDelegate
 0:31.78   location: class BrowserApp
 0:32.17 /var/tmp/build/firefox-cbe1939d23ce/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:427: error: cannot find symbol
 0:32.17     private final TelemetryCorePingDelegate mTelemetryCorePingDelegate = new TelemetryCorePingDelegate();
 0:32.17                                                                              ^
 0:32.17   symbol:   class TelemetryCorePingDelegate
 0:32.17   location: class BrowserApp
 0:33.23 Note: Some input files use or override a deprecated API.
 0:33.23 Note: Recompile with -Xlint:deprecation for details.
 0:33.23 3 errors
 0:33.25  FAILED

Additionally, do we actually need the TabSender interface in our SendTab stub?

comment:18 Changed 2 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:19 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201902 removed

Moving remaining tickets to March.

comment:20 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201903 removed

Moving tickets to April.

Note: See TracTickets for help on using tickets.