From 1dde3e7f75aa7d82ead094517afaf4117b558dd3 Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Tue, 10 Oct 2023 14:58:43 +0000 Subject: Revert^2 "Fail adb install on external profile errors." Revert submission 24993368-revert-24773688-dm-profile-validation-SONEGLGSYT Reason for revert: Relanding the changes with flags Reverted changes: /q/submissionid:24993368-revert-24773688-dm-profile-validation-SONEGLGSYT Bug: 278080573 Change-Id: Ia60b7572ea9c9bafe39c63ec1ed5fe432781e04f Merged-In: I51f45cfa216b15f6c355bfba6ddc4ca620cdf0ae (cherry picked from commit e424064f12fb4fe91ba721b1a9f2277a2892c630 with modifications) --- AconfigFlags.bp | 21 ++++++++ core/java/android/content/pm/PackageInstaller.java | 7 +++ core/java/android/content/pm/flags.aconfig | 8 +++ .../java/com/android/server/pm/InstallRequest.java | 57 +++++++++++++++++----- .../android/server/pm/PackageInstallerSession.java | 4 ++ .../android/server/pm/PackageManagerService.java | 3 ++ .../server/pm/PackageManagerShellCommand.java | 17 +++++-- 7 files changed, 101 insertions(+), 16 deletions(-) create mode 100644 core/java/android/content/pm/flags.aconfig diff --git a/AconfigFlags.bp b/AconfigFlags.bp index f0b759878e20..f2cae097f518 100644 --- a/AconfigFlags.bp +++ b/AconfigFlags.bp @@ -13,6 +13,7 @@ // limitations under the License. aconfig_srcjars = [ + ":android.content.pm.flags-aconfig-java{.generated_srcjars}", ":android.hardware.biometrics.flags-aconfig-java{.generated_srcjars}", ":android.nfc.flags-aconfig-java{.generated_srcjars}", ":android.os.flags-aconfig-java{.generated_srcjars}", @@ -145,6 +146,26 @@ java_aconfig_library { defaults: ["framework-minus-apex-aconfig-java-defaults"], } +// Package Manager +aconfig_declarations { + name: "android.content.pm.flags-aconfig", + package: "android.content.pm", + srcs: ["core/java/android/content/pm/flags.aconfig"], +} + +java_aconfig_library { + name: "android.content.pm.flags-aconfig-java", + aconfig_declarations: "android.content.pm.flags-aconfig", + defaults: ["framework-minus-apex-aconfig-java-defaults"], +} + +java_aconfig_library { + name: "android.content.pm.flags-aconfig-java-host", + aconfig_declarations: "android.content.pm.flags-aconfig", + host_supported: true, + defaults: ["framework-minus-apex-aconfig-java-defaults"], +} + // Biometrics aconfig_declarations { name: "android.hardware.biometrics.flags-aconfig", diff --git a/core/java/android/content/pm/PackageInstaller.java b/core/java/android/content/pm/PackageInstaller.java index ef26235a3879..f11b1b7ee76d 100644 --- a/core/java/android/content/pm/PackageInstaller.java +++ b/core/java/android/content/pm/PackageInstaller.java @@ -344,6 +344,13 @@ public class PackageInstaller { public static final String EXTRA_RESOLVED_BASE_PATH = "android.content.pm.extra.RESOLVED_BASE_PATH"; + /** + * A list of warnings that occurred during installation. + * + * @hide + */ + public static final String EXTRA_WARNINGS = "android.content.pm.extra.WARNINGS"; + /** * Streaming installation pending. * Caller should make sure DataLoader is able to prepare image and reinitiate the operation. diff --git a/core/java/android/content/pm/flags.aconfig b/core/java/android/content/pm/flags.aconfig new file mode 100644 index 000000000000..524253371e49 --- /dev/null +++ b/core/java/android/content/pm/flags.aconfig @@ -0,0 +1,8 @@ +package: "android.content.pm" + +flag { + name: "use_art_service_v2" + namespace: "package_manager_service" + description: "Feature flag to enable the features that rely on new ART Service APIs that are in the VIC version of the ART module." + bug: "304741685" +} diff --git a/services/core/java/com/android/server/pm/InstallRequest.java b/services/core/java/com/android/server/pm/InstallRequest.java index 6fc14e814525..046dcc2dce7c 100644 --- a/services/core/java/com/android/server/pm/InstallRequest.java +++ b/services/core/java/com/android/server/pm/InstallRequest.java @@ -22,6 +22,8 @@ import static android.content.pm.PackageManager.INSTALL_SCENARIO_DEFAULT; import static android.content.pm.PackageManager.INSTALL_SUCCEEDED; import static android.os.Process.INVALID_UID; +import static com.android.server.art.model.DexoptResult.DexContainerFileDexoptResult; +import static com.android.server.art.model.DexoptResult.PackageDexoptResult; import static com.android.server.pm.PackageManagerService.SCAN_AS_INSTANT_APP; import static com.android.server.pm.PackageManagerService.TAG; @@ -30,6 +32,7 @@ import android.annotation.Nullable; import android.apex.ApexInfo; import android.app.AppOpsManager; import android.content.pm.DataLoaderType; +import android.content.pm.Flags; import android.content.pm.IPackageInstallObserver2; import android.content.pm.PackageInstaller; import android.content.pm.PackageManager; @@ -52,6 +55,7 @@ import com.android.server.pm.pkg.parsing.ParsingPackageUtils; import java.io.File; import java.util.ArrayList; +import java.util.LinkedHashSet; import java.util.List; final class InstallRequest { @@ -130,6 +134,9 @@ final class InstallRequest { private int mDexoptStatus; + @NonNull + private ArrayList mWarnings = new ArrayList<>(); + // New install InstallRequest(InstallingSession params) { mUserId = params.getUser().getIdentifier(); @@ -615,6 +622,11 @@ final class InstallRequest { return mDexoptStatus; } + @NonNull + public ArrayList getWarnings() { + return mWarnings; + } + public void setScanFlags(int scanFlags) { mScanFlags = scanFlags; } @@ -757,6 +769,10 @@ final class InstallRequest { } } + public void addWarning(@NonNull String warning) { + mWarnings.add(warning); + } + public void onPrepareStarted() { if (mPackageMetrics != null) { mPackageMetrics.onStepStarted(PackageMetrics.STEP_PREPARE); @@ -806,22 +822,37 @@ final class InstallRequest { } public void onDexoptFinished(DexoptResult dexoptResult) { - if (mPackageMetrics == null) { - return; - } - mDexoptStatus = dexoptResult.getFinalStatus(); - if (mDexoptStatus != DexoptResult.DEXOPT_PERFORMED) { - return; + // Only report external profile warnings when installing from adb. The goal is to warn app + // developers if they have provided bad external profiles, so it's not beneficial to report + // those warnings in the normal app install workflow. + if (isInstallFromAdb() && Flags.useArtServiceV2()) { + var externalProfileErrors = new LinkedHashSet(); + for (PackageDexoptResult packageResult : dexoptResult.getPackageDexoptResults()) { + for (DexContainerFileDexoptResult fileResult : + packageResult.getDexContainerFileDexoptResults()) { + externalProfileErrors.addAll(fileResult.getExternalProfileErrors()); + } + } + if (!externalProfileErrors.isEmpty()) { + addWarning("Error occurred during dexopt when processing external profiles:\n " + + String.join("\n ", externalProfileErrors)); + } } - long durationMillis = 0; - for (DexoptResult.PackageDexoptResult packageResult : - dexoptResult.getPackageDexoptResults()) { - for (DexoptResult.DexContainerFileDexoptResult fileResult : - packageResult.getDexContainerFileDexoptResults()) { - durationMillis += fileResult.getDex2oatWallTimeMillis(); + + // Report dexopt metrics. + if (mPackageMetrics != null) { + mDexoptStatus = dexoptResult.getFinalStatus(); + if (mDexoptStatus == DexoptResult.DEXOPT_PERFORMED) { + long durationMillis = 0; + for (PackageDexoptResult packageResult : dexoptResult.getPackageDexoptResults()) { + for (DexContainerFileDexoptResult fileResult : + packageResult.getDexContainerFileDexoptResults()) { + durationMillis += fileResult.getDex2oatWallTimeMillis(); + } + } + mPackageMetrics.onStepFinished(PackageMetrics.STEP_DEXOPT, durationMillis); } } - mPackageMetrics.onStepFinished(PackageMetrics.STEP_DEXOPT, durationMillis); } public void onInstallCompleted() { diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index f0e38955f050..4157066f7e7f 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -4904,6 +4904,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { if (!TextUtils.isEmpty(existing)) { fillIn.putExtra(PackageInstaller.EXTRA_OTHER_PACKAGE_NAME, existing); } + ArrayList warnings = extras.getStringArrayList(PackageInstaller.EXTRA_WARNINGS); + if (!ArrayUtils.isEmpty(warnings)) { + fillIn.putStringArrayListExtra(PackageInstaller.EXTRA_WARNINGS, warnings); + } } try { final BroadcastOptions options = BroadcastOptions.makeBasic(); diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index f2b62eaf25f8..20bd56d56945 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -1415,6 +1415,9 @@ public class PackageManagerService implements PackageSender, TestUtilityService break; } } + if (!request.getWarnings().isEmpty()) { + extras.putStringArrayList(PackageInstaller.EXTRA_WARNINGS, request.getWarnings()); + } return extras; } diff --git a/services/core/java/com/android/server/pm/PackageManagerShellCommand.java b/services/core/java/com/android/server/pm/PackageManagerShellCommand.java index 2a7ebfb75dde..2327b85ed0bb 100644 --- a/services/core/java/com/android/server/pm/PackageManagerShellCommand.java +++ b/services/core/java/com/android/server/pm/PackageManagerShellCommand.java @@ -3982,10 +3982,21 @@ class PackageManagerShellCommand extends ShellCommand { session.commit(receiver.getIntentSender()); if (!session.isStaged()) { final Intent result = receiver.getResult(); - final int status = result.getIntExtra(PackageInstaller.EXTRA_STATUS, - PackageInstaller.STATUS_FAILURE); + int status = result.getIntExtra( + PackageInstaller.EXTRA_STATUS, PackageInstaller.STATUS_FAILURE); + List warnings = + result.getStringArrayListExtra(PackageInstaller.EXTRA_WARNINGS); if (status == PackageInstaller.STATUS_SUCCESS) { - if (logSuccess) { + if (!ArrayUtils.isEmpty(warnings)) { + // Don't start the output string with "Success" because that will make adb + // treat this as a success. + for (String warning : warnings) { + pw.println("Warning: " + warning); + } + // Treat warnings as failure to draw app developers' attention. + status = PackageInstaller.STATUS_FAILURE; + pw.println("Completed with warning(s)"); + } else if (logSuccess) { pw.println("Success"); } } else { -- cgit v1.2.3-59-g8ed1b