From 2d377453d92da42a90c19f9547de32ca4a06c018 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 14 Jul 2017 11:18:09 +1000 Subject: [PATCH 1/4] Ensure swapping doesn't get confused by apks in different repos. While investigating #1086 which was about swap being busted, I discovered that we recently introduced a worse bug when working with multi sig stuff. The swap process, when installing an app (or even when listening for if a user started installing - before they even did anything), would ask for an apk from any repo. This is wrong, because we should only ask for the apks from the swap repo when presented with a swap dialog. By fixing this so that it asks for a specific apk, this may also fix the issue in #1086, because that was about us not asking for enough info from the database for each Apk which was returned. Now we just return all columns, because the performance overhead should be minimal, but it prevents this class of bugs, where we didn't fully populate our value object. However, I'm not confident that it is fixed, because I was unable to reproduce it due to the other crash fixed in this change. Relevant crash: ``` java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String org.fdroid.fdroid.data.Apk.getUrl()' on a null object reference at org.fdroid.fdroid.views.swap.SwapAppsView$AppListAdapter$ViewHolder.setApp(SwapAppsView.java:311) at org.fdroid.fdroid.views.swap.SwapAppsView$AppListAdapter.bindView(SwapAppsView.java:422) at org.fdroid.fdroid.views.swap.SwapAppsView$AppListAdapter.newView(SwapAppsView.java:414) at android.support.v4.widget.CursorAdapter.getView(CursorAdapter.java:269) at android.widget.AbsListView.obtainView(AbsListView.java:2349) at android.widget.ListView.makeAndAddView(ListView.java:1864) at android.widget.ListView.fillDown(ListView.java:698) ... ``` --- .../org/fdroid/fdroid/data/ApkProvider.java | 42 ++++++++++++++++--- .../fdroid/views/swap/SwapAppsView.java | 29 +++++++++---- .../views/swap/SwapWorkflowActivity.java | 4 +- .../test/java/org/fdroid/fdroid/Assert.java | 7 ++++ .../java/org/fdroid/fdroid/TestUtils.java | 11 ++++- .../fdroid/fdroid/data/ApkProviderTest.java | 22 ++++++++++ .../fdroid/fdroid/data/RepoProviderTest.java | 6 +++ 7 files changed, 102 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java index 0fedc95db..290e6674d 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java @@ -132,13 +132,15 @@ public class ApkProvider extends FDroidProvider { return cursorToList(cursor); } - public static Apk get(Context context, Uri uri) { - return get(context, uri, Cols.ALL); + @NonNull + public static List findAppVersionsByRepo(Context context, App app, Repo repo) { + ContentResolver resolver = context.getContentResolver(); + final Uri uri = getRepoUri(repo.getId(), app.packageName); + Cursor cursor = resolver.query(uri, Cols.ALL, null, null, null); + return cursorToList(cursor); } - public static Apk get(Context context, Uri uri, String[] fields) { - ContentResolver resolver = context.getContentResolver(); - Cursor cursor = resolver.query(uri, fields, null, null, null); + private static Apk cursorToApk(Cursor cursor) { Apk apk = null; if (cursor != null) { if (cursor.getCount() > 0) { @@ -150,6 +152,17 @@ public class ApkProvider extends FDroidProvider { return apk; } + public static Apk get(Context context, Uri uri) { + return get(context, uri, Cols.ALL); + } + + @Nullable + public static Apk get(Context context, Uri uri, String[] fields) { + ContentResolver resolver = context.getContentResolver(); + Cursor cursor = resolver.query(uri, fields, null, null, null); + return cursorToApk(cursor); + } + @NonNull public static List findApksByHash(Context context, String apkHash) { ContentResolver resolver = context.getContentResolver(); @@ -167,10 +180,12 @@ public class ApkProvider extends FDroidProvider { private static final int CODE_APK_ROW_ID = CODE_APKS + 1; static final int CODE_APK_FROM_ANY_REPO = CODE_APK_ROW_ID + 1; static final int CODE_APK_FROM_REPO = CODE_APK_FROM_ANY_REPO + 1; + private static final int CODE_REPO_APP = CODE_APK_FROM_REPO + 1; private static final String PROVIDER_NAME = "ApkProvider"; protected static final String PATH_APK_FROM_ANY_REPO = "apk-any-repo"; protected static final String PATH_APK_FROM_REPO = "apk-from-repo"; + protected static final String PATH_REPO_APP = "repo-app"; private static final String PATH_APKS = "apks"; private static final String PATH_APP = "app"; private static final String PATH_REPO = "repo"; @@ -187,6 +202,7 @@ public class ApkProvider extends FDroidProvider { PACKAGE_FIELDS.put(Cols.Package.PACKAGE_NAME, PackageTable.Cols.PACKAGE_NAME); MATCHER.addURI(getAuthority(), PATH_REPO + "/#", CODE_REPO); + MATCHER.addURI(getAuthority(), PATH_REPO_APP + "/#/*", CODE_REPO_APP); MATCHER.addURI(getAuthority(), PATH_APK_FROM_ANY_REPO + "/#/*/*", CODE_APK_FROM_ANY_REPO); MATCHER.addURI(getAuthority(), PATH_APK_FROM_ANY_REPO + "/#/*", CODE_APK_FROM_ANY_REPO); MATCHER.addURI(getAuthority(), PATH_APK_FROM_REPO + "/#/#", CODE_APK_FROM_REPO); @@ -227,6 +243,15 @@ public class ApkProvider extends FDroidProvider { .build(); } + public static Uri getRepoUri(long repoId, String packageName) { + return getContentUri() + .buildUpon() + .appendPath(PATH_REPO_APP) + .appendPath(Long.toString(repoId)) + .appendPath(packageName) + .build(); + } + public static Uri getApkFromAnyRepoUri(Apk apk) { return getApkFromAnyRepoUri(apk.packageName, apk.versionCode, null); } @@ -427,6 +452,13 @@ public class ApkProvider extends FDroidProvider { QuerySelection query = new QuerySelection(selection, selectionArgs); switch (MATCHER.match(uri)) { + case CODE_REPO_APP: + List uriSegments = uri.getPathSegments(); + Long repoId = Long.parseLong(uriSegments.get(1)); + String packageName = uriSegments.get(2); + query = query.add(queryRepo(repoId)).add(queryPackage(packageName)); + break; + case CODE_LIST: break; diff --git a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java index 664ed1d08..933f44393 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java +++ b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java @@ -53,6 +53,7 @@ import org.fdroid.fdroid.localrepo.SwapService; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; +import java.util.List; import java.util.Timer; import java.util.TimerTask; @@ -233,8 +234,13 @@ public class SwapAppsView extends ListView implements private class ViewHolder { private final LocalBroadcastManager localBroadcastManager; + + @Nullable private App app; + @Nullable + private Apk apk; + ProgressBar progressView; TextView nameView; ImageView iconView; @@ -305,14 +311,19 @@ public class SwapAppsView extends ListView implements if (this.app == null || !this.app.packageName.equals(app.packageName)) { this.app = app; - Context context = getContext(); - Apk apk = ApkProvider.Helper.findApkFromAnyRepo(context, - app.packageName, app.suggestedVersionCode); - String urlString = apk.getUrl(); + List availableApks = ApkProvider.Helper.findAppVersionsByRepo(getActivity(), app, repo); + if (availableApks.size() > 0) { + // Swap repos only add one version of an app, so we will just ask for the first apk. + this.apk = availableApks.get(0); + } - // TODO unregister receivers? or will they just die with this instance - localBroadcastManager.registerReceiver(downloadReceiver, - DownloaderService.getIntentFilter(urlString)); + if (apk != null) { + String urlString = apk.getUrl(); + + // TODO unregister receivers? or will they just die with this instance + IntentFilter downloadFilter = DownloaderService.getIntentFilter(urlString); + localBroadcastManager.registerReceiver(downloadReceiver, downloadFilter); + } // NOTE: Instead of continually unregistering and re-registering the observer // (with a different URI), this could equally be done by only having one @@ -364,8 +375,8 @@ public class SwapAppsView extends ListView implements OnClickListener installListener = new OnClickListener() { @Override public void onClick(View v) { - if (app.hasUpdates() || app.compatible) { - getActivity().install(app); + if (apk != null && (app.hasUpdates() || app.compatible)) { + getActivity().install(app, apk); showProgress(); } } diff --git a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java index bccf4a796..5dcd9453e 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java +++ b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java @@ -39,7 +39,6 @@ import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Apk; -import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.NewRepoConfig; import org.fdroid.fdroid.installer.InstallManagerService; @@ -766,8 +765,7 @@ public class SwapWorkflowActivity extends AppCompatActivity { } } - public void install(@NonNull final App app) { - final Apk apk = ApkProvider.Helper.findApkFromAnyRepo(this, app.packageName, app.suggestedVersionCode); + public void install(@NonNull final App app, @NonNull final Apk apk) { Uri downloadUri = Uri.parse(apk.getUrl()); localBroadcastManager.registerReceiver(installReceiver, Installer.getInstallIntentFilter(downloadUri)); diff --git a/app/src/test/java/org/fdroid/fdroid/Assert.java b/app/src/test/java/org/fdroid/fdroid/Assert.java index a9db602e7..e54b3f9ae 100644 --- a/app/src/test/java/org/fdroid/fdroid/Assert.java +++ b/app/src/test/java/org/fdroid/fdroid/Assert.java @@ -9,6 +9,7 @@ import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.data.InstalledAppProvider; +import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.Schema.ApkTable; import org.fdroid.fdroid.data.Schema.AppMetadataTable; import org.fdroid.fdroid.data.Schema.InstalledAppTable; @@ -181,6 +182,12 @@ public class Assert { return insertApp(context, packageName, name, new ContentValues()); } + public static App insertApp(Context context, String packageName, String name, Repo repo) { + ContentValues values = new ContentValues(); + values.put(AppMetadataTable.Cols.REPO_ID, repo.getId()); + return insertApp(context, packageName, name, values); + } + public static App insertApp(Context context, String packageName, String name, ContentValues additionalValues) { ContentValues values = new ContentValues(); diff --git a/app/src/test/java/org/fdroid/fdroid/TestUtils.java b/app/src/test/java/org/fdroid/fdroid/TestUtils.java index d387c7355..29db4acd7 100644 --- a/app/src/test/java/org/fdroid/fdroid/TestUtils.java +++ b/app/src/test/java/org/fdroid/fdroid/TestUtils.java @@ -6,7 +6,10 @@ import android.content.ContentValues; import android.content.Context; import android.content.ContextWrapper; import android.content.pm.ProviderInfo; +import android.net.Uri; +import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.data.Repo; @@ -81,10 +84,14 @@ public class TestUtils { assertEquals(message, formatSigForDebugging(expected), formatSigForDebugging(actual)); } - public static void insertApk(Context context, App app, int versionCode, String signature) { + public static Apk insertApk(Context context, App app, int versionCode, String signature) { ContentValues values = new ContentValues(); values.put(Schema.ApkTable.Cols.SIGNATURE, signature); - Assert.insertApk(context, app, versionCode, values); + + long repoId = app.repoId > 0 ? app.repoId : 1; + values.put(Schema.ApkTable.Cols.REPO_ID, repoId); + Uri uri = Assert.insertApk(context, app, versionCode, values); + return ApkProvider.Helper.findByUri(context, uri, Schema.ApkTable.Cols.ALL); } public static App insertApp(Context context, String packageName, String appName, int upstreamVersionCode, diff --git a/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java b/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java index 2384949b8..3e1057e58 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java @@ -6,6 +6,7 @@ import android.database.Cursor; import android.net.Uri; import org.fdroid.fdroid.Assert; import org.fdroid.fdroid.BuildConfig; +import org.fdroid.fdroid.TestUtils; import org.fdroid.fdroid.data.Schema.ApkTable.Cols; import org.fdroid.fdroid.data.Schema.RepoTable; import org.fdroid.fdroid.mock.MockApk; @@ -271,6 +272,27 @@ public class ApkProviderTest extends FDroidProviderTest { assertBelongsToApp(thingoApks, "com.apk.thingo"); } + @Test + public void findApksForAppInSpecificRepo() { + Repo fdroidRepo = RepoProvider.Helper.findByAddress(context, "https://f-droid.org/repo"); + Repo swapRepo = RepoProviderTest.insertRepo(context, "http://192.168.1.3/fdroid/repo", "", "22", "", true); + + App officialFDroid = insertApp(context, "org.fdroid.fdroid", "F-Droid (Official)", fdroidRepo); + TestUtils.insertApk(context, officialFDroid, 4, TestUtils.FDROID_SIG); + TestUtils.insertApk(context, officialFDroid, 5, TestUtils.FDROID_SIG); + + App debugSwapFDroid = insertApp(context, "org.fdroid.fdroid", "F-Droid (Debug)", swapRepo); + TestUtils.insertApk(context, debugSwapFDroid, 6, TestUtils.THIRD_PARTY_SIG); + + List foundOfficialApks = ApkProvider.Helper.findAppVersionsByRepo(context, officialFDroid, fdroidRepo); + assertEquals(2, foundOfficialApks.size()); + + List debugSwapApks = ApkProvider.Helper.findAppVersionsByRepo(context, officialFDroid, swapRepo); + assertEquals(1, debugSwapApks.size()); + assertEquals(debugSwapFDroid.getId(), debugSwapApks.get(0).appId); + assertEquals(6, debugSwapApks.get(0).versionCode); + } + @Test public void testUpdate() { diff --git a/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java b/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java index 31baab616..cbcd865d9 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java @@ -256,11 +256,17 @@ public class RepoProviderTest extends FDroidProviderTest { public static Repo insertRepo(Context context, String address, String description, String fingerprint, @Nullable String name) { + return insertRepo(context, address, description, fingerprint, name, false); + } + + public static Repo insertRepo(Context context, String address, String description, + String fingerprint, @Nullable String name, boolean isSwap) { ContentValues values = new ContentValues(); values.put(RepoTable.Cols.ADDRESS, address); values.put(RepoTable.Cols.DESCRIPTION, description); values.put(RepoTable.Cols.FINGERPRINT, fingerprint); values.put(RepoTable.Cols.NAME, name); + values.put(RepoTable.Cols.IS_SWAP, isSwap); RepoProvider.Helper.insert(context, values); return RepoProvider.Helper.findByAddress(context, address); From 618f83bb23a15ef0c7ec7e2efa978b65eca962b4 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 14 Jul 2017 12:17:32 +1000 Subject: [PATCH 2/4] Formatting --- .../java/org/fdroid/fdroid/views/swap/SwapAppsView.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java index 933f44393..5f034476b 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java +++ b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java @@ -296,8 +296,11 @@ public class SwapAppsView extends ListView implements public void onChange(boolean selfChange) { Activity activity = getActivity(); if (activity != null) { - app = AppProvider.Helper.findSpecificApp(getActivity().getContentResolver(), - app.packageName, app.repoId, AppMetadataTable.Cols.ALL); + app = AppProvider.Helper.findSpecificApp( + getActivity().getContentResolver(), + app.packageName, + app.repoId, + AppMetadataTable.Cols.ALL); resetView(); } } From 3b41287cf7c495ef2a3ecb60136879dcdc12ac0d Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 1 Aug 2017 13:02:35 +1000 Subject: [PATCH 3/4] Remove unused methods. There are two methods which allow callers to choose which fields to return. These were originally added for performance, so you only ask for what you need. However empirically the performance gain doesn't mean anything, because it is dwarfed by the query that was just executed. However, it does open the code up to bugs because we forget to ask for the right fields. So now it just returns all fields when querying for apks. --- .../java/org/fdroid/fdroid/data/ApkProvider.java | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java index 290e6674d..2b59e139d 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java @@ -114,14 +114,10 @@ public class ApkProvider extends FDroidProvider { } public static List findByPackageName(Context context, String packageName) { - return findByPackageName(context, packageName, Cols.ALL); - } - - public static List findByPackageName(Context context, String packageName, String[] projection) { ContentResolver resolver = context.getContentResolver(); final Uri uri = getAppUri(packageName); final String sort = "apk." + Cols.VERSION_CODE + " DESC"; - Cursor cursor = resolver.query(uri, projection, null, null, sort); + Cursor cursor = resolver.query(uri, Cols.ALL, null, null, sort); return cursorToList(cursor); } @@ -153,13 +149,8 @@ public class ApkProvider extends FDroidProvider { } public static Apk get(Context context, Uri uri) { - return get(context, uri, Cols.ALL); - } - - @Nullable - public static Apk get(Context context, Uri uri, String[] fields) { ContentResolver resolver = context.getContentResolver(); - Cursor cursor = resolver.query(uri, fields, null, null, null); + Cursor cursor = resolver.query(uri, Cols.ALL, null, null, null); return cursorToApk(cursor); } From a3a0c0a15d9b550642f25b27713a01436786671d Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 1 Aug 2017 13:04:04 +1000 Subject: [PATCH 4/4] More verbose logging There are some ACRA reports with this IllegalStateException getting hit. It used to be that it was only ever because we forgot to request the correct fields from the database. However now I'm not sure that this is the only source. Perhaps it is also possible in response to parcelling apk instances, or maybe something else? Either way, this should provide further info about whether the apk doesn't belong to a repo for some reason. --- app/src/main/java/org/fdroid/fdroid/data/Apk.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index 0668c8663..6fc5ac197 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -250,8 +250,10 @@ public class Apk extends ValueObject implements Comparable, Parcelable { private void checkRepoAddress() { if (repoAddress == null || apkName == null) { - throw new IllegalStateException("Apk needs to have both Schema.ApkTable.Cols.REPO_ADDRESS and " - + "Schema.ApkTable.Cols.NAME set in order to calculate URL."); + throw new IllegalStateException( + "Apk needs to have both Schema.ApkTable.Cols.REPO_ADDRESS and " + + "Schema.ApkTable.Cols.NAME set in order to calculate URL " + + "[package: " + packageName + ", versionCode: " + versionCode + ", repoId: " + repoId + "]"); } }