Opened 16 months ago

Last modified 9 months ago

#26574 new defect

Save TBA updates in the internal android storage

Reported by: igt0 Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TorBrowserTeam201901
Cc: sysrqb, gk, mcs, brade Actual Points:
Parent ID: #26242 Points:
Reviewer: Sponsor: Sponsor8

Description

Currently, UpdateService downloads the new apk and saves it to an external storage, it violates the disk avoidance principle.

We should modify its code to use the internal storage.

Child Tickets

Attachments (1)

0001-Bug-26574-Save-TBA-updates-in-the-internal-android-s.patch (2.7 KB) - added by igt0 16 months ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 16 months ago by igt0

We can not use the internal cache storage, though. It doesn't allow medium/big size files. (Normally fennec APK is around 30MB).

comment:2 Changed 16 months ago by igt0

Third party apps are not able to access the data directory of an specific application (unless they shared the same sharedUserId). So if we just store the apk in the fennec internal storage, the android installer is not able to access the APK. I am investigating alternatives.

comment:3 Changed 16 months ago by igt0

I found a workaround that allows the PackageInstaller to access and read the file [File.setReadable(true, false)] [0].

However, when the APK is being installed, Fennec throws a strict policy violation exception:

07-01 22:05:55.557  1088  1113 D InputDispatcher: Window went away: Window{d6eb27c u0 com.google.android.packageinstaller/com.android.packageinstaller.PackageInstallerActivity}
07-01 22:05:56.034 16797 16797 D StrictMode: StrictMode policy violation; ~duration=3 ms: android.os.StrictMode$StrictModeDiskReadViolation: policy=589855 violation=2
07-01 22:05:56.034 16797 16797 D StrictMode:    at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1263)
07-01 22:05:56.034 16797 16797 D StrictMode:    at libcore.io.BlockGuardOs.stat(BlockGuardOs.java:292)
07-01 22:05:56.034 16797 16797 D StrictMode:    at java.io.File.isDirectory(File.java:522)
07-01 22:05:56.034 16797 16797 D StrictMode:    at org.mozilla.gecko.GeckoThread.canUseProfile(GeckoThread.java:200)
07-01 22:05:56.034 16797 16797 D StrictMode:    at org.mozilla.gecko.GeckoThread.canUseProfile(GeckoThread.java:224)
07-01 22:05:56.034 16797 16797 D StrictMode:    at org.mozilla.gecko.GeckoService.initGecko(GeckoService.java:173)
07-01 22:05:56.034 16797 16797 D StrictMode:    at org.mozilla.gecko.GeckoService.handleIntent(GeckoService.java:194)
07-01 22:05:56.034 16797 16797 D StrictMode:    at org.mozilla.gecko.GeckoService.onStartCommand(GeckoService.java:233)
07-01 22:05:56.034 16797 16797 D StrictMode:    at android.app.ActivityThread.handleServiceArgs(ActivityThread.java:3062)
07-01 22:05:56.034 16797 16797 D StrictMode:    at android.app.ActivityThread.access$2200(ActivityThread.java:163)
07-01 22:05:56.034 16797 16797 D StrictMode:    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1460)
07-01 22:05:56.034 16797 16797 D StrictMode:    at android.os.Handler.dispatchMessage(Handler.java:102)
07-01 22:05:56.034 16797 16797 D StrictMode:    at android.os.Looper.loop(Looper.java:148)
07-01 22:05:56.034 16797 16797 D StrictMode:    at android.app.ActivityThread.main(ActivityThread.java:5585)
07-01 22:05:56.034 16797 16797 D StrictMode:    at java.lang.reflect.Method.invoke(Native Method)
07-01 22:05:56.034 16797 16797 D StrictMode:    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:730)
07-01 22:05:56.034 16797 16797 D StrictMode:    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:620)

[0] https://developer.android.com/reference/java/io/File#setReadable(boolean)

Last edited 16 months ago by igt0 (previous) (diff)

comment:4 Changed 16 months ago by igt0

Status: newneeds_review

I created an initial patch, It needs more tests however I will add it to review so we can have more eyes on it.

https://trac.torproject.org/projects/tor/attachment/ticket/26574/0001-Bug-26574-Save-TBA-updates-in-the-internal-android-s.patch

comment:5 Changed 16 months ago by igt0

Parent ID: #26242

comment:6 in reply to:  1 ; Changed 15 months ago by sysrqb

Replying to igt0:

We can not use the internal cache storage, though. It doesn't allow medium/big size files. (Normally fennec APK is around 30MB).

Are the MARs ~30MB, too? I expect the APKs will be 30-40MB, but I wonder if the updates are smaller? I'll a little sad if we're forced to download a new APK for every update. But, if this is limitation imposed by the operating system, then we may need to use that workaround. As long as the user consents to downloading the update for TBA begins downloading the new APK, I don't believe this violates the private browsing mode requirements.

comment:7 Changed 15 months ago by gk

