Skip to content
Snippets Groups Projects

Tests and slight improvements for multi-sig support when calculating suggested versions.

Merged username-removed-25042 requested to merge pserwylo/fdroidclient:multi-sig-tests into master
All threads resolved!

Working towards multi-sig support. This adds tests and makes a change to the suggested version code query to ensure that apks with a different signature to the installed app do not end up being "suggested" to the user.

It also drops the primary key from fdroid_apk which used to be a composite of appId + versionCode + repoId. This is no longer relevant or correct, so instead it now just uses the default rowid provided by sqlite for the primary key.

Note that although the tests work, the actual client will appear broken due to #1052 (closed). I don't think that needs to stop this being reviewed and merged though.

Related to but not sufficient to complete #740 (closed).

Things that still need doing which are broken with multi-sig (but which are not made any worse by this MR) are:

  • When touching "Install" in AppDetails, it does the following to get the apk to install: ApkProvider.Helper.findApkFromAnyRepo(this, app.packageName, app.suggestedVersionCode);. Each place where there is an assumption that packageName + app.suggestedVersionCode is enough to find the suggested apk is no longer suitable. They also need to take into account ``app.installedSig` or something.
Edited by username-removed-25042

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • changed milestone to %0.104

  • GitLab is playing funny-buggers and I can't see your comments sometimes. However, I think that there is not a need for a special case in InstalledAppProviderService. I believe this code will always pick up the new version of F-Droid:

        public static void compareToPackageManager(Context context) {
            Map<String, Long> cachedInfo = InstalledAppProvider.Helper.all(context);
    
            List<PackageInfo> packageInfoList = context.getPackageManager()
                    .getInstalledPackages(PackageManager.GET_SIGNATURES);
            for (PackageInfo packageInfo : packageInfoList) {
                if (cachedInfo.containsKey(packageInfo.packageName)) {
                    if (packageInfo.lastUpdateTime < 1262300400000L // 2010-01-01 00:00
                            || packageInfo.lastUpdateTime > cachedInfo.get(packageInfo.packageName)) {
                        insert(context, packageInfo);
                    }
                    cachedInfo.remove(packageInfo.packageName);
                } else {
                    insert(context, packageInfo);
                }
            }
    
            for (String packageName : cachedInfo.keySet()) {
                delete(context, packageName);
            }
        }

    This is what happens:

    • F-Droid will be in cachedInfo with an update time of the previous time it was updated.
    • packageInfo.lastUpdateTime will be > cachedInfo.get("org.fdroid.fdroid")
    • Thus insert(context, packageInfo) will always get called.

    Does that sound right?

  • Yeah that makes sense. Then I guess my understanding of InstalledAppProviderService.insert() wasn't quite right. It looks like it is updating existing database entries rather than creating duplicates.

  • Bah, my earlier comment was actually for !530 (merged), got confused sorry.

  • Then I guess my understanding wasn't quite right.

    You were correct. The actual content provider InstalledAppProvider.insert() method (not the service) does a db().replaceOrThrow() which is effectively "attempt to insert, if the primary key already exists, delete it and reinsert it" in a single SQL statement.

  • Oh wow, this is really really really slow. If you go to preferences and then enable "Unstable updates" it will take about 40 seconds on my Moto X 2nd gen to do that query. I'm going to mark this as WIP until I get that fixed.

  • username-removed-25042 marked as a Work In Progress

    marked as a Work In Progress

  • username-removed-25042 changed the description

    changed the description

  • username-removed-25042 unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Okay, that took longer than I'd hoped, but it turns out:

    • The fix for !497 (merged) actually introduced some huge slowness to start with (i.e. 30 seconds extra, just to calculate the suggested vercode).
    • Thus, the first commit in this branch addresses that by removing a subquery and replacing with a JOIN. Due to the correct usage of DB indexes, this reduces the time back down from 30s to 100ms.
    • Then, the following commits do what you have already reviewed:
      • Add a further check for suggested version, which takes into account the installed app (if any) sig.
      • This increases the time from 100ms back up to 50s or so.
    • A subsequent commit then does a bigger refactor, changing the join from fdroid_installedApp -> fdroid_package via the integer primary key of the package table, rather than the string package name.
      • This drops the time from ~50s to ~30s (still completely unacceptable, but a good improvement for the future).
    • Finally, after investigating, I figured out that the temporary apk table used by the updater only has an index on VERSION_CODE, which is terrible for performance.
      • This removes that index, and replaces it with an index on APP_ID which drops the time from ~30s back to ~100ms.

    Happy to go into the details of why this is the case if you'd like, just let me know.

    Edited by username-removed-25042
  • mentioned in merge request !536 (merged)

  • added apksig ~16415 labels

  • This is working for me, at least in terms of not crashing when index-v1.jar contains APKs with matching packageName and versionCode, but different signatures. They show up fine too, and I can install them. But updating gave me the old "different signature" error. I'll try again with !536 (merged).

  • username-removed-24982 resolved all discussions

    resolved all discussions

  • mentioned in merge request !537 (merged)

  • Please register or sign in to reply
    Loading