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

Commit 4188d8f8 authored by Ruslan Tkhakokhov's avatar Ruslan Tkhakokhov
Browse files

Do not write backup enabled status to file on init 

 When backup service is brought up, we read the enabled status
from a file and store it in a class variable in
UserBackupManagerService. However, we also write the value we just read
back into the file. This is an unncessary operation and it makes the
code more fragile: if we read the wrong value due to some I/O error,
we'll then write it back making the error permanent.

More details here: https://docs.google.com/document/d/1GtR2HYMiSJuy0wuxBg8n_zkEJGl3C5apcvpvKheNa_A/edit

Bug: 149084461
Test: 1. "bmgr enable true"; verify "bmgr enabled" returns true; reboot; verify "bmgr enabled" returns true
      2. "bmgr enable false"; verify "bmgr enabled" returns false; reboot; verify "bmgr enabled" returns false
      3. atest RunBackupFrameworksServicesRoboTests
      4. atest FrameworksServicesTests:UserBackupManagerServiceTest
Change-Id: If213a6b1de0de51c9337138be347c304b44689d1
parent 0145f7e5

services/backup/java/com/android/server/backup/UserBackupManagerService.java

+103 −51
Original line number Diff line number Diff line
@@ -157,7 +157,6 @@ import java.util.HashMap; 
import java.util.HashSet;

import java.util.LinkedList;

import java.util.List;

import java.util.Map;

import java.util.Objects;

import java.util.Queue;

import java.util.Random;
@@ -316,7 +315,7 @@ public class UserBackupManagerService { 


    private final IBackupManager mBackupManagerBinder;



    private boolean mEnabled;   // access to this is synchronized on 'this'

    private boolean mEnabled;   // writes to this are synchronized on 'this'

    private boolean mSetupComplete;

    private boolean mAutoRestore;
@@ -508,6 +507,36 @@ public class UserBackupManagerService { 
                != 0;

    }



    @VisibleForTesting

    UserBackupManagerService(Context context) {

        mContext = context;



        mUserId = 0;

        mRegisterTransportsRequestedTime = 0;



        mBaseStateDir = null;

        mDataDir = null;

        mJournalDir = null;

        mFullBackupScheduleFile = null;

        mSetupObserver = null;

        mRunInitReceiver = null;

        mRunInitIntent = null;

        mAgentTimeoutParameters = null;

        mTransportManager = null;

        mPackageManager = null;

        mActivityManagerInternal = null;

        mAlarmManager = null;

        mConstants = null;

        mWakelock = null;

        mBackupHandler = null;

        mBackupPreferences = null;

        mBackupPasswordManager = null;

        mPackageManagerBinder = null;

        mActivityManager = null;

        mStorageManager = null;

        mBackupManagerBinder = null;

    }



    private UserBackupManagerService(

            @UserIdInt int userId,

            Context context,
@@ -627,9 +656,11 @@ public class UserBackupManagerService { 
        initPackageTracking();

    }



    @VisibleForTesting

    void initializeBackupEnableState() {

        boolean isEnabled = UserBackupManagerFilePersistedSettings.readBackupEnableState(mUserId);

        setBackupEnabled(isEnabled);

        boolean isEnabled = readEnabledState();

        // Don't persist value to disk since we just read it from there.

        setBackupEnabled(isEnabled, /* persistToDisk */ false);

    }



    /** Cleans up state when the user of this service is stopped. */
@@ -2855,6 +2886,10 @@ public class UserBackupManagerService { 


    /** User-configurable enabling/disabling of backups. */

    public void setBackupEnabled(boolean enable) {

        setBackupEnabled(enable, /* persistToDisk */ true);

    }



    private void setBackupEnabled(boolean enable, boolean persistToDisk) {

        mContext.enforceCallingOrSelfPermission(android.Manifest.permission.BACKUP,

                "setBackupEnabled");
@@ -2864,10 +2899,20 @@ public class UserBackupManagerService { 
        try {

            boolean wasEnabled = mEnabled;

            synchronized (this) {

                UserBackupManagerFilePersistedSettings.writeBackupEnableState(mUserId, enable);

                if (persistToDisk) {

                    writeEnabledState(enable);

                }

                mEnabled = enable;

            }



            updateStateOnBackupEnabled(wasEnabled, enable);

        } finally {

            Binder.restoreCallingIdentity(oldId);

        }

    }



    @VisibleForTesting

    void updateStateOnBackupEnabled(boolean wasEnabled, boolean enable) {

        synchronized (mQueueLock) {

            if (enable && !wasEnabled && mSetupComplete) {

                // if we've just been enabled, start scheduling backup passes
@@ -2915,9 +2960,16 @@ public class UserBackupManagerService { 
                }

            }

        }

        } finally {

            Binder.restoreCallingIdentity(oldId);

    }



    @VisibleForTesting

    void writeEnabledState(boolean enable) {

        UserBackupManagerFilePersistedSettings.writeBackupEnableState(mUserId, enable);

    }



    @VisibleForTesting

    boolean readEnabledState() {

        return UserBackupManagerFilePersistedSettings.readBackupEnableState(mUserId);

    }



    /** Enable/disable automatic restore of app data at install time. */

services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java

0 → 100644
+80 −0
Original line number Diff line number Diff line 
/*

 * Copyright (C) 2020 The Android Open Source Project

 *

 * Licensed under the Apache License, Version 2.0 (the "License");

 * you may not use this file except in compliance with the License.

 * You may obtain a copy of the License at

 *

 *      http://www.apache.org/licenses/LICENSE-2.0

 *

 * Unless required by applicable law or agreed to in writing, software

 * distributed under the License is distributed on an "AS IS" BASIS,

 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

 * See the License for the specific language governing permissions and

 * limitations under the License.

 */



package com.android.server.backup;



import static com.google.common.truth.Truth.assertThat;



import android.content.Context;

import android.platform.test.annotations.Presubmit;



import androidx.test.runner.AndroidJUnit4;



import org.junit.Before;

import org.junit.Test;

import org.junit.runner.RunWith;

import org.mockito.Mock;

import org.mockito.MockitoAnnotations;



@Presubmit

@RunWith(AndroidJUnit4.class)

public class UserBackupManagerServiceTest {

    @Mock Context mContext;



    private TestBackupService mService;



    @Before

    public void setUp() {

        MockitoAnnotations.initMocks(this);



        mService = new TestBackupService(mContext);

    }



    @Test

    public void initializeBackupEnableState_doesntWriteStateToDisk() {

        mService.initializeBackupEnableState();



        assertThat(mService.isEnabledStatePersisted).isFalse();

    }



    @Test

    public void updateBackupEnableState_writesStateToDisk() {

        mService.setBackupEnabled(true);



        assertThat(mService.isEnabledStatePersisted).isTrue();

    }



    private static class TestBackupService extends UserBackupManagerService {

        boolean isEnabledStatePersisted = false;



        TestBackupService(Context context) {

            super(context);

        }



        @Override

        void writeEnabledState(boolean enable) {

            isEnabledStatePersisted = true;

        }



        @Override

        boolean readEnabledState() {

            return false;

        }



        @Override

        void updateStateOnBackupEnabled(boolean wasEnabled, boolean enable) {}

    }

}