Merge "Fix TODOs made during migration to libbinder_ndk" into rvc-dev

cmds/statsd/src/StatsService.cpp

@@ -930,8 +930,6 @@ Status StatsService::informOnePackage(const string& app, int32_t uid, int64_t ve
     ENFORCE_UID(AID_SYSTEM);
 
     VLOG("StatsService::informOnePackage was called");
-    // TODO(b/149254662): This is gross. We should consider changing statsd
-    // internals to use std::string.
     String16 utf16App = String16(app.c_str());
     String16 utf16VersionString = String16(versionString.c_str());
     String16 utf16Installer = String16(installer.c_str());

cmds/statsd/src/anomaly/AlarmMonitor.cpp

@@ -38,8 +38,6 @@ AlarmMonitor::~AlarmMonitor() {}
 void AlarmMonitor::setStatsCompanionService(
         shared_ptr<IStatsCompanionService> statsCompanionService) {
     std::lock_guard<std::mutex> lock(mLock);
-    // TODO(b/149254662): determine if tmpForLock is needed now that we have moved
-    // from sp to shared_ptr
     shared_ptr<IStatsCompanionService> tmpForLock = mStatsCompanionService;
     mStatsCompanionService = statsCompanionService;
     if (statsCompanionService == nullptr) {

cmds/statsd/src/config/ConfigManager.cpp

@@ -54,20 +54,22 @@ struct ConfigReceiverDeathCookie {
     shared_ptr<IPendingIntentRef> mPir;
 };
 
-static void configReceiverDied(void* cookie) {
+void ConfigManager::configReceiverDied(void* cookie) {
     auto cookie_ = static_cast<ConfigReceiverDeathCookie*>(cookie);
-    sp<ConfigManager> configManager = cookie_->mConfigManager;
-    ConfigKey configKey = cookie_->mConfigKey;
-    shared_ptr<IPendingIntentRef> pir = cookie_->mPir;
+    sp<ConfigManager>& thiz = cookie_->mConfigManager;
+    ConfigKey& configKey = cookie_->mConfigKey;
+    shared_ptr<IPendingIntentRef>& pir = cookie_->mPir;
 
-    // TODO(b/149254662): Fix threading. This currently fails if a new
-    // pir gets set between the get and the remove.
-    if (configManager->GetConfigReceiver(configKey) == pir) {
-        configManager->RemoveConfigReceiver(configKey);
+    // Erase the mapping from the config key to the config receiver (pir) if the
+    // mapping still exists.
+    lock_guard<mutex> lock(thiz->mMutex);
+    auto it = thiz->mConfigReceivers.find(configKey);
+    if (it != thiz->mConfigReceivers.end() && it->second == pir) {
+        thiz->mConfigReceivers.erase(configKey);
     }
-    // The death recipient corresponding to this specific pir can never
-    // be triggered again, so free up resources.
-    // TODO(b/149254662): Investigate other options to manage the memory.
+
+    // The death recipient corresponding to this specific pir can never be
+    // triggered again, so free up resources.
     delete cookie_;
 }
 
@@ -83,26 +85,29 @@ struct ActiveConfigChangedReceiverDeathCookie {
     shared_ptr<IPendingIntentRef> mPir;
 };
 
-static void activeConfigChangedReceiverDied(void* cookie) {
+void ConfigManager::activeConfigChangedReceiverDied(void* cookie) {
     auto cookie_ = static_cast<ActiveConfigChangedReceiverDeathCookie*>(cookie);
-    sp<ConfigManager> configManager = cookie_->mConfigManager;
+    sp<ConfigManager>& thiz = cookie_->mConfigManager;
     int uid = cookie_->mUid;
-    shared_ptr<IPendingIntentRef> pir = cookie_->mPir;
+    shared_ptr<IPendingIntentRef>& pir = cookie_->mPir;
 
-    // TODO(b/149254662): Fix threading. This currently fails if a new
-    // pir gets set between the get and the remove.
-    if (configManager->GetActiveConfigsChangedReceiver(uid) == pir) {
-        configManager->RemoveActiveConfigsChangedReceiver(uid);
+    // Erase the mapping from the config key to the active config changed
+    // receiver (pir) if the mapping still exists.
+    lock_guard<mutex> lock(thiz->mMutex);
+    auto it = thiz->mActiveConfigsChangedReceivers.find(uid);
+    if (it != thiz->mActiveConfigsChangedReceivers.end() && it->second == pir) {
+        thiz->mActiveConfigsChangedReceivers.erase(uid);
     }
+
     // The death recipient corresponding to this specific pir can never
     // be triggered again, so free up resources.
     delete cookie_;
 }
 
-ConfigManager::ConfigManager():
+ConfigManager::ConfigManager() :
     mConfigReceiverDeathRecipient(AIBinder_DeathRecipient_new(configReceiverDied)),
     mActiveConfigChangedReceiverDeathRecipient(
-        AIBinder_DeathRecipient_new(activeConfigChangedReceiverDied)) {
+            AIBinder_DeathRecipient_new(activeConfigChangedReceiverDied)) {
 }
 
 ConfigManager::~ConfigManager() {
@@ -189,8 +194,10 @@ void ConfigManager::RemoveConfigReceiver(const ConfigKey& key) {
 
 void ConfigManager::SetActiveConfigsChangedReceiver(const int uid,
                                                     const shared_ptr<IPendingIntentRef>& pir) {
-    lock_guard<mutex> lock(mMutex);
-    mActiveConfigsChangedReceivers[uid] = pir;
+    {
+        lock_guard<mutex> lock(mMutex);
+        mActiveConfigsChangedReceivers[uid] = pir;
+    }
     AIBinder_linkToDeath(pir->asBinder().get(), mActiveConfigChangedReceiverDeathRecipient.get(),
                          new ActiveConfigChangedReceiverDeathCookie(this, uid, pir));
 }

cmds/statsd/src/config/ConfigManager.h

@@ -161,6 +161,19 @@ private:
     // IPendingIntentRef dies.
     ::ndk::ScopedAIBinder_DeathRecipient mConfigReceiverDeathRecipient;
     ::ndk::ScopedAIBinder_DeathRecipient mActiveConfigChangedReceiverDeathRecipient;
+
+    /**
+     * Death recipient callback that is called when a config receiver dies.
+     * The cookie is a pointer to a ConfigReceiverDeathCookie.
+     */
+    static void configReceiverDied(void* cookie);
+
+    /**
+     * Death recipient callback that is called when an active config changed
+     * receiver dies. The cookie is a pointer to an
+     * ActiveConfigChangedReceiverDeathCookie.
+     */
+    static void activeConfigChangedReceiverDied(void* cookie);
 };
 
 }  // namespace statsd

cmds/statsd/src/external/StatsPullerManager.cpp

@@ -85,12 +85,9 @@ void StatsPullerManager::updateAlarmLocked() {
         return;
     }
 
-    // TODO(b/149254662): Why are we creating a copy here? This is different
-    // from the other places where we create a copy because we don't reassign
-    // mStatsCompanionService so a destructor can't implicitly be called...
-    shared_ptr<IStatsCompanionService> statsCompanionServiceCopy = mStatsCompanionService;
-    if (statsCompanionServiceCopy != nullptr) {
-        statsCompanionServiceCopy->setPullingAlarm(mNextPullTimeNs / 1000000);
+    // TODO(b/151045771): do not hold a lock while making a binder call
+    if (mStatsCompanionService != nullptr) {
+        mStatsCompanionService->setPullingAlarm(mNextPullTimeNs / 1000000);
     } else {
         VLOG("StatsCompanionService not available. Alarm not set.");
     }
@@ -99,8 +96,6 @@ void StatsPullerManager::updateAlarmLocked() {
 
 void StatsPullerManager::SetStatsCompanionService(
         shared_ptr<IStatsCompanionService> statsCompanionService) {
-    // TODO(b/149254662): Why are we using AutoMutex instead of lock_guard?
-    // Additionally, do we need the temporary shared_ptr to prevent deadlocks?
     AutoMutex _l(mLock);
     shared_ptr<IStatsCompanionService> tmpForLock = mStatsCompanionService;
     mStatsCompanionService = statsCompanionService;

cmds/statsd/src/subscriber/SubscriberReporter.cpp

@@ -39,33 +39,47 @@ struct BroadcastSubscriberDeathCookie {
     shared_ptr<IPendingIntentRef> mPir;
 };
 
-static void broadcastSubscriberDied(void* cookie) {
-    BroadcastSubscriberDeathCookie* cookie_ = (BroadcastSubscriberDeathCookie*)cookie;
-    ConfigKey configKey = cookie_->mConfigKey;
+void SubscriberReporter::broadcastSubscriberDied(void* cookie) {
+    auto cookie_ = static_cast<BroadcastSubscriberDeathCookie*>(cookie);
+    ConfigKey& configKey = cookie_->mConfigKey;
     int64_t subscriberId = cookie_->mSubscriberId;
-    shared_ptr<IPendingIntentRef> pir = cookie_->mPir;
+    shared_ptr<IPendingIntentRef>& pir = cookie_->mPir;
 
-    // TODO(b/149254662): Fix threading. This currently fails if a new pir gets
-    // set between the get and the unset.
-    if (SubscriberReporter::getInstance().getBroadcastSubscriber(configKey, subscriberId) == pir) {
-        SubscriberReporter::getInstance().unsetBroadcastSubscriber(configKey, subscriberId);
+    SubscriberReporter& thiz = getInstance();
+
+    // Erase the mapping from a (config_key, subscriberId) to a pir if the
+    // mapping exists.
+    lock_guard<mutex> lock(thiz.mLock);
+    auto subscriberMapIt = thiz.mIntentMap.find(configKey);
+    if (subscriberMapIt != thiz.mIntentMap.end()) {
+        auto subscriberMap = subscriberMapIt->second;
+        auto pirIt = subscriberMap.find(subscriberId);
+        if (pirIt != subscriberMap.end() && pirIt->second == pir) {
+            subscriberMap.erase(subscriberId);
+            if (subscriberMap.empty()) {
+                thiz.mIntentMap.erase(configKey);
+            }
+        }
     }
+
     // The death recipient corresponding to this specific pir can never be
     // triggered again, so free up resources.
     delete cookie_;
 }
 
-static ::ndk::ScopedAIBinder_DeathRecipient sBroadcastSubscriberDeathRecipient(
-        AIBinder_DeathRecipient_new(broadcastSubscriberDied));
+SubscriberReporter::SubscriberReporter() :
+    mBroadcastSubscriberDeathRecipient(AIBinder_DeathRecipient_new(broadcastSubscriberDied)) {
+}
 
 void SubscriberReporter::setBroadcastSubscriber(const ConfigKey& configKey,
                                                 int64_t subscriberId,
                                                 const shared_ptr<IPendingIntentRef>& pir) {
     VLOG("SubscriberReporter::setBroadcastSubscriber called.");
-    lock_guard<mutex> lock(mLock);
-    mIntentMap[configKey][subscriberId] = pir;
-    // TODO(b/149254662): Is it ok to call linkToDeath while holding a lock?
-    AIBinder_linkToDeath(pir->asBinder().get(), sBroadcastSubscriberDeathRecipient.get(),
+    {
+        lock_guard<mutex> lock(mLock);
+        mIntentMap[configKey][subscriberId] = pir;
+    }
+    AIBinder_linkToDeath(pir->asBinder().get(), mBroadcastSubscriberDeathRecipient.get(),
                          new BroadcastSubscriberDeathCookie(configKey, subscriberId, pir));
 }
 
@@ -103,8 +117,6 @@ void SubscriberReporter::alertBroadcastSubscriber(const ConfigKey& configKey,
     }
     int64_t subscriberId = subscription.broadcast_subscriber_details().subscriber_id();
 
-    // TODO(b/149254662): Is there a way to convert a RepeatedPtrField into a
-    // vector without copying?
     vector<string> cookies;
     cookies.reserve(subscription.broadcast_subscriber_details().cookie_size());
     for (auto& cookie : subscription.broadcast_subscriber_details().cookie()) {

cmds/statsd/src/subscriber/SubscriberReporter.h

@@ -78,7 +78,7 @@ public:
                                                          int64_t subscriberId);
 
 private:
-    SubscriberReporter() {};
+    SubscriberReporter();
 
     mutable mutex mLock;
 
@@ -94,6 +94,14 @@ private:
                              const Subscription& subscription,
                              const vector<string>& cookies,
                              const MetricDimensionKey& dimKey) const;
+
+    ::ndk::ScopedAIBinder_DeathRecipient mBroadcastSubscriberDeathRecipient;
+
+    /**
+     * Death recipient callback that is called when a broadcast subscriber dies.
+     * The cookie is a pointer to a BroadcastSubscriberDeathCookie.
+     */
+    static void broadcastSubscriberDied(void* cookie);
 };
 
 }  // namespace statsd