Fix deadlock in camera destruction after client app's crash

* why deadlock happened: when an app (CTS camera test) crashes while using
  camera, its binder is closed and reference counter is decreased. If camera
  is inside callback, sp<Client> inside callback will hold the Client instance,
  and Client instance is destroyed when the callback ends as sp<Client> to hold
  it no longer exists. The destructor of Client instance tries to clean up
  camera H/W which tries to stop threads created by camera HAL including the
  thread context where the callback is running. This causes deadlock where the
  callback thread itself is waiting for itself to terminate.
  Note that the deadlock will not happen if camera callback is not active. In
  that case, closing of binder will force the destruction of Client instance,
  and the destruction happens in binder thread.

* Fix: Forces Client descruction in binder thread
- remove sp<Client> from callbacks to prevent destruction in callback context
- add client lock to allow callback to use raw pointer safely. This prevents
  the destructor from deleting the instance while callback is using it.
- add status change inside destructor with client lock to safely destroy Client

Bug: 6214383
Change-Id: Ic6d6396d4d95ce9e72a16ec2480ae65c100fe806

2 files changed, 60 insertions, 18 deletions

diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp
index 22836e3..dc3f083 100644
--- a/services/camera/libcameraservice/CameraService.cpp
+++ b/services/camera/libcameraservice/CameraService.cpp
@@ -299,9 +299,14 @@ void CameraService::removeClient(const sp<ICameraClient>& cameraClient) {
     LOG1("CameraService::removeClient X (pid %d)", callingPid);
 }
 
