Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6a422d6a authored by Bernardo Rufino's avatar Bernardo Rufino
Browse files

[KV] Consider throwing BackupAgent a failure 

And not a success as it used to be.

Bug: 111051813
Bug: 110082831
Test: atest KeyValueBackupTaskTest
Test: 1. BackupApp throwing in onBackup.
      2. adb shell bmgr backupnow com.google.android.apps.backupapp
      3. Verify logs and that it threw and we did not save backup data for it.
Test: 1. BackupApp not throwing.
      2. adb shell bmgr backupnow com.google.android.apps.backupapp
      3. Verify logs and that data was sent to transport.

Change-Id: Idb7fe298f64786668989c30cdce53355aeef7277
parent 47b3cf93

core/java/android/app/backup/BackupAgent.java

+8 −1
Original line number Diff line number Diff line
@@ -125,6 +125,11 @@ public abstract class BackupAgent extends ContextWrapper { 
    private static final String TAG = "BackupAgent";

    private static final boolean DEBUG = false;



    /** @hide */

    public static final int RESULT_SUCCESS = 0;

    /** @hide */

    public static final int RESULT_ERROR = -1;



    /** @hide */

    public static final int TYPE_EOF = 0;
@@ -955,8 +960,10 @@ public abstract class BackupAgent extends ContextWrapper { 
            BackupDataOutput output = new BackupDataOutput(

                    data.getFileDescriptor(), quotaBytes, transportFlags);



            long result = RESULT_ERROR;

            try {

                BackupAgent.this.onBackup(oldState, output, newState);

                result = RESULT_SUCCESS;

            } catch (IOException ex) {

                Log.d(TAG, "onBackup (" + BackupAgent.this.getClass().getName() + ") threw", ex);

                throw new RuntimeException(ex);
@@ -971,7 +978,7 @@ public abstract class BackupAgent extends ContextWrapper { 


                Binder.restoreCallingIdentity(ident);

                try {

                    callbackBinder.operationComplete(0);

                    callbackBinder.operationComplete(result);

                } catch (RemoteException e) {

                    // we'll time out anyway, so we're safe

                }

services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupReporter.java

+10 −4
Original line number Diff line number Diff line
@@ -52,11 +52,9 @@ import java.util.List; 
//       verify calls to this object. Add these and more assertions to the test of this class.

@VisibleForTesting

public class KeyValueBackupReporter {

    @VisibleForTesting

    static final String TAG = "KeyValueBackupTask";

    @VisibleForTesting static final String TAG = "KeyValueBackupTask";

    private static final boolean DEBUG = BackupManagerService.DEBUG;

    @VisibleForTesting

    static final boolean MORE_DEBUG = BackupManagerService.MORE_DEBUG || true;

    @VisibleForTesting static final boolean MORE_DEBUG = BackupManagerService.MORE_DEBUG || true;



    static void onNewThread(String threadName) {

        if (DEBUG) {
@@ -357,6 +355,14 @@ public class KeyValueBackupReporter { 
                                null, BackupManagerMonitor.EXTRA_LOG_CANCEL_ALL, true));

    }



    void onAgentResultError(@Nullable PackageInfo packageInfo) {

        String packageName = getPackageName(packageInfo);

        BackupObserverUtils.sendBackupOnPackageResult(

                mObserver, packageName, BackupManager.ERROR_AGENT_FAILURE);

        EventLog.writeEvent(EventLogTags.BACKUP_AGENT_FAILURE, packageName, "result error");

        Slog.w(TAG, "Agent " + packageName + " error in onBackup()");

    }



    private String getPackageName(@Nullable PackageInfo packageInfo) {

        return (packageInfo != null) ? packageInfo.packageName : "no_package_yet";

    }

services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java

+27 −27
Original line number Diff line number Diff line
@@ -251,15 +251,15 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { 
    @Nullable private final DataChangedJournal mJournal;

    @Nullable private PerformFullTransportBackupTask mFullBackupTask;



    private IBackupAgent mAgentBinder;

    private PackageInfo mCurrentPackage;

    private File mSavedStateFile;

    private File mBackupDataFile;

    private File mNewStateFile;

    private ParcelFileDescriptor mSavedState;

    private ParcelFileDescriptor mBackupData;

    private ParcelFileDescriptor mNewState;

    private int mStatus;

    @Nullable private IBackupAgent mAgentBinder;

    @Nullable private PackageInfo mCurrentPackage;

    @Nullable private File mSavedStateFile;

    @Nullable private File mBackupDataFile;

    @Nullable private File mNewStateFile;

    @Nullable private ParcelFileDescriptor mSavedState;

    @Nullable private ParcelFileDescriptor mBackupData;

    @Nullable private ParcelFileDescriptor mNewState;



    /**

     * This {@link ConditionVariable} is used to signal that the cancel operation has been
@@ -344,23 +344,32 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { 
    }



    /** Returns whether to consume next queue package. */

    private boolean handleAgentResult(RemoteResult result) {

    private boolean handleAgentResult(@Nullable PackageInfo packageInfo, RemoteResult result) {

        if (result == RemoteResult.FAILED_THREAD_INTERRUPTED) {

            // Not an explicit cancel, we need to flag it.

            mCancelled = true;

            handleAgentCancelled();

            mReporter.onAgentCancelled(packageInfo);

            errorCleanup();

            return false;

        }

        if (result == RemoteResult.FAILED_CANCELLED) {

            handleAgentCancelled();

            mReporter.onAgentCancelled(packageInfo);

            errorCleanup();

            return false;

        }

        if (result == RemoteResult.FAILED_TIMED_OUT) {

            handleAgentTimeout();

            mReporter.onAgentTimedOut(packageInfo);

            errorCleanup();

            return true;

        }

        Preconditions.checkState(result.succeeded());

        return sendDataToTransport(result.get());

        Preconditions.checkState(result.isPresent());

        long agentResult = result.get();

        if (agentResult == BackupAgent.RESULT_ERROR) {

            mReporter.onAgentResultError(packageInfo);

            errorCleanup();

            return true;

        }

        return sendDataToTransport();

    }



    @Override
@@ -463,7 +472,7 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { 
        }



        Preconditions.checkNotNull(agentResult);

        return handleAgentResult(agentResult);

        return handleAgentResult(mCurrentPackage, agentResult);

    }



    /** Returns whether to consume next queue package. */
@@ -543,7 +552,8 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { 
            return false;

        }



        return handleAgentResult(agentResult);

        Preconditions.checkNotNull(agentResult);

        return handleAgentResult(mCurrentPackage, agentResult);

    }



    private void finishTask() {
@@ -790,7 +800,7 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { 
    }



    /** Returns whether to consume next queue package. */

    private boolean sendDataToTransport(long agentResult) {

    private boolean sendDataToTransport() {

        Preconditions.checkState(mBackupData != null);



        String packageName = mCurrentPackage.packageName;
@@ -994,16 +1004,6 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { 
        mCancelAcknowledged.block();

    }



    private void handleAgentTimeout() {

        mReporter.onAgentTimedOut(mCurrentPackage);

        errorCleanup();

    }



    private void handleAgentCancelled() {

        mReporter.onAgentCancelled(mCurrentPackage);

        errorCleanup();

    }



    private void revertTask() {

        mReporter.onRevertTask();

        long delay;

services/backup/java/com/android/server/backup/remote/FutureBackupCallback.java

+2 −2
Original line number Diff line number Diff line
@@ -23,7 +23,7 @@ import java.util.concurrent.CompletableFuture; 


/**

 * An implementation of {@link IBackupCallback} that completes the {@link CompletableFuture}

 * provided in the constructor with a successful {@link RemoteResult}.

 * provided in the constructor with a present {@link RemoteResult}.

 */

public class FutureBackupCallback extends IBackupCallback.Stub {

    private final CompletableFuture<RemoteResult> mFuture;
@@ -34,6 +34,6 @@ public class FutureBackupCallback extends IBackupCallback.Stub { 


    @Override

    public void operationComplete(long result) throws RemoteException {

        mFuture.complete(RemoteResult.successful(result));

        mFuture.complete(RemoteResult.of(result));

    }

}

services/backup/java/com/android/server/backup/remote/RemoteCall.java

+1 −1
Original line number Diff line number Diff line
@@ -83,7 +83,7 @@ public class RemoteCall { 
     *

     * <ul>

     *   <li>The callback passed to {@link RemoteCallable} is called with the result. We return a

     *       successful {@link RemoteResult} with the result.

     *       present {@link RemoteResult} with the result.

     *   <li>Time-out happens. We return {@link RemoteResult#FAILED_TIMED_OUT}.

     *   <li>Someone calls {@link #cancel()} on this object. We return {@link

     *       RemoteResult#FAILED_CANCELLED}.