Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c93cac27 authored by Emilian Peev's avatar Emilian Peev
Browse files

Camera: Track provider instances 

In some very rare corner cases where the binder death notification
gets delayed and races against the new provider 'onRegistration'
callback, camera service can temporarily hold two camera providers
with the same name. Depending on scheduling, if 'onRegistration'
executes first, it will get rejected as the provider name already
exists. The subsequent binder death notification callback will
remove all camera devices.
To avoid this, track all camera providers with a combination of the
provider name and unique instance id. If we receive a registration
callback with provider name that already exists, defer the
initialization sequence until the previous instance gets released
correctly.

Bug: 164019313
Test: cameraservice_test
--gtest_filter=CameraProviderManagerTest.BinderDeathRegistrationRaceTest,
Camera CTS,
Partner testing

Change-Id: I207a12da1fb09b1c45dae8697049f1fec34627d3
parent 02ebd761

services/camera/libcameraservice/common/CameraProviderManager.cpp

+55 −21
Original line number Diff line number Diff line
@@ -474,12 +474,12 @@ void CameraProviderManager::removeRef(DeviceMode usageType, const std::string &c 
hardware::Return<void> CameraProviderManager::onRegistration(

        const hardware::hidl_string& /*fqName*/,

        const hardware::hidl_string& name,

        bool /*preexisting*/) {

        bool preexisting) {

    std::lock_guard<std::mutex> providerLock(mProviderLifecycleLock);

    {

        std::lock_guard<std::mutex> lock(mInterfaceMutex);



        addProviderLocked(name);

        addProviderLocked(name, preexisting);

    }



    sp<StatusListener> listener = getStatusListener();
@@ -1230,33 +1230,53 @@ CameraProviderManager::isHiddenPhysicalCameraInternal(const std::string& cameraI 
    return falseRet;

}



status_t CameraProviderManager::addProviderLocked(const std::string& newProvider) {

    for (const auto& providerInfo : mProviders) {

        if (providerInfo->mProviderName == newProvider) {

            ALOGW("%s: Camera provider HAL with name '%s' already registered", __FUNCTION__,

                    newProvider.c_str());

            return ALREADY_EXISTS;

        }

    }



status_t CameraProviderManager::tryToInitializeProviderLocked(

        const std::string& providerName, const sp<ProviderInfo>& providerInfo) {

    sp<provider::V2_4::ICameraProvider> interface;

    interface = mServiceProxy->tryGetService(newProvider);

    interface = mServiceProxy->tryGetService(providerName);



    if (interface == nullptr) {

        // The interface may not be started yet. In that case, this is not a

        // fatal error.

        ALOGW("%s: Camera provider HAL '%s' is not actually available", __FUNCTION__,

                newProvider.c_str());

                providerName.c_str());

        return BAD_VALUE;

    }



    sp<ProviderInfo> providerInfo = new ProviderInfo(newProvider, this);

    status_t res = providerInfo->initialize(interface, mDeviceState);

    return providerInfo->initialize(interface, mDeviceState);

}



status_t CameraProviderManager::addProviderLocked(const std::string& newProvider,

        bool preexisting) {

    // Several camera provider instances can be temporarily present.

    // Defer initialization of a new instance until the older instance is properly removed.

    auto providerInstance = newProvider + "-" + std::to_string(mProviderInstanceId);

    bool providerPresent = false;

    for (const auto& providerInfo : mProviders) {

        if (providerInfo->mProviderName == newProvider) {

            ALOGW("%s: Camera provider HAL with name '%s' already registered",

                    __FUNCTION__, newProvider.c_str());

            if (preexisting) {

                return ALREADY_EXISTS;

            } else{

                ALOGW("%s: The new provider instance will get initialized immediately after the"

                        " currently present instance is removed!", __FUNCTION__);

                providerPresent = true;

                break;

            }

        }

    }



    sp<ProviderInfo> providerInfo = new ProviderInfo(newProvider, providerInstance, this);

    if (!providerPresent) {

        status_t res = tryToInitializeProviderLocked(newProvider, providerInfo);

        if (res != OK) {

            return res;

        }

    }



    mProviders.push_back(providerInfo);

    mProviderInstanceId++;



    return OK;

}
@@ -1266,12 +1286,14 @@ status_t CameraProviderManager::removeProvider(const std::string& provider) { 
    std::unique_lock<std::mutex> lock(mInterfaceMutex);

    std::vector<String8> removedDeviceIds;

    status_t res = NAME_NOT_FOUND;

    std::string removedProviderName;

    for (auto it = mProviders.begin(); it != mProviders.end(); it++) {

        if ((*it)->mProviderName == provider) {

        if ((*it)->mProviderInstance == provider) {

            removedDeviceIds.reserve((*it)->mDevices.size());

            for (auto& deviceInfo : (*it)->mDevices) {

                removedDeviceIds.push_back(String8(deviceInfo->mId.c_str()));

            }

            removedProviderName = (*it)->mProviderName;

            mProviders.erase(it);

            res = OK;

            break;
@@ -1281,6 +1303,14 @@ status_t CameraProviderManager::removeProvider(const std::string& provider) { 
        ALOGW("%s: Camera provider HAL with name '%s' is not registered", __FUNCTION__,

                provider.c_str());

    } else {

        // Check if there are any newer camera instances from the same provider and try to

        // initialize.

        for (const auto& providerInfo : mProviders) {

            if (providerInfo->mProviderName == removedProviderName) {

                return tryToInitializeProviderLocked(removedProviderName, providerInfo);

            }

        }



        // Inform camera service of loss of presence for all the devices from this provider,

        // without lock held for reentrancy

        sp<StatusListener> listener = getStatusListener();
@@ -1289,7 +1319,9 @@ status_t CameraProviderManager::removeProvider(const std::string& provider) { 
            for (auto& id : removedDeviceIds) {

                listener->onDeviceStatusChanged(id, CameraDeviceStatus::NOT_PRESENT);

            }

            lock.lock();

        }



    }

    return res;

}
@@ -1303,8 +1335,10 @@ sp<CameraProviderManager::StatusListener> CameraProviderManager::getStatusListen 


CameraProviderManager::ProviderInfo::ProviderInfo(

        const std::string &providerName,

        const std::string &providerInstance,

        CameraProviderManager *manager) :

        mProviderName(providerName),

        mProviderInstance(providerInstance),

        mProviderTagid(generateVendorTagId(providerName)),

        mUniqueDeviceCount(0),

        mManager(manager) {
@@ -1628,7 +1662,7 @@ void CameraProviderManager::ProviderInfo::removeDevice(std::string id) { 


status_t CameraProviderManager::ProviderInfo::dump(int fd, const Vector<String16>&) const {

    dprintf(fd, "== Camera Provider HAL %s (v2.%d, %s) static info: %zu devices: ==\n",

            mProviderName.c_str(),

            mProviderInstance.c_str(),

            mMinorVersion,

            mIsRemote ? "remote" : "passthrough",

            mDevices.size());
@@ -1944,12 +1978,12 @@ hardware::Return<void> CameraProviderManager::ProviderInfo::torchModeStatusChang 
void CameraProviderManager::ProviderInfo::serviceDied(uint64_t cookie,

        const wp<hidl::base::V1_0::IBase>& who) {

    (void) who;

    ALOGI("Camera provider '%s' has died; removing it", mProviderName.c_str());

    ALOGI("Camera provider '%s' has died; removing it", mProviderInstance.c_str());

    if (cookie != mId) {

        ALOGW("%s: Unexpected serviceDied cookie %" PRIu64 ", expected %" PRIu32,

                __FUNCTION__, cookie, mId);

    }

    mManager->removeProvider(mProviderName);

    mManager->removeProvider(mProviderInstance);

}



status_t CameraProviderManager::ProviderInfo::setUpVendorTags() {

services/camera/libcameraservice/common/CameraProviderManager.h

+7 −2
Original line number Diff line number Diff line
@@ -365,6 +365,7 @@ private: 
            virtual public hardware::hidl_death_recipient

    {

        const std::string mProviderName;

        const std::string mProviderInstance;

        const metadata_vendor_id_t mProviderTagid;

        int mMinorVersion;

        sp<VendorTagDescriptor> mVendorTagDescriptor;
@@ -379,7 +380,7 @@ private: 


        sp<hardware::camera::provider::V2_4::ICameraProvider> mSavedInterface;



        ProviderInfo(const std::string &providerName,

        ProviderInfo(const std::string &providerName, const std::string &providerInstance,

                CameraProviderManager *manager);

        ~ProviderInfo();
@@ -657,7 +658,10 @@ private: 
            hardware::hidl_version minVersion = hardware::hidl_version{0,0},

            hardware::hidl_version maxVersion = hardware::hidl_version{1000,0}) const;



    status_t addProviderLocked(const std::string& newProvider);

    status_t addProviderLocked(const std::string& newProvider, bool preexisting = false);



    status_t tryToInitializeProviderLocked(const std::string& providerName,

            const sp<ProviderInfo>& providerInfo);



    bool isLogicalCameraLocked(const std::string& id, std::vector<std::string>* physicalCameraIds);
@@ -666,6 +670,7 @@ private: 


    bool isValidDeviceLocked(const std::string &id, uint16_t majorVersion) const;



    size_t mProviderInstanceId = 0;

    std::vector<sp<ProviderInfo>> mProviders;



    void addProviderToMap(

services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp

+65 −0
Original line number Diff line number Diff line
@@ -23,7 +23,9 @@ 
#include <android/hardware/camera/device/3.2/ICameraDeviceCallback.h>

#include <android/hardware/camera/device/3.2/ICameraDeviceSession.h>

#include <camera_metadata_hidden.h>

#include <hidl/HidlBinderSupport.h>

#include <gtest/gtest.h>

#include <utility>



using namespace android;

using namespace android::hardware::camera;
@@ -173,6 +175,25 @@ struct TestICameraProvider : virtual public provider::V2_5::ICameraProvider { 
        return hardware::Void();

    }



    virtual ::android::hardware::Return<bool> linkToDeath(

            const ::android::sp<::android::hardware::hidl_death_recipient>& recipient,

            uint64_t cookie) {

        if (mInitialDeathRecipient.get() == nullptr) {

            mInitialDeathRecipient =

                std::make_unique<::android::hardware::hidl_binder_death_recipient>(recipient,

                        cookie, this);

        }

        return true;

    }



    void signalInitialBinderDeathRecipient() {

        if (mInitialDeathRecipient.get() != nullptr) {

            mInitialDeathRecipient->binderDied(nullptr /*who*/);

        }

    }



    std::unique_ptr<::android::hardware::hidl_binder_death_recipient> mInitialDeathRecipient;



    enum MethodNames {

        SET_CALLBACK,

        GET_VENDOR_TAGS,
@@ -567,3 +588,47 @@ TEST(CameraProviderManagerTest, BadHalStartupTest) { 
    ASSERT_EQ(serviceProxy.mLastRequestedServiceNames.back(), testProviderInstanceName) <<

            "Incorrect instance requested from service manager";

}



// Test that CameraProviderManager can handle races between provider death notifications and

// provider registration callbacks

TEST(CameraProviderManagerTest, BinderDeathRegistrationRaceTest) {



    std::vector<hardware::hidl_string> deviceNames;

    deviceNames.push_back("device@3.2/test/0");

    deviceNames.push_back("device@3.2/test/1");

    hardware::hidl_vec<common::V1_0::VendorTagSection> vendorSection;

    status_t res;



    sp<CameraProviderManager> providerManager = new CameraProviderManager();

    sp<TestStatusListener> statusListener = new TestStatusListener();

    TestInteractionProxy serviceProxy;

    sp<TestICameraProvider> provider =  new TestICameraProvider(deviceNames,

            vendorSection);



    // Not setting up provider in the service proxy yet, to test cases where a

    // HAL isn't starting right

    res = providerManager->initialize(statusListener, &serviceProxy);

    ASSERT_EQ(res, OK) << "Unable to initialize provider manager";



    // Now set up provider and trigger a registration

    serviceProxy.setProvider(provider);



    hardware::hidl_string testProviderFqInterfaceName =

            "android.hardware.camera.provider@2.4::ICameraProvider";

    hardware::hidl_string testProviderInstanceName = "test/0";

    serviceProxy.mManagerNotificationInterface->onRegistration(

            testProviderFqInterfaceName,

            testProviderInstanceName, false);



    // Simulate artificial delay of the registration callback which arrives before the

    // death notification

    serviceProxy.mManagerNotificationInterface->onRegistration(

            testProviderFqInterfaceName,

            testProviderInstanceName, false);



    provider->signalInitialBinderDeathRecipient();



    auto deviceCount = static_cast<unsigned> (providerManager->getCameraCount().second);

    ASSERT_EQ(deviceCount, deviceNames.size()) <<

            "Unexpected amount of camera devices";

}