-sp<CameraService::Client> CameraService::getClientById(int cameraId) {
+CameraService::Client* CameraService::getClientByIdUnsafe(int cameraId) {
     if (cameraId < 0 || cameraId >= mNumberOfCameras) return NULL;
-    return mClient[cameraId].promote();
+    return mClient[cameraId].unsafe_get();
+}
+
+Mutex* CameraService::getClientLockById(int cameraId) {
+    if (cameraId < 0 || cameraId >= mNumberOfCameras) return NULL;
+    return &mClientLock[cameraId];
 }
 
 status_t CameraService::onTransact(
@@ -408,6 +413,7 @@ CameraService::Client::Client(const sp<CameraService>& cameraService,
     mMsgEnabled = 0;
     mSurface = 0;
     mPreviewWindow = 0;
+    mDestructionStarted = false;
     mHardware->setCallbacks(notifyCallback,
                             dataCallback,
                             dataCallbackTimestamp,
@@ -428,6 +434,12 @@ CameraService::Client::Client(const sp<CameraService>& cameraService,
 
 // tear down the client
 CameraService::Client::~Client() {
+    // this lock should never be NULL
+    Mutex* lock = mCameraService->getClientLockById(mCameraId);
+    lock->lock();
+    mDestructionStarted = true;
+    // client will not be accessed from callback. should unlock to prevent dead-lock in disconnect
+    lock->unlock();
     int callingPid = getCallingPid();
     LOG1("Client::~Client E (pid %d, this %p)", callingPid, this);
 
@@ -994,16 +1006,22 @@ bool CameraService::Client::lockIfMessageWanted(int32_t msgType) {
 
 // ----------------------------------------------------------------------------
 
-// Converts from a raw pointer to the client to a strong pointer during a
-// hardware callback. This requires the callbacks only happen when the client
-// is still alive.
-sp<CameraService::Client> CameraService::Client::getClientFromCookie(void* user) {
-    sp<Client> client = gCameraService->getClientById((int) user);
+Mutex* CameraService::Client::getClientLockFromCookie(void* user) {
+    return gCameraService->getClientLockById((int) user);
+}
+
+// Provide client pointer for callbacks. Client lock returned from getClientLockFromCookie should
+// be acquired for this to be safe
+CameraService::Client* CameraService::Client::getClientFromCookie(void* user) {
+    Client* client = gCameraService->getClientByIdUnsafe((int) user);
 
     // This could happen if the Client is in the process of shutting down (the
     // last strong reference is gone, but the destructor hasn't finished
     // stopping the hardware).
-    if (client == 0) return NULL;
+    if (client == NULL) return NULL;
+
+    // destruction already started, so should not be accessed
+    if (client->mDestructionStarted) return NULL;
 
     // The checks below are not necessary and are for debugging only.
     if (client->mCameraService.get() != gCameraService) {
@@ -1046,8 +1064,13 @@ void CameraService::Client::notifyCallback(int32_t msgType, int32_t ext1,
         int32_t ext2, void* user) {
     LOG2("notifyCallback(%d)", msgType);
 
-    sp<Client> client = getClientFromCookie(user);
-    if (client == 0) return;
+    Mutex* lock = getClientLockFromCookie(user);
+    if (lock == NULL) return;
+    Mutex::Autolock alock(*lock);
+
+    Client* client = getClientFromCookie(user);
+    if (client == NULL) return;
+
     if (!client->lockIfMessageWanted(msgType)) return;
 
     switch (msgType) {
@@ -1065,10 +1088,14 @@ void CameraService::Client::dataCallback(int32_t msgType,
         const sp<IMemory>& dataPtr, camera_frame_metadata_t *metadata, void* user) {
     LOG2("dataCallback(%d)", msgType);
 
-    sp<Client> client = getClientFromCookie(user);
-    if (client == 0) return;
-    if (!client->lockIfMessageWanted(msgType)) return;
+    Mutex* lock = getClientLockFromCookie(user);
+    if (lock == NULL) return;
+    Mutex::Autolock alock(*lock);
+
+    Client* client = getClientFromCookie(user);
+    if (client == NULL) return;
 
+    if (!client->lockIfMessageWanted(msgType)) return;
     if (dataPtr == 0 && metadata == NULL) {
         ALOGE("Null data returned in data callback");
         client->handleGenericNotify(CAMERA_MSG_ERROR, UNKNOWN_ERROR, 0);
@@ -1098,8 +1125,13 @@ void CameraService::Client::dataCallbackTimestamp(nsecs_t timestamp,
         int32_t msgType, const sp<IMemory>& dataPtr, void* user) {
     LOG2("dataCallbackTimestamp(%d)", msgType);
 
-    sp<Client> client = getClientFromCookie(user);
-    if (client == 0) return;
+    Mutex* lock = getClientLockFromCookie(user);
+    if (lock == NULL) return;
+    Mutex::Autolock alock(*lock);
+
+    Client* client = getClientFromCookie(user);
+    if (client == NULL) return;
+
     if (!client->lockIfMessageWanted(msgType)) return;
 
     if (dataPtr == 0) {
diff --git a/services/camera/libcameraservice/CameraService.h b/services/camera/libcameraservice/CameraService.h
index 457c79b..7972201 100644
--- a/services/camera/libcameraservice/CameraService.h
+++ b/services/camera/libcameraservice/CameraService.h
@@ -49,7 +49,10 @@ public:
     virtual sp<ICamera> connect(const sp<ICameraClient>& cameraClient, int cameraId,
                                 bool force, bool keep);
     virtual void        removeClient(const sp<ICameraClient>& cameraClient);
-    virtual sp<Client>  getClientById(int cameraId);
+    // returns plain pointer of client. Note that mClientLock should be acquired to
+    // prevent the client from destruction. The result can be NULL.
+    virtual Client*     getClientByIdUnsafe(int cameraId);
+    virtual Mutex*      getClientLockById(int cameraId);
 
     virtual status_t    dump(int fd, const Vector<String16>& args);
     virtual status_t    onTransact(uint32_t code, const Parcel& data,
@@ -69,6 +72,7 @@ public:
 private:
     Mutex               mServiceLock;
     wp<Client>          mClient[MAX_CAMERAS];  // protected by mServiceLock
+    Mutex               mClientLock[MAX_CAMERAS]; // prevent Client destruction inside callbacks
     int                 mNumberOfCameras;
 
     // atomics to record whether the hardware is allocated to some client.
@@ -147,8 +151,9 @@ private:
         static void             dataCallback(int32_t msgType, const sp<IMemory>& dataPtr,
                                              camera_frame_metadata_t *metadata, void* user);
         static void             dataCallbackTimestamp(nsecs_t timestamp, int32_t msgType, const sp<IMemory>& dataPtr, void* user);
-        // convert client from cookie
-        static sp<Client>       getClientFromCookie(void* user);
+        static Mutex*        getClientLockFromCookie(void* user);
+        // convert client from cookie. Client lock should be acquired before getting Client.
+        static Client*       getClientFromCookie(void* user);
         // handlers for messages
         void                    handleShutter(void);
         void                    handlePreviewData(int32_t msgType, const sp<IMemory>& mem,
@@ -204,6 +209,11 @@ private:
         // of the original one), we allocate mPreviewBuffer and reuse it if possible.
         sp<MemoryHeapBase>              mPreviewBuffer;
 
+        // the instance is in the middle of destruction. When this is set,
+        // the instance should not be accessed from callback.
+        // CameraService's mClientLock should be acquired to access this.
+        bool                            mDestructionStarted;
+
         // We need to avoid the deadlock when the incoming command thread and
         // the CameraHardwareInterface callback thread both want to grab mLock.
         // An extra flag is used to tell the callback thread that it should stop