Camera2: Handle client processes dying by closing camera resources

CameraService now subscribes to binder death notifications
for each client, and disconnects the client if the death happened
without cleanly shutting down the client first.

Bug: 7258314
Change-Id: I7803836b589fd8f0dfe00f6c28a707b82012e751

11 files changed, 121 insertions, 28 deletions

diff --git a/services/camera/libcameraservice/Camera2Client.cpp b/services/camera/libcameraservice/Camera2Client.cpp
index 59ec6b6..948b59f 100644
--- a/services/camera/libcameraservice/Camera2Client.cpp
+++ b/services/camera/libcameraservice/Camera2Client.cpp
@@ -47,9 +47,10 @@ Camera2Client::Camera2Client(const sp<CameraService>& cameraService,
         const sp<ICameraClient>& cameraClient,
         int cameraId,
         int cameraFacing,
-        int clientPid):
+        int clientPid,
+        int servicePid):
         Client(cameraService, cameraClient,
-                cameraId, cameraFacing, clientPid),
+                cameraId, cameraFacing, clientPid, servicePid),
         mSharedCameraClient(cameraClient),
         mParameters(cameraId, cameraFacing)
 {
@@ -64,10 +65,10 @@ Camera2Client::Camera2Client(const sp<CameraService>& cameraService,
 
 status_t Camera2Client::checkPid(const char* checkLocation) const {
     int callingPid = getCallingPid();
-    if (callingPid == mClientPid) return NO_ERROR;
+    if (callingPid == mClientPid || callingPid == mServicePid) return NO_ERROR;
 
     ALOGE("%s: attempt to use a locked camera from a different process"
-            " (old pid %d, new pid %d)", checkLocation, mClientPid, callingPid);
+            " (old pid %d, new pid %d, servicePid %d)", checkLocation, mClientPid, callingPid, mServicePid);
     return PERMISSION_DENIED;
 }
 
@@ -138,8 +139,15 @@ Camera2Client::~Camera2Client() {
 
     mDestructionStarted = true;
 
-    SharedParameters::Lock l(mParameters);
-    if (l.mParameters.state != Parameters::DISCONNECTED) {
+    Parameters::State state;
+    // warning:
+    //   holding on to locks more than necessary may be hazardous to your health
+    {
+        SharedParameters::Lock l(mParameters);
+        state = l.mParameters.state;
+    }
+
+    if (state != Parameters::DISCONNECTED) {
         // Rewrite mClientPid to allow shutdown by CameraService
         mClientPid = getCallingPid();
         disconnect();
diff --git a/services/camera/libcameraservice/Camera2Client.h b/services/camera/libcameraservice/Camera2Client.h
index 3a9d307..fb1dcde 100644
--- a/services/camera/libcameraservice/Camera2Client.h
+++ b/services/camera/libcameraservice/Camera2Client.h
@@ -74,7 +74,8 @@ public:
             const sp<ICameraClient>& cameraClient,
             int cameraId,
             int cameraFacing,
-            int clientPid);
+            int clientPid,
+            int servicePid);
     virtual ~Camera2Client();
 
     status_t initialize(camera_module_t *module);
diff --git a/services/camera/libcameraservice/Camera2Device.cpp b/services/camera/libcameraservice/Camera2Device.cpp
index 2e4098e..6da9bef 100644
--- a/services/camera/libcameraservice/Camera2Device.cpp
+++ b/services/camera/libcameraservice/Camera2Device.cpp
@@ -42,6 +42,7 @@ Camera2Device::Camera2Device(int id):
 Camera2Device::~Camera2Device()
 {
     ATRACE_CALL();
+    ALOGV("%s: Tearing down for camera id %d", __FUNCTION__, mId);
     disconnect();
 }
 
diff --git a/services/camera/libcameraservice/CameraClient.cpp b/services/camera/libcameraservice/CameraClient.cpp
index c9c816a..5b59ef9 100644
--- a/services/camera/libcameraservice/CameraClient.cpp
+++ b/services/camera/libcameraservice/CameraClient.cpp
@@ -40,9 +40,9 @@ static int getCallingUid() {
 
 CameraClient::CameraClient(const sp<CameraService>& cameraService,
         const sp<ICameraClient>& cameraClient,
-        int cameraId, int cameraFacing, int clientPid):
+        int cameraId, int cameraFacing, int clientPid, int servicePid):
         Client(cameraService, cameraClient,
-                cameraId, cameraFacing, clientPid)
+                cameraId, cameraFacing, clientPid, servicePid)
 {
     int callingPid = getCallingPid();
     LOG1("CameraClient::CameraClient E (pid %d, id %d)", callingPid, cameraId);
@@ -124,7 +124,7 @@ status_t CameraClient::dump(int fd, const Vector<String16>& args) {
 
 status_t CameraClient::checkPid() const {
     int callingPid = getCallingPid();
-    if (callingPid == mClientPid) return NO_ERROR;
+    if (callingPid == mClientPid || callingPid == mServicePid) return NO_ERROR;
 
     ALOGW("attempt to use a locked camera from a different process"
          " (old pid %d, new pid %d)", mClientPid, callingPid);
diff --git a/services/camera/libcameraservice/CameraClient.h b/services/camera/libcameraservice/CameraClient.h
index 256298d..2f31c4e 100644
--- a/services/camera/libcameraservice/CameraClient.h
+++ b/services/camera/libcameraservice/CameraClient.h
@@ -55,7 +55,8 @@ public:
             const sp<ICameraClient>& cameraClient,
             int cameraId,
             int cameraFacing,
-            int clientPid);
+            int clientPid,
+            int servicePid);
     ~CameraClient();
 
     status_t initialize(camera_module_t *module);
diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp
index 878afde..4d48d8d 100644
--- a/services/camera/libcameraservice/CameraService.cpp
+++ b/services/camera/libcameraservice/CameraService.cpp
@@ -196,11 +196,11 @@ sp<ICamera> CameraService::connect(
     switch(deviceVersion) {
       case CAMERA_DEVICE_API_VERSION_1_0:
         client = new CameraClient(this, cameraClient, cameraId,
-                info.facing, callingPid);
+                info.facing, callingPid, getpid());
         break;
       case CAMERA_DEVICE_API_VERSION_2_0:
         client = new Camera2Client(this, cameraClient, cameraId,
-                info.facing, callingPid);
+                info.facing, callingPid, getpid());
         break;
       default:
         ALOGE("Unknown camera device HAL version: %d", deviceVersion);
@@ -211,8 +211,10 @@ sp<ICamera> CameraService::connect(
         return NULL;
     }
 
+    cameraClient->asBinder()->linkToDeath(this);
+
     mClient[cameraId] = client;
-    LOG1("CameraService::connect X (id %d)", cameraId);
+    LOG1("CameraService::connect X (id %d, this pid is %d)", cameraId, getpid());
     return client;
 }
 
@@ -220,12 +222,29 @@ void CameraService::removeClient(const sp<ICameraClient>& cameraClient) {
     int callingPid = getCallingPid();
     LOG1("CameraService::removeClient E (pid %d)", callingPid);
 
-    for (int i = 0; i < mNumberOfCameras; i++) {
-        // Declare this before the lock to make absolutely sure the
-        // destructor won't be called with the lock held.
-        sp<Client> client;
+    // Declare this before the lock to make absolutely sure the
+    // destructor won't be called with the lock held.
+    Mutex::Autolock lock(mServiceLock);
+
+    int outIndex;
+    sp<Client> client = findClientUnsafe(cameraClient, outIndex);
+
+    if (client != 0) {
+        // Found our camera, clear and leave.
+        LOG1("removeClient: clear camera %d", outIndex);
+        mClient[outIndex].clear();
 
-        Mutex::Autolock lock(mServiceLock);
+        client->unlinkToDeath(this);
+    }
+
+    LOG1("CameraService::removeClient X (pid %d)", callingPid);
+}
+
+sp<CameraService::Client> CameraService::findClientUnsafe(
+                        const sp<ICameraClient>& cameraClient, int& outIndex) {
+    sp<Client> client;
+
+    for (int i = 0; i < mNumberOfCameras; i++) {
 
         // This happens when we have already disconnected (or this is
         // just another unused camera).
@@ -235,20 +254,21 @@ void CameraService::removeClient(const sp<ICameraClient>& cameraClient) {
         // Client::~Client() -> disconnect() -> removeClient().
         client = mClient[i].promote();
 
-        if (client == 0) {
+        // Clean up stale client entry
+        if (client == NULL) {
             mClient[i].clear();
             continue;
         }
 
         if (cameraClient->asBinder() == client->getCameraClient()->asBinder()) {
-            // Found our camera, clear and leave.
-            LOG1("removeClient: clear camera %d", i);
-            mClient[i].clear();
-            break;
+            // Found our camera
+            outIndex = i;
+            return client;
         }
     }
 
-    LOG1("CameraService::removeClient X (pid %d)", callingPid);
+    outIndex = -1;
+    return NULL;
 }
 
 CameraService::Client* CameraService::getClientByIdUnsafe(int cameraId) {
@@ -261,6 +281,21 @@ Mutex* CameraService::getClientLockById(int cameraId) {
     return &mClientLock[cameraId];
 }
 
+/*virtual*/sp<CameraService::Client> CameraService::getClientByRemote(
+                                const sp<ICameraClient>& cameraClient) {
+
+    // Declare this before the lock to make absolutely sure the
+    // destructor won't be called with the lock held.
+    sp<Client> client;
+
+    Mutex::Autolock lock(mServiceLock);
+
+    int outIndex;
+    client = findClientUnsafe(cameraClient, outIndex);
+
+    return client;
+}
+
 status_t CameraService::onTransact(
     uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) {
     // Permission checks
@@ -292,10 +327,14 @@ status_t CameraService::onTransact(
 // the hardware first.
 void CameraService::setCameraBusy(int cameraId) {
     android_atomic_write(1, &mBusy[cameraId]);
+
+    ALOGV("setCameraBusy cameraId=%d", cameraId);
 }
 
 void CameraService::setCameraFree(int cameraId) {
     android_atomic_write(0, &mBusy[cameraId]);
+
+    ALOGV("setCameraFree cameraId=%d", cameraId);
 }
 
 // We share the media players for shutter and recording sound for all clients.
@@ -350,7 +389,7 @@ void CameraService::playSound(sound_kind kind) {
 
 CameraService::Client::Client(const sp<CameraService>& cameraService,
         const sp<ICameraClient>& cameraClient,
-        int cameraId, int cameraFacing, int clientPid) {
+        int cameraId, int cameraFacing, int clientPid, int servicePid) {
     int callingPid = getCallingPid();
     LOG1("Client::Client E (pid %d, id %d)", callingPid, cameraId);
 
@@ -359,6 +398,7 @@ CameraService::Client::Client(const sp<CameraService>& cameraService,
     mCameraId = cameraId;
     mCameraFacing = cameraFacing;
     mClientPid = clientPid;
+    mServicePid = servicePid;
     mDestructionStarted = false;
 
     cameraService->setCameraBusy(cameraId);
@@ -514,4 +554,26 @@ status_t CameraService::dump(int fd, const Vector<String16>& args) {
     return NO_ERROR;
 }
 
+/*virtual*/void CameraService::binderDied(
+    const wp<IBinder> &who) {
+
+    ALOGV("java clients' binder died");
+
+    sp<IBinder> whoStrong = who.promote();
+
+    if (whoStrong == 0) {
+        ALOGV("java clients' binder death already cleaned up (normal case)");
+        return;
+    }
+
+    sp<ICameraClient> iCamClient = interface_cast<ICameraClient>(whoStrong);
+
+    sp<Client> cameraClient = getClientByRemote(iCamClient);
+    ALOGW("Disconnecting camera client %p since the binder for it "
+          "died (this pid %d)", cameraClient.get(), getCallingPid());
+
+    cameraClient->disconnect();
+
+}
+
 }; // namespace android
diff --git a/services/camera/libcameraservice/CameraService.h b/services/camera/libcameraservice/CameraService.h
index 630fca7..f1e7df6 100644
--- a/services/camera/libcameraservice/CameraService.h
+++ b/services/camera/libcameraservice/CameraService.h
@@ -34,7 +34,8 @@ class MediaPlayer;
 
 class CameraService :
     public BinderService<CameraService>,
-    public BnCameraService
+    public BnCameraService,
+    public IBinder::DeathRecipient
 {
     friend class BinderService<CameraService>;
 public:
@@ -54,6 +55,8 @@ public:
     virtual Client*     getClientByIdUnsafe(int cameraId);
     virtual Mutex*      getClientLockById(int cameraId);
 
+    virtual sp<Client>  getClientByRemote(const sp<ICameraClient>& cameraClient);
+
     virtual status_t    dump(int fd, const Vector<String16>& args);
     virtual status_t    onTransact(uint32_t code, const Parcel& data,
                                    Parcel* reply, uint32_t flags);
@@ -100,7 +103,8 @@ public:
                 const sp<ICameraClient>& cameraClient,
                 int cameraId,
                 int cameraFacing,
-                int clientPid);
+                int clientPid,
+                int servicePid);
         ~Client();
 
         // return our camera client
@@ -128,6 +132,7 @@ public:
         int                             mCameraId;       // immutable after constructor
         int                             mCameraFacing;   // immutable after constructor
         pid_t                           mClientPid;
+        pid_t                           mServicePid;     // immutable after constructor
 
     };
 
@@ -137,6 +142,9 @@ private:
     Mutex               mClientLock[MAX_CAMERAS]; // prevent Client destruction inside callbacks
     int                 mNumberOfCameras;
 
+    // needs to be called with mServiceLock held
+    sp<Client>          findClientUnsafe(const sp<ICameraClient>& cameraClient, int& outIndex);
+
     // atomics to record whether the hardware is allocated to some client.
     volatile int32_t    mBusy[MAX_CAMERAS];
     void                setCameraBusy(int cameraId);
@@ -150,6 +158,9 @@ private:
     int                 mSoundRef;  // reference count (release all MediaPlayer when 0)
 
     camera_module_t *mModule;
+
+    // IBinder::DeathRecipient implementation
+    virtual void binderDied(const wp<IBinder> &who);
 };
 
 } // namespace android
diff --git a/services/camera/libcameraservice/camera2/CallbackProcessor.cpp b/services/camera/libcameraservice/camera2/CallbackProcessor.cpp
index ede97a6..3e9c255 100644
--- a/services/camera/libcameraservice/camera2/CallbackProcessor.cpp
+++ b/services/camera/libcameraservice/camera2/CallbackProcessor.cpp
@@ -86,6 +86,8 @@ status_t CallbackProcessor::updateStream(const Parameters &params) {
             // Since size should only change while preview is not running,
             // assuming that all existing use of old callback stream is
             // completed.
+            ALOGV("%s: Camera %d: Deleting stream %d since the buffer dimensions changed",
+                __FUNCTION__, client->getCameraId(), mCallbackStreamId);
             res = device->deleteStream(mCallbackStreamId);
             if (res != OK) {
                 ALOGE("%s: Camera %d: Unable to delete old output stream "
diff --git a/services/camera/libcameraservice/camera2/JpegProcessor.cpp b/services/camera/libcameraservice/camera2/JpegProcessor.cpp
index 7b368fa..a353679 100644
--- a/services/camera/libcameraservice/camera2/JpegProcessor.cpp
+++ b/services/camera/libcameraservice/camera2/JpegProcessor.cpp
@@ -107,6 +107,8 @@ status_t JpegProcessor::updateStream(const Parameters &params) {
         }
         if (currentWidth != (uint32_t)params.pictureWidth ||
                 currentHeight != (uint32_t)params.pictureHeight) {
+            ALOGV("%s: Camera %d: Deleting stream %d since the buffer dimensions changed",
+                __FUNCTION__, client->getCameraId(), mCaptureStreamId);
             res = device->deleteStream(mCaptureStreamId);
             if (res != OK) {
                 ALOGE("%s: Camera %d: Unable to delete old output stream "
diff --git a/services/camera/libcameraservice/camera2/StreamingProcessor.cpp b/services/camera/libcameraservice/camera2/StreamingProcessor.cpp
index feaeb6c..744b7ed 100644
--- a/services/camera/libcameraservice/camera2/StreamingProcessor.cpp
+++ b/services/camera/libcameraservice/camera2/StreamingProcessor.cpp
@@ -170,6 +170,9 @@ status_t StreamingProcessor::deletePreviewStream() {
         if (client == 0) return INVALID_OPERATION;
         sp<Camera2Device> device = client->getCameraDevice();
 
+        ALOGV("%s: for cameraId %d on streamId %d",
+            __FUNCTION__, client->getCameraId(), mPreviewStreamId);
+
         res = device->waitUntilDrained();
         if (res != OK) {
             ALOGE("%s: Error waiting for preview to drain: %s (%d)",
diff --git a/services/camera/libcameraservice/camera2/ZslProcessor.cpp b/services/camera/libcameraservice/camera2/ZslProcessor.cpp
index f462527..1bcf97e 100644
--- a/services/camera/libcameraservice/camera2/ZslProcessor.cpp
+++ b/services/camera/libcameraservice/camera2/ZslProcessor.cpp
@@ -147,6 +147,8 @@ status_t ZslProcessor::updateStream(const Parameters &params) {
                         client->getCameraId(), strerror(-res), res);
                 return res;
             }
+            ALOGV("%s: Camera %d: Deleting stream %d since the buffer dimensions changed",
+                __FUNCTION__, client->getCameraId(), mZslStreamId);
             res = device->deleteStream(mZslStreamId);
             if (res != OK) {
                 ALOGE("%s: Camera %d: Unable to delete old output stream "