commit 9e0302fcc104bbeddc4c9c0f913b9f633de84dc1
author Jayant Chowdhary <jchowdhary@google.com> Tue Jul 16 11:04:06 2019 -0700
committer Jayant Chowdhary <jchowdhary@google.com> Wed Jul 17 14:30:10 2019 -0700
tree fa3f75ad3291885bb054ea2bd8a310c16679f58a
parent 4c6d99520c95386e5a4f4a94aafe9c63053ae734

AImage: don't allow ~AImageReader to run before AImages are deleted.

We can never be sure whether ~AImageReader() has run to completion or
not in AImage::close(). So we move clean up to another AImageReader
method and make sure that's called when in AImageReader_delete. AImage
now contains an sp<> to AImageReader so we can be sure that its members
wouldn't have been destroyed by ~AImageReader and we can check whether
AImageReader_delete has been called on it.

This also prevents us from deadlocking since AImage_delete could also cause
~AImageReader to run with AImageReader::mLock held in AImage::close().

Bug: 137571625
Bug: 137694217

Test: enroll; auth
Test: cts native AImageReader, camera, graphics tests

Change-Id: If5803cb6fcb6f4800032069872daaeac1cd36ed2
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>

media/ndk/NdkImage.cpp

diff --git a/media/ndk/NdkImage.cpp b/media/ndk/NdkImage.cpp
index 1883f63..1145b7b 100644
--- a/media/ndk/NdkImage.cpp
+++ b/media/ndk/NdkImage.cpp
@@ -35,6 +35,7 @@
         int64_t timestamp, int32_t width, int32_t height, int32_t numPlanes) :
         mReader(reader), mFormat(format), mUsage(usage), mBuffer(buffer), mLockedBuffer(nullptr),
         mTimestamp(timestamp), mWidth(width), mHeight(height), mNumPlanes(numPlanes) {
+    LOG_FATAL_IF(reader == nullptr, "AImageReader shouldn't be null while creating AImage");
 }
 
 AImage::~AImage() {
@@ -57,14 +58,9 @@
     if (mIsClosed) {
         return;
     }
-    sp<AImageReader> reader = mReader.promote();
-    if (reader != nullptr) {
-        reader->releaseImageLocked(this, releaseFenceFd);
-    } else if (mBuffer != nullptr) {
-        LOG_ALWAYS_FATAL("%s: parent AImageReader closed without releasing image %p",
-                __FUNCTION__, this);
+    if (!mReader->mIsClosed) {
+        mReader->releaseImageLocked(this, releaseFenceFd);
     }
-
     // Should have been set to nullptr in releaseImageLocked
     // Set to nullptr here for extra safety only
     mBuffer = nullptr;
@@ -83,22 +79,12 @@
 
 void
 AImage::lockReader() const {
-    sp<AImageReader> reader = mReader.promote();
-    if (reader == nullptr) {
-        // Reader has been closed
-        return;
-    }
-    reader->mLock.lock();
+    mReader->mLock.lock();
 }
 
 void
 AImage::unlockReader() const {
-    sp<AImageReader> reader = mReader.promote();
-    if (reader == nullptr) {
-        // Reader has been closed
-        return;
-    }
-    reader->mLock.unlock();
+    mReader->mLock.unlock();
 }
 
 media_status_t

media/ndk/NdkImagePriv.h

diff --git a/media/ndk/NdkImagePriv.h b/media/ndk/NdkImagePriv.h
index e0f16da..0e8cbcb 100644
--- a/media/ndk/NdkImagePriv.h
+++ b/media/ndk/NdkImagePriv.h
@@ -72,7 +72,7 @@
     uint32_t getJpegSize() const;
 
     // When reader is close, AImage will only accept close API call
-    wp<AImageReader>           mReader;
+    const sp<AImageReader>     mReader;
     const int32_t              mFormat;
     const uint64_t             mUsage;  // AHARDWAREBUFFER_USAGE_* flags.
     BufferItem*                mBuffer;

media/ndk/NdkImageReader.cpp

diff --git a/media/ndk/NdkImageReader.cpp b/media/ndk/NdkImageReader.cpp
index 830f752..c0ceb3d 100644
--- a/media/ndk/NdkImageReader.cpp
+++ b/media/ndk/NdkImageReader.cpp
@@ -272,6 +272,11 @@
       mFrameListener(new FrameListener(this)),
       mBufferRemovedListener(new BufferRemovedListener(this)) {}
 
+AImageReader::~AImageReader() {
+    Mutex::Autolock _l(mLock);
+    LOG_FATAL_IF("AImageReader not closed before destruction", mIsClosed != true);
+}
+
 media_status_t
 AImageReader::init() {
     PublicFormat publicFormat = static_cast<PublicFormat>(mFormat);
@@ -347,8 +352,12 @@
     return AMEDIA_OK;
 }
 
-AImageReader::~AImageReader() {
+void AImageReader::close() {
     Mutex::Autolock _l(mLock);
+    if (mIsClosed) {
+        return;
+    }
+    mIsClosed = true;
     AImageReader_ImageListener nullListener = {nullptr, nullptr};
     setImageListenerLocked(&nullListener);
 
@@ -741,6 +750,7 @@
 void AImageReader_delete(AImageReader* reader) {
     ALOGV("%s", __FUNCTION__);
     if (reader != nullptr) {
+        reader->close();
         reader->decStrong((void*) AImageReader_delete);
     }
     return;

media/ndk/NdkImageReaderPriv.h

diff --git a/media/ndk/NdkImageReaderPriv.h b/media/ndk/NdkImageReaderPriv.h
index 19bd704..0779a71 100644
--- a/media/ndk/NdkImageReaderPriv.h
+++ b/media/ndk/NdkImageReaderPriv.h
@@ -76,6 +76,7 @@
     int32_t        getHeight()    const { return mHeight; };
     int32_t        getFormat()    const { return mFormat; };
     int32_t        getMaxImages() const { return mMaxImages; };
+    void           close();
 
   private:
 
@@ -165,6 +166,7 @@
     native_handle_t*           mWindowHandle = nullptr;
 
     List<AImage*>              mAcquiredImages;
+    bool                       mIsClosed = false;
 
     Mutex                      mLock;
 };