commit 9a54579ac5d68bd1b665c159901183c604078cdd
author Alex Buynytskyy <alexbuy@google.com> Fri Apr 17 15:34:47 2020 -0700
committer Alex Buynytskyy <alexbuy@google.com> Fri Apr 17 22:07:10 2020 -0700
tree 6819ec5694c4ed1db7d2d5029e9aa44dd43a0039
parent ab65cb1824d27f1c864b3cccc5075b5e1b96667c

Cleaning up resources on mount destruction.

DataLoaderStub's lifetime is controlled externally, but we want to
release resources ASAP.

Bug: b/153874006
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest
Change-Id: I34035f36d1fe4ed0e4916014d859feb7fe2c0a09

services/incremental/IncrementalService.cpp

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 68b7ac0..79699bb 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -160,8 +160,8 @@
 
 IncrementalService::IncFsMount::~IncFsMount() {
     if (dataLoaderStub) {
-        dataLoaderStub->requestDestroy();
-        dataLoaderStub->waitForDestroy();
+        dataLoaderStub->cleanupResources();
+        dataLoaderStub = {};
     }
     LOG(INFO) << "Unmounting and cleaning up mount " << mountId << " with root '" << root << '\'';
     for (auto&& [target, _] : bindPoints) {
@@ -999,7 +999,8 @@
             return false;
         }
     }
-    return dataLoaderStub->requestStart();
+    dataLoaderStub->requestStart();
+    return true;
 }
 
 void IncrementalService::mountExistingImages() {
@@ -1466,11 +1467,17 @@
         mParams(std::move(params)),
         mControl(std::move(control)),
         mListener(externalListener ? *externalListener : DataLoaderStatusListener()) {
-    //
 }
 
-IncrementalService::DataLoaderStub::~DataLoaderStub() {
-    waitForDestroy();
+IncrementalService::DataLoaderStub::~DataLoaderStub() = default;
+
+void IncrementalService::DataLoaderStub::cleanupResources() {
+    requestDestroy();
+    mParams = {};
+    mControl = {};
+    waitForStatus(IDataLoaderStatusListener::DATA_LOADER_DESTROYED, std::chrono::seconds(60));
+    mListener = {};
+    mId = kInvalidStorageId;
 }
 
 bool IncrementalService::DataLoaderStub::requestCreate() {
@@ -1485,10 +1492,6 @@
     return setTargetStatus(IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
 }
 
-bool IncrementalService::DataLoaderStub::waitForDestroy(Clock::duration duration) {
-    return waitForStatus(IDataLoaderStatusListener::DATA_LOADER_DESTROYED, duration);
-}
-
 bool IncrementalService::DataLoaderStub::setTargetStatus(int status) {
     {
         std::unique_lock lock(mStatusMutex);
@@ -1541,6 +1544,10 @@
 }
 
 bool IncrementalService::DataLoaderStub::fsmStep() {
+    if (!isValid()) {
+        return false;
+    }
+
     int currentStatus;
     int targetStatus;
     {
@@ -1580,6 +1587,15 @@
 }
 
 binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mountId, int newStatus) {
+    if (!isValid()) {
+        return binder::Status::
+                fromServiceSpecificError(-EINVAL, "onStatusChange came to invalid DataLoaderStub");
+    }
+    if (mId != mountId) {
+        LOG(ERROR) << "Mount ID mismatch: expected " << mId << ", but got: " << mountId;
+        return binder::Status::fromServiceSpecificError(-EPERM, "Mount ID mismatch.");
+    }
+
     {
         std::unique_lock lock(mStatusMutex);
         if (mCurrentStatus == newStatus) {

services/incremental/IncrementalService.h

diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 8b28bac..bd01d77 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -173,13 +173,14 @@
                        FileSystemControlParcel&& control,
                        const DataLoaderStatusListener* externalListener);
         ~DataLoaderStub();
+        // Cleans up the internal state and invalidates DataLoaderStub. Any subsequent calls will
+        // result in an error.
+        void cleanupResources();
 
         bool requestCreate();
         bool requestStart();
         bool requestDestroy();
 
-        bool waitForDestroy(Clock::duration duration = std::chrono::seconds(60));
-
         void onDump(int fd);
 
         MountId id() const { return mId; }
@@ -188,6 +189,8 @@
     private:
         binder::Status onStatusChanged(MountId mount, int newStatus) final;
 
+        bool isValid() const { return mId != kInvalidStorageId; }
+
         bool create();
         bool start();
         bool destroy();
@@ -198,10 +201,10 @@
         bool fsmStep();
 
         IncrementalService& mService;
-        MountId const mId;
-        DataLoaderParamsParcel const mParams;
-        FileSystemControlParcel const mControl;
-        DataLoaderStatusListener const mListener;
+        MountId mId = kInvalidStorageId;
+        DataLoaderParamsParcel mParams;
+        FileSystemControlParcel mControl;
+        DataLoaderStatusListener mListener;
 
         std::mutex mStatusMutex;
         std::condition_variable mStatusCondition;