Keywords: TorBrowserTeam201807R added

comment:8 in reply to:  6 Changed 15 months ago by igt0

Replying to sysrqb:

Replying to igt0:

We can not use the internal cache storage, though. It doesn't allow medium/big size files. (Normally fennec APK is around 30MB).

Are the MARs ~30MB, too? I expect the APKs will be 30-40MB, but I wonder if the updates are smaller? I'll a little sad if we're forced to download a new APK for every update. But, if this is limitation imposed by the operating system, then we may need to use that workaround. As long as the user consents to downloading the update for TBA begins downloading the new APK, I don't believe this violates the private browsing mode requirements.

So the Updater doesn't handle MAR files. Even in the https://ftp.mozilla.org/pub/mobile/releases/60.0.2/ I don't see any MAR file. So I think we will not be able to use it.

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

Replying to igt0:

I found a workaround that allows the PackageInstaller to access and read the file [File.setReadable(true, false)] [0].

However, when the APK is being installed, Fennec throws a strict policy violation exception:

The patch looks good (but I didn't test it). Do you know or remember when this strict policy violation exception was thrown? Was it after the download was completed? Also, can you describe how you tested this?

comment:10 Changed 15 months ago by gk

FWIW, there are other updater related pieces that use at least external permission prompts (e.g. https://dxr.mozilla.org/mozilla-esr60/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#955) which we should deal with if/when we move to internal storage.

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

Replying to sysrqb:

Replying to igt0:

I found a workaround that allows the PackageInstaller to access and read the file [File.setReadable(true, false)] [0].

However, when the APK is being installed, Fennec throws a strict policy violation exception:

The patch looks good (but I didn't test it). Do you know or remember when this strict policy violation exception was thrown? Was it after the download was completed? Also, can you describe how you tested this?

Strict policy violation: it was an issue in my side. I was pointing for an APK with different ID, thus the updater was not allowing an app A update an app B (that is good!).

Was it after the download was completed: No, when the download is completed, an android notification is sent and the user must click on it to start the update process.

How to test: There are two preferences you need to change app.update.autodownload and app.update.url.android.

The app.update.autodownload you can change to 2(it means enabled).

The app.update.url.android is the hard part:

  1. Create an update server (https://developer.mozilla.org/en-US/docs/Mozilla/Setting_up_an_update_server)
  2. Point the app.update.url.android to the update server:

<MY_ONION_DOMAIN>/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%MOZ_VERSION%/update.xml

  1. go to: Settings -> The Tor Project Tor Browser(last option) -> About Tor Browser

And click in the check for updates, if everything is in place it will start to download the apk and in the end you will see a notification about updating TBA.

So things that we need to do before merging the patch, we need to decide if the URL makes sense for us or if we can make it shorter. And update the prefs file.

comment:12 Changed 15 months ago by gk

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201807R removed

comment:13 Changed 14 months ago by igt0

Interesting analysis about downloading stuff in the external storage: https://issuetracker.google.com/issues/112630336

comment:14 Changed 14 months ago by gk

Keywords: TorBrowserTeam201809R added; TorBrowserTeam201808R removed

Moving review tickets to September

comment:15 Changed 14 months ago by sysrqb

Keywords: TBA-a2 added

comment:16 Changed 13 months ago by gk

Keywords: TorBrowserTeam201810R added; TorBrowserTeam201809R removed

Moving review tickets to October

comment:17 Changed 12 months ago by pili

Sponsor: Sponsor8

comment:18 Changed 12 months ago by gk

Keywords: TorBrowserTeam201811 added

Moving our tickets to November.

comment:19 Changed 12 months ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201810R TorBrowserTeam201811 removed

comment:20 Changed 11 months ago by gk

Keywords: TorBrowserTeam201812R added; TorBrowserTeam201811R removed

Move review tickets to Decemeber.

comment:21 Changed 11 months ago by gk

Keywords: TBA-a3 added

Setting tag for third Tor Browser for Android alpha milestone.

comment:22 Changed 11 months ago by gk

Keywords: TBA-a2 removed

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

comment:23 Changed 10 months ago by gk

Keywords: TorBrowserTeam201901R added; TorBrowserTeam201812R removed

Moving review tickets to 2019.

comment:24 in reply to:  11 Changed 9 months ago by gk

Cc: mcs brade added
Keywords: TorBrowserTeam201901 added; TBA-a3 TorBrowserTeam201901R removed

Replying to igt0:

So things that we need to do before merging the patch, we need to decide if the URL makes sense for us or if we can make it shorter. And update the prefs file.

+ see my comment:10.

This is for the builds we make available on our website which should support updating eventually. However, that's probably too much for our first stable series given that we are just two months away and testing the updater might take more time (especially as Mozilla is making not much use of it themselves). Thus, removing this from our TBA-a3 radar.

comment:25 Changed 9 months ago by gk

Status: needs_reviewnew
Note: See TracTickets for help on using tickets.