Fix race with Asset destruction and printing allocation stats

am: 2582465bb4

Change-Id: I1dbb7609fa4d770e513e6f628e4c598600607383

include/androidfw/Asset.h

@@ -44,7 +44,7 @@ namespace android {
  */
 class Asset {
 public:
-    virtual ~Asset(void);
+    virtual ~Asset(void) = default;
 
     static int32_t getGlobalCount();
     static String8 getAssetAllocations();
@@ -119,6 +119,19 @@ public:
     const char* getAssetSource(void) const { return mAssetSource.string(); }
 
 protected:
+    /*
+     * Adds this Asset to the global Asset list for debugging and
+     * accounting.
+     * Concrete subclasses must call this in their constructor.
+     */
+    static void registerAsset(Asset* asset);
+
+    /*
+     * Removes this Asset from the global Asset list.
+     * Concrete subclasses must call this in their destructor.
+     */
+    static void unregisterAsset(Asset* asset);
+
     Asset(void);        // constructor; only invoked indirectly
 
     /* handle common seek() housekeeping */

libs/androidfw/Asset.cpp

@@ -52,6 +52,47 @@ static int32_t gCount = 0;
 static Asset* gHead = NULL;
 static Asset* gTail = NULL;
 
+void Asset::registerAsset(Asset* asset)
+{
+    AutoMutex _l(gAssetLock);
+    gCount++;
+    asset->mNext = asset->mPrev = NULL;
+    if (gTail == NULL) {
+        gHead = gTail = asset;
+    } else {
+        asset->mPrev = gTail;
+        gTail->mNext = asset;
+        gTail = asset;
+    }
+
+    if (kIsDebug) {
+        ALOGI("Creating Asset %p #%d\n", asset, gCount);
+    }
+}
+
+void Asset::unregisterAsset(Asset* asset)
+{
+    AutoMutex _l(gAssetLock);
+    gCount--;
+    if (gHead == asset) {
+        gHead = asset->mNext;
+    }
+    if (gTail == asset) {
+        gTail = asset->mPrev;
+    }
+    if (asset->mNext != NULL) {
+        asset->mNext->mPrev = asset->mPrev;
+    }
+    if (asset->mPrev != NULL) {
+        asset->mPrev->mNext = asset->mNext;
+    }
+    asset->mNext = asset->mPrev = NULL;
+
+    if (kIsDebug) {
+        ALOGI("Destroying Asset in %p #%d\n", asset, gCount);
+    }
+}
+
 int32_t Asset::getGlobalCount()
 {
     AutoMutex _l(gAssetLock);
@@ -79,43 +120,8 @@ String8 Asset::getAssetAllocations()
 }
 
 Asset::Asset(void)
-    : mAccessMode(ACCESS_UNKNOWN)
+    : mAccessMode(ACCESS_UNKNOWN), mNext(NULL), mPrev(NULL)
 {
-    AutoMutex _l(gAssetLock);
-    gCount++;
-    mNext = mPrev = NULL;
-    if (gTail == NULL) {
-        gHead = gTail = this;
-    } else {
-        mPrev = gTail;
-        gTail->mNext = this;
-        gTail = this;
-    }
-    if (kIsDebug) {
-        ALOGI("Creating Asset %p #%d\n", this, gCount);
-    }
-}
-
-Asset::~Asset(void)
-{
-    AutoMutex _l(gAssetLock);
-    gCount--;
-    if (gHead == this) {
-        gHead = mNext;
-    }
-    if (gTail == this) {
-        gTail = mPrev;
-    }
-    if (mNext != NULL) {
-        mNext->mPrev = mPrev;
-    }
-    if (mPrev != NULL) {
-        mPrev->mNext = mNext;
-    }
-    mNext = mPrev = NULL;
-    if (kIsDebug) {
-        ALOGI("Destroying Asset in %p #%d\n", this, gCount);
-    }
 }
 
 /*
@@ -361,6 +367,9 @@ off64_t Asset::handleSeek(off64_t offset, int whence, off64_t curPosn, off64_t m
 _FileAsset::_FileAsset(void)
     : mStart(0), mLength(0), mOffset(0), mFp(NULL), mFileName(NULL), mMap(NULL), mBuf(NULL)
 {
+    // Register the Asset with the global list here after it is fully constructed and its
+    // vtable pointer points to this concrete type. b/31113965
+    registerAsset(this);
 }
 
 /*
@@ -369,6 +378,10 @@ _FileAsset::_FileAsset(void)
 _FileAsset::~_FileAsset(void)
 {
     close();
+
+    // Unregister the Asset from the global list here before it is destructed and while its vtable
+    // pointer still points to this concrete type. b/31113965
+    unregisterAsset(this);
 }
 
 /*
@@ -685,6 +698,9 @@ _CompressedAsset::_CompressedAsset(void)
     : mStart(0), mCompressedLen(0), mUncompressedLen(0), mOffset(0),
       mMap(NULL), mFd(-1), mZipInflater(NULL), mBuf(NULL)
 {
+    // Register the Asset with the global list here after it is fully constructed and its
+    // vtable pointer points to this concrete type. b/31113965
+    registerAsset(this);
 }
 
 /*
@@ -693,6 +709,10 @@ _CompressedAsset::_CompressedAsset(void)
 _CompressedAsset::~_CompressedAsset(void)
 {
     close();
+
+    // Unregister the Asset from the global list here before it is destructed and while its vtable
+    // pointer still points to this concrete type. b/31113965
+    unregisterAsset(this);
 }
 
 /*

libs/androidfw/tests/Android.mk

@@ -22,6 +22,7 @@ LOCAL_PATH:= $(call my-dir)
 
 testFiles := \
     AppAsLib_test.cpp \
+    Asset_test.cpp \
     AttributeFinder_test.cpp \
     ByteBucketArray_test.cpp \
     Config_test.cpp \

libs/androidfw/tests/Asset_test.cpp

@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <androidfw/Asset.h>
+
+#include <gtest/gtest.h>
+
+using namespace android;
+
+TEST(AssetTest, FileAssetRegistersItself) {
+    const int32_t count = Asset::getGlobalCount();
+    Asset* asset = new _FileAsset();
+    EXPECT_EQ(count + 1, Asset::getGlobalCount());
+    delete asset;
+    EXPECT_EQ(count, Asset::getGlobalCount());
+}
+
+TEST(AssetTest, CompressedAssetRegistersItself) {
+    const int32_t count = Asset::getGlobalCount();
+    Asset* asset = new _CompressedAsset();
+    EXPECT_EQ(count + 1, Asset::getGlobalCount());
+    delete asset;
+    EXPECT_EQ(count, Asset::getGlobalCount());
+}