commit 0bdbccf37cc405ca18a1b959df83906a022f4886
author Alex Buynytskyy <alexbuy@google.com> Thu Apr 23 20:36:42 2020 -0700
committer Alex Buynytskyy <alexbuy@google.com> Thu Apr 23 22:28:36 2020 -0700
tree ca6e43af0344f503a7a35cdaf577b7f3ee9c2cb6
parent bd70574e1249b91e6f1f70681fa2ed8d3386f96b

Corner cases handling.

- crash on disappearing dataloaders,
- more robust binder callbacks processing,
- heavy unbind lifting moved to a separate thread to unblock Binder.

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

services/incremental/IncrementalService.cpp

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index a1b4f24..cae5027 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -761,7 +761,10 @@
     std::unique_lock l2(ifs->lock);
     if (ifs->bindPoints.size() <= 1) {
         ifs->bindPoints.clear();
-        deleteStorageLocked(*ifs, std::move(l2));
+        std::thread([this, ifs, l2 = std::move(l2)]() mutable {
+            mJni->initializeForCurrentThread();
+            deleteStorageLocked(*ifs, std::move(l2));
+        }).detach();
     } else {
         const std::string savedFile = std::move(bindIt->second.savedFilename);
         ifs->bindPoints.erase(bindIt);
@@ -770,6 +773,7 @@
             mIncFs->unlink(ifs->control, path::join(ifs->root, constants().mount, savedFile));
         }
     }
+
     return 0;
 }
 
@@ -1676,6 +1680,20 @@
     mId = kInvalidStorageId;
 }
 
+sp<content::pm::IDataLoader> IncrementalService::DataLoaderStub::getDataLoader() {
+    sp<IDataLoader> dataloader;
+    auto status = mService.mDataLoaderManager->getDataLoader(mId, &dataloader);
+    if (!status.isOk()) {
+        LOG(ERROR) << "Failed to get dataloader: " << status.toString8();
+        return {};
+    }
+    if (!dataloader) {
+        LOG(ERROR) << "DataLoader is null: " << status.toString8();
+        return {};
+    }
+    return dataloader;
+}
+
 bool IncrementalService::DataLoaderStub::requestCreate() {
     return setTargetStatus(IDataLoaderStatusListener::DATA_LOADER_CREATED);
 }
@@ -1720,17 +1738,11 @@
 }
 
 bool IncrementalService::DataLoaderStub::create() {
-    sp<IDataLoader> dataloader;
-    auto status = mService.mDataLoaderManager->getDataLoader(mId, &dataloader);
-    if (!status.isOk()) {
-        LOG(ERROR) << "Failed to get dataloader: " << status.toString8();
-        return false;
-    }
+    auto dataloader = getDataLoader();
     if (!dataloader) {
-        LOG(ERROR) << "DataLoader is null: " << status.toString8();
         return false;
     }
-    status = dataloader->create(mId, mParams, mControl, this);
+    auto status = dataloader->create(mId, mParams, mControl, this);
     if (!status.isOk()) {
         LOG(ERROR) << "Failed to start DataLoader: " << status.toString8();
         return false;
@@ -1739,17 +1751,11 @@
 }
 
 bool IncrementalService::DataLoaderStub::start() {
-    sp<IDataLoader> dataloader;
-    auto status = mService.mDataLoaderManager->getDataLoader(mId, &dataloader);
-    if (!status.isOk()) {
-        LOG(ERROR) << "Failed to get dataloader: " << status.toString8();
-        return false;
-    }
+    auto dataloader = getDataLoader();
     if (!dataloader) {
-        LOG(ERROR) << "DataLoader is null: " << status.toString8();
         return false;
     }
-    status = dataloader->start(mId);
+    auto status = dataloader->start(mId);
     if (!status.isOk()) {
         LOG(ERROR) << "Failed to start DataLoader: " << status.toString8();
         return false;
@@ -1758,8 +1764,7 @@
 }
 
 bool IncrementalService::DataLoaderStub::destroy() {
-    mService.mDataLoaderManager->unbindFromDataLoader(mId);
-    return true;
+    return mService.mDataLoaderManager->unbindFromDataLoader(mId).isOk();
 }
 
 bool IncrementalService::DataLoaderStub::fsmStep() {
@@ -1823,8 +1828,8 @@
         if (mCurrentStatus == newStatus) {
             return binder::Status::ok();
         }
-        mCurrentStatus = newStatus;
         oldStatus = mCurrentStatus;
+        mCurrentStatus = newStatus;
         targetStatus = mTargetStatus;
     }
 

services/incremental/IncrementalService.h

diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 1de0070..81fbe74 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -179,6 +179,7 @@
         binder::Status onStatusChanged(MountId mount, int newStatus) final;
 
         bool isValid() const { return mId != kInvalidStorageId; }
+        sp<content::pm::IDataLoader> getDataLoader();
 
         bool bind();
         bool create();