diff options
4 files changed, 158 insertions, 58 deletions
diff --git a/services/core/java/com/android/server/pm/DataLoaderManagerService.java b/services/core/java/com/android/server/pm/DataLoaderManagerService.java index 09baf6e0a817..ae9c38498b10 100644 --- a/services/core/java/com/android/server/pm/DataLoaderManagerService.java +++ b/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(); } diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp index 149dfa6be6c4..8d084cddf0de 100644 --- a/services/incremental/IncrementalService.cpp +++ b/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) - << "Dataloader has to be destroyed prior to destructor: " << mId - << ", status: " << mStatus; + waitForDestroy(); } 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() { - if (mStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) { +bool IncrementalService::DataLoaderStub::requestStart() { + { + std::unique_lock lock(mStatusMutex); mStartRequested = true; - return 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() { - mDestroyRequested = true; + { + 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); - const auto& ifs = mService.getIfsLocked(mountId); - if (!ifs) { - LOG(WARNING) << "Received data loader status " << int(newStatus) - << " for unknown mount " << mountId; + std::unique_lock lock(mStatusMutex); + if (mStatus == newStatus) { return binder::Status::ok(); } - mStatus = newStatus; - if (!mDestroyRequested && newStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED) { - mService.deleteStorageLocked(*ifs, std::move(l)); - return binder::Status::ok(); - } + startRequested = mStartRequested; + destroyRequested = mDestroyRequested; + + 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 diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h index c016bab067be..8c79d7725dcf 100644 --- a/services/incremental/IncrementalService.h +++ b/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>; diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp index 117dca8c37b3..f9737fee450d 100644 --- a/services/incremental/test/IncrementalServiceTest.cpp +++ b/services/incremental/test/IncrementalServiceTest.cpp @@ -103,25 +103,34 @@ private: TemporaryFile logFile; }; -class FakeDataLoader : public IDataLoader { +class MockDataLoader : public IDataLoader { public: - IBinder* onAsBinder() override { return nullptr; } - binder::Status create(int32_t, const DataLoaderParamsParcel&, const FileSystemControlParcel&, - const sp<IDataLoaderStatusListener>&) override { - return binder::Status::ok(); - } - binder::Status start(int32_t) override { return binder::Status::ok(); } - binder::Status stop(int32_t) override { return binder::Status::ok(); } - binder::Status destroy(int32_t) override { return binder::Status::ok(); } - binder::Status prepareImage(int32_t, - const std::vector<InstallationFileParcel>&, - const std::vector<std::string>&) override { - return binder::Status::ok(); + 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; } + MOCK_METHOD4(create, + binder::Status(int32_t id, const DataLoaderParamsParcel& params, + const FileSystemControlParcel& control, + const sp<IDataLoaderStatusListener>& listener)); + MOCK_METHOD1(start, binder::Status(int32_t id)); + MOCK_METHOD1(stop, binder::Status(int32_t id)); + MOCK_METHOD1(destroy, binder::Status(int32_t id)); + MOCK_METHOD3(prepareImage, + binder::Status(int32_t id, const std::vector<InstallationFileParcel>& addedFiles, + const std::vector<std::string>& removedFiles)); }; 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() { - mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_DESTROYED); - } - void setDataLoaderStatusReady() { + void setDataLoaderStatusCreated() { 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(_)); + mDataLoaderManager->getDataLoaderSuccess(); + 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->setDataLoaderStatusNotReady(); + mDataLoaderManager->setDataLoaderStatusCreated(); + // Simulated crash/other connection breakage. + mDataLoaderManager->setDataLoaderStatusDestroyed(); } -TEST_F(IncrementalServiceTest, testStartDataLoaderSuccess) { +TEST_F(IncrementalServiceTest, testStartDataLoaderCreate) { mVold->mountIncFsSuccess(); mIncFs->makeFileSuccess(); mVold->bindMountSuccess(); mDataLoaderManager->initializeDataLoaderSuccess(); mDataLoaderManager->getDataLoaderSuccess(); - EXPECT_CALL(*mDataLoaderManager, destroyDataLoader(_)); + 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); - mDataLoaderManager->setDataLoaderStatusReady(); ASSERT_TRUE(mIncrementalService->startLoading(storageId)); + mDataLoaderManager->setDataLoaderStatusCreated(); } TEST_F(IncrementalServiceTest, testSetIncFsMountOptionsSuccess) { |