Merge "DataLoader lifecycle." into rvc-dev

services/core/java/com/android/server/pm/DataLoaderManagerService.java

@@ -204,6 +204,12 @@ public class DataLoaderManagerService extends SystemService {
 
         @Override
         public void onServiceDisconnected(ComponentName arg0) {
+            if (mListener != null) {
+                try {
+                    mListener.onStatusChanged(mId, IDataLoaderStatusListener.DATA_LOADER_DESTROYED);
+                } catch (RemoteException ignored) {
+                }
+            }
             remove();
         }
 

services/incremental/IncrementalService.cpp

@@ -258,10 +258,6 @@ IncrementalService::IncrementalService(ServiceManagerWrapper&& sm, std::string_v
     mountExistingImages();
 }
 
-FileId IncrementalService::idFromMetadata(std::span<const uint8_t> metadata) {
-    return IncFs_FileIdFromMetadata({(const char*)metadata.data(), metadata.size()});
-}
-
 IncrementalService::~IncrementalService() {
     {
         std::lock_guard lock(mJobMutex);
@@ -1016,7 +1012,7 @@ bool IncrementalService::startLoading(StorageId storage) const {
             return false;
         }
     }
-    return dataLoaderStub->start();
+    return dataLoaderStub->requestStart();
 }
 
 void IncrementalService::mountExistingImages() {
@@ -1475,12 +1471,15 @@ void IncrementalService::onAppOpChanged(const std::string& packageName) {
 }
 
 IncrementalService::DataLoaderStub::~DataLoaderStub() {
-    CHECK(mStatus == -1 || mStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED)
+    waitForDestroy();
-            << "Dataloader has to be destroyed prior to destructor: " << mId
-            << ", status: " << mStatus;
 }
 
 bool IncrementalService::DataLoaderStub::create() {
+    {
+        std::unique_lock lock(mStatusMutex);
+        mStartRequested = false;
+        mDestroyRequested = false;
+    }
     bool created = false;
     auto status = mService.mDataLoaderManager->initializeDataLoader(mId, mParams, mControl, this,
                                                                     &created);
@@ -1491,12 +1490,18 @@ bool IncrementalService::DataLoaderStub::create() {
     return true;
 }
 
-bool IncrementalService::DataLoaderStub::start() {
+bool IncrementalService::DataLoaderStub::requestStart() {
-    if (mStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
+    {
+        std::unique_lock lock(mStatusMutex);
         mStartRequested = true;
+        if (mStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
             return true;
         }
+    }
+    return start();
+}
 
+bool IncrementalService::DataLoaderStub::start() {
     sp<IDataLoader> dataloader;
     auto status = mService.mDataLoaderManager->getDataLoader(mId, &dataloader);
     if (!status.isOk()) {
@@ -1513,8 +1518,21 @@ bool IncrementalService::DataLoaderStub::start() {
 }
 
 void IncrementalService::DataLoaderStub::destroy() {
+    {
+        std::unique_lock lock(mStatusMutex);
         mDestroyRequested = true;
+    }
     mService.mDataLoaderManager->destroyDataLoader(mId);
+
+    waitForDestroy();
+}
+
+bool IncrementalService::DataLoaderStub::waitForDestroy() {
+    auto now = std::chrono::steady_clock::now();
+    std::unique_lock lock(mStatusMutex);
+    return mStatusCondition.wait_until(lock, now + 60s, [this] {
+        return mStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
+    });
 }
 
 binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mountId, int newStatus) {
@@ -1523,34 +1541,36 @@ binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mount
     }
 
     if (mListener) {
-        // Give an external listener a chance to act before we destroy something.
         mListener->onStatusChanged(mountId, newStatus);
     }
 
+    bool startRequested;
+    bool destroyRequested;
     {
-        std::unique_lock l(mService.mLock);
+        std::unique_lock lock(mStatusMutex);
-        const auto& ifs = mService.getIfsLocked(mountId);
+        if (mStatus == newStatus) {
-        if (!ifs) {
-            LOG(WARNING) << "Received data loader status " << int(newStatus)
-                         << " for unknown mount " << mountId;
             return binder::Status::ok();
         }
-        mStatus = newStatus;
 
-        if (!mDestroyRequested && newStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED) {
+        startRequested = mStartRequested;
-            mService.deleteStorageLocked(*ifs, std::move(l));
+        destroyRequested = mDestroyRequested;
-            return binder::Status::ok();
+
-        }
+        mStatus = newStatus;
     }
 
     switch (newStatus) {
         case IDataLoaderStatusListener::DATA_LOADER_CREATED: {
-            if (mStartRequested) {
+            if (startRequested) {
+                LOG(WARNING) << "Start was requested, triggering, for mount: " << mountId;
                 start();
             }
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_DESTROYED: {
+            if (!destroyRequested) {
+                LOG(WARNING) << "DataLoader destroyed, reconnecting, for mount: " << mountId;
+                create();
+            }
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_STARTED: {
@@ -1589,4 +1609,8 @@ binder::Status IncrementalService::IncrementalServiceConnector::setStorageParams
     return binder::Status::ok();
 }
 
+FileId IncrementalService::idFromMetadata(std::span<const uint8_t> metadata) {
+    return IncFs_FileIdFromMetadata({(const char*)metadata.data(), metadata.size()});
+}
+
 } // namespace android::incremental

services/incremental/IncrementalService.h

@@ -180,27 +180,32 @@ private:
         ~DataLoaderStub();
 
         bool create();
-        bool start();
+        bool requestStart();
         void destroy();
 
         // accessors
         MountId id() const { return mId; }
         const DataLoaderParamsParcel& params() const { return mParams; }
-        int status() const { return mStatus.load(); }
+        int status() const { return mStatus; }
         bool startRequested() const { return mStartRequested; }
 
     private:
         binder::Status onStatusChanged(MountId mount, int newStatus) final;
 
+        bool start();
+        bool waitForDestroy();
+
         IncrementalService& mService;
         MountId const mId;
         DataLoaderParamsParcel const mParams;
         FileSystemControlParcel const mControl;
         DataLoaderStatusListener const mListener;
 
-        std::atomic<int> mStatus = -1;
+        std::mutex mStatusMutex;
+        std::condition_variable mStatusCondition;
+        int mStatus = IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
         bool mStartRequested = false;
-        bool mDestroyRequested = false;
+        bool mDestroyRequested = true;
     };
     using DataLoaderStubPtr = sp<DataLoaderStub>;
 

services/incremental/test/IncrementalServiceTest.cpp

@@ -103,25 +103,34 @@ private:
     TemporaryFile logFile;
 };
 
-class FakeDataLoader : public IDataLoader {
+class MockDataLoader : public IDataLoader {
 public:
+    MockDataLoader() {
+        ON_CALL(*this, create(_, _, _, _)).WillByDefault(Return((binder::Status::ok())));
+        ON_CALL(*this, start(_)).WillByDefault(Return((binder::Status::ok())));
+        ON_CALL(*this, stop(_)).WillByDefault(Return((binder::Status::ok())));
+        ON_CALL(*this, destroy(_)).WillByDefault(Return((binder::Status::ok())));
+        ON_CALL(*this, prepareImage(_, _, _)).WillByDefault(Return((binder::Status::ok())));
+    }
     IBinder* onAsBinder() override { return nullptr; }
-    binder::Status create(int32_t, const DataLoaderParamsParcel&, const FileSystemControlParcel&,
+    MOCK_METHOD4(create,
-                          const sp<IDataLoaderStatusListener>&) override {
+                 binder::Status(int32_t id, const DataLoaderParamsParcel& params,
-        return binder::Status::ok();
+                                const FileSystemControlParcel& control,
-    }
+                                const sp<IDataLoaderStatusListener>& listener));
-    binder::Status start(int32_t) override { return binder::Status::ok(); }
+    MOCK_METHOD1(start, binder::Status(int32_t id));
-    binder::Status stop(int32_t) override { return binder::Status::ok(); }
+    MOCK_METHOD1(stop, binder::Status(int32_t id));
-    binder::Status destroy(int32_t) override { return binder::Status::ok(); }
+    MOCK_METHOD1(destroy, binder::Status(int32_t id));
-    binder::Status prepareImage(int32_t,
+    MOCK_METHOD3(prepareImage,
-                                const std::vector<InstallationFileParcel>&,
+                 binder::Status(int32_t id, const std::vector<InstallationFileParcel>& addedFiles,
-                                const std::vector<std::string>&) override {
+                                const std::vector<std::string>& removedFiles));
-        return binder::Status::ok();
-    }
 };
 
 class MockDataLoaderManager : public DataLoaderManagerWrapper {
 public:
+    MockDataLoaderManager(sp<IDataLoader> dataLoader) : mDataLoaderHolder(std::move(dataLoader)) {
+        EXPECT_TRUE(mDataLoaderHolder != nullptr);
+    }
+
     MOCK_CONST_METHOD5(initializeDataLoader,
                        binder::Status(int32_t mountId, const DataLoaderParamsParcel& params,
                                       const FileSystemControlParcel& control,
@@ -144,9 +153,9 @@ public:
         ON_CALL(*this, getDataLoader(_, _))
                 .WillByDefault(Invoke(this, &MockDataLoaderManager::getDataLoaderOk));
     }
-    void destroyDataLoaderOk() {
+    void destroyDataLoaderSuccess() {
         ON_CALL(*this, destroyDataLoader(_))
-                .WillByDefault(Invoke(this, &MockDataLoaderManager::setDataLoaderStatusDestroyed));
+                .WillByDefault(Invoke(this, &MockDataLoaderManager::destroyDataLoaderOk));
     }
     binder::Status initializeDataLoaderOk(int32_t mountId, const DataLoaderParamsParcel& params,
                                           const FileSystemControlParcel& control,
@@ -155,20 +164,30 @@ public:
         mId = mountId;
         mListener = listener;
         mServiceConnector = control.service;
+        mDataLoader = mDataLoaderHolder;
         *_aidl_return = true;
-        return binder::Status::ok();
+        return mDataLoader->create(mountId, params, control, listener);
     }
     binder::Status getDataLoaderOk(int32_t mountId, sp<IDataLoader>* _aidl_return) {
         *_aidl_return = mDataLoader;
         return binder::Status::ok();
     }
-    void setDataLoaderStatusNotReady() {
+    void setDataLoaderStatusCreated() {
-        mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
-    }
-    void setDataLoaderStatusReady() {
         mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_CREATED);
     }
-    binder::Status setDataLoaderStatusDestroyed(int32_t id) {
+    void setDataLoaderStatusStarted() {
+        mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_STARTED);
+    }
+    void setDataLoaderStatusDestroyed() {
+        mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+    }
+    binder::Status destroyDataLoaderOk(int32_t id) {
+        if (mDataLoader) {
+            if (auto status = mDataLoader->destroy(id); !status.isOk()) {
+                return status;
+            }
+            mDataLoader = nullptr;
+        }
         if (mListener) {
             mListener->onStatusChanged(id, IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
         }
@@ -185,7 +204,8 @@ private:
     int mId;
     sp<IDataLoaderStatusListener> mListener;
     sp<IIncrementalServiceConnector> mServiceConnector;
-    sp<IDataLoader> mDataLoader = sp<IDataLoader>(new FakeDataLoader());
+    sp<IDataLoader> mDataLoader;
+    sp<IDataLoader> mDataLoaderHolder;
 };
 
 class MockIncFs : public IncFsWrapper {
@@ -303,7 +323,9 @@ public:
     void SetUp() override {
         auto vold = std::make_unique<NiceMock<MockVoldService>>();
         mVold = vold.get();
-        auto dataloaderManager = std::make_unique<NiceMock<MockDataLoaderManager>>();
+        sp<NiceMock<MockDataLoader>> dataLoader{new NiceMock<MockDataLoader>};
+        mDataLoader = dataLoader.get();
+        auto dataloaderManager = std::make_unique<NiceMock<MockDataLoaderManager>>(dataLoader);
         mDataLoaderManager = dataloaderManager.get();
         auto incFs = std::make_unique<NiceMock<MockIncFs>>();
         mIncFs = incFs.get();
@@ -321,7 +343,7 @@ public:
                                                      mRootDir.path);
         mDataLoaderParcel.packageName = "com.test";
         mDataLoaderParcel.arguments = "uri";
-        mDataLoaderManager->destroyDataLoaderOk();
+        mDataLoaderManager->destroyDataLoaderSuccess();
         mIncrementalService->onSystemReady();
     }
 
@@ -352,6 +374,7 @@ protected:
     NiceMock<MockDataLoaderManager>* mDataLoaderManager;
     NiceMock<MockAppOpsManager>* mAppOpsManager;
     NiceMock<MockJniWrapper>* mJni;
+    NiceMock<MockDataLoader>* mDataLoader;
     std::unique_ptr<IncrementalService> mIncrementalService;
     TemporaryDir mRootDir;
     DataLoaderParamsParcel mDataLoaderParcel;
@@ -410,7 +433,11 @@ TEST_F(IncrementalServiceTest, testCreateStoragePrepareDataLoaderFails) {
     mIncFs->makeFileSuccess();
     mVold->bindMountSuccess();
     mDataLoaderManager->initializeDataLoaderFails();
+    EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(1);
     EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(1);
+    EXPECT_CALL(*mDataLoader, create(_, _, _, _)).Times(0);
+    EXPECT_CALL(*mDataLoader, start(_)).Times(0);
+    EXPECT_CALL(*mDataLoader, destroy(_)).Times(0);
     EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
     TemporaryDir tempDir;
     int storageId =
@@ -424,46 +451,84 @@ TEST_F(IncrementalServiceTest, testDeleteStorageSuccess) {
     mIncFs->makeFileSuccess();
     mVold->bindMountSuccess();
     mDataLoaderManager->initializeDataLoaderSuccess();
+    EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(1);
     EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(1);
+    EXPECT_CALL(*mDataLoader, create(_, _, _, _)).Times(1);
+    EXPECT_CALL(*mDataLoader, start(_)).Times(0);
+    EXPECT_CALL(*mDataLoader, destroy(_)).Times(1);
     EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
     TemporaryDir tempDir;
     int storageId =
             mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
                                                IncrementalService::CreateOptions::CreateNew);
     ASSERT_GE(storageId, 0);
+    mDataLoaderManager->setDataLoaderStatusCreated();
     mIncrementalService->deleteStorage(storageId);
 }
 
-TEST_F(IncrementalServiceTest, testOnStatusNotReady) {
+TEST_F(IncrementalServiceTest, testDataLoaderDestroyed) {
-    mVold->mountIncFsSuccess();
-    mIncFs->makeFileSuccess();
-    mVold->bindMountSuccess();
-    mDataLoaderManager->initializeDataLoaderSuccess();
-    EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
-    EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
-    TemporaryDir tempDir;
-    int storageId =
-            mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
-                                               IncrementalService::CreateOptions::CreateNew);
-    ASSERT_GE(storageId, 0);
-    mDataLoaderManager->setDataLoaderStatusNotReady();
-}
-
-TEST_F(IncrementalServiceTest, testStartDataLoaderSuccess) {
     mVold->mountIncFsSuccess();
     mIncFs->makeFileSuccess();
     mVold->bindMountSuccess();
     mDataLoaderManager->initializeDataLoaderSuccess();
     mDataLoaderManager->getDataLoaderSuccess();
-    EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_));
+    EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(2);
+    EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(1);
+    EXPECT_CALL(*mDataLoader, create(_, _, _, _)).Times(2);
+    EXPECT_CALL(*mDataLoader, start(_)).Times(0);
+    EXPECT_CALL(*mDataLoader, destroy(_)).Times(1);
     EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
     TemporaryDir tempDir;
     int storageId =
             mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
                                                IncrementalService::CreateOptions::CreateNew);
     ASSERT_GE(storageId, 0);
-    mDataLoaderManager->setDataLoaderStatusReady();
+    mDataLoaderManager->setDataLoaderStatusCreated();
+    // Simulated crash/other connection breakage.
+    mDataLoaderManager->setDataLoaderStatusDestroyed();
+}
+
+TEST_F(IncrementalServiceTest, testStartDataLoaderCreate) {
+    mVold->mountIncFsSuccess();
+    mIncFs->makeFileSuccess();
+    mVold->bindMountSuccess();
+    mDataLoaderManager->initializeDataLoaderSuccess();
+    mDataLoaderManager->getDataLoaderSuccess();
+    EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(1);
+    EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(1);
+    EXPECT_CALL(*mDataLoader, create(_, _, _, _)).Times(1);
+    EXPECT_CALL(*mDataLoader, start(_)).Times(1);
+    EXPECT_CALL(*mDataLoader, destroy(_)).Times(1);
+    EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
+    TemporaryDir tempDir;
+    int storageId =
+            mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
+                                               IncrementalService::CreateOptions::CreateNew);
+    ASSERT_GE(storageId, 0);
+    mDataLoaderManager->setDataLoaderStatusCreated();
     ASSERT_TRUE(mIncrementalService->startLoading(storageId));
+    mDataLoaderManager->setDataLoaderStatusStarted();
+}
+
+TEST_F(IncrementalServiceTest, testStartDataLoaderPendingStart) {
+    mVold->mountIncFsSuccess();
+    mIncFs->makeFileSuccess();
+    mVold->bindMountSuccess();
+    mDataLoaderManager->initializeDataLoaderSuccess();
+    mDataLoaderManager->getDataLoaderSuccess();
+    EXPECT_CALL(*mDataLoaderManager, initializeDataLoader(_, _, _, _, _)).Times(1);
+    EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)).Times(1);
+    EXPECT_CALL(*mDataLoader, create(_, _, _, _)).Times(1);
+    EXPECT_CALL(*mDataLoader, start(_)).Times(1);
+    EXPECT_CALL(*mDataLoader, destroy(_)).Times(1);
+    EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
+    TemporaryDir tempDir;
+    int storageId =
+            mIncrementalService->createStorage(tempDir.path, std::move(mDataLoaderParcel), {},
+                                               IncrementalService::CreateOptions::CreateNew);
+    ASSERT_GE(storageId, 0);
+    ASSERT_TRUE(mIncrementalService->startLoading(storageId));
+    mDataLoaderManager->setDataLoaderStatusCreated();
 }
 
 TEST_F(IncrementalServiceTest, testSetIncFsMountOptionsSuccess) {