Skip to content

Fix Mutex destructors being incorrectly run at exit (cl/364325997) #345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/rest/transport_curl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,12 @@ CurlThread* g_curl_thread = nullptr;
int g_initialize_count = 0;

// Mutex for Curl initialization.
Mutex g_initialize_mutex; // NOLINT
Mutex* g_initialize_mutex = new Mutex();

} // namespace

void InitTransportCurl() {
MutexLock lock(g_initialize_mutex);
MutexLock lock(*g_initialize_mutex);
if (g_initialize_count == 0) {
// Initialize curl.
CURLcode global_init_code = curl_global_init(CURL_GLOBAL_ALL);
Expand All @@ -333,7 +333,7 @@ void InitTransportCurl() {
}

void CleanupTransportCurl() {
MutexLock lock(g_initialize_mutex);
MutexLock lock(*g_initialize_mutex);
assert(g_initialize_count > 0);
g_initialize_count--;
if (g_initialize_count == 0) {
Expand Down
10 changes: 5 additions & 5 deletions app/rest/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,23 @@ const char kGet[] = "GET";
const char kPost[] = "POST";

// Mutex for utils initialization and distribution of CURL pointers.
static ::firebase::Mutex g_util_curl_mutex; // NOLINT
static ::firebase::Mutex* g_util_curl_mutex = new Mutex();

CURL* g_curl_instance = nullptr;
int g_init_ref_count = 0;

void* CreateCurlPtr() { // NOLINT
MutexLock lock(g_util_curl_mutex);
MutexLock lock(*g_util_curl_mutex);
return curl_easy_init();
}

void DestroyCurlPtr(void* curl_ptr) {
MutexLock lock(g_util_curl_mutex);
MutexLock lock(*g_util_curl_mutex);
curl_easy_cleanup(static_cast<CURL*>(curl_ptr));
}

void Initialize() {
MutexLock curl_lock(g_util_curl_mutex);
MutexLock curl_lock(*g_util_curl_mutex);
assert(g_init_ref_count >= 0);
if (g_init_ref_count == 0) {
g_curl_instance = reinterpret_cast<CURL*>(CreateCurlPtr());
Expand All @@ -70,7 +70,7 @@ void Initialize() {
}

void Terminate() {
MutexLock curl_lock(g_util_curl_mutex);
MutexLock curl_lock(*g_util_curl_mutex);
--g_init_ref_count;
assert(g_init_ref_count >= 0);
if (g_init_ref_count == 0) {
Expand Down
24 changes: 12 additions & 12 deletions app/src/app_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ class LibraryRegistry {
};

// Guards g_apps and g_default_app.
static Mutex g_app_mutex; // NOLINT
static Mutex* g_app_mutex = new Mutex();
static std::map<std::string, UniquePtr<AppData>>* g_apps;
static App* g_default_app = nullptr;
LibraryRegistry* LibraryRegistry::library_registry_ = nullptr;
Expand All @@ -265,7 +265,7 @@ App* AddApp(App* app, std::map<std::string, InitResult>* results) {
assert(app);
App* existing_app = FindAppByName(app->name());
FIREBASE_ASSERT_RETURN(nullptr, !existing_app);
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
if (IsDefaultAppName(app->name())) {
assert(!g_default_app);
g_default_app = app;
Expand Down Expand Up @@ -309,7 +309,7 @@ App* AddApp(App* app, std::map<std::string, InitResult>* results) {

App* FindAppByName(const char* name) {
assert(name);
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
if (g_apps) {
auto it = g_apps->find(std::string(name));
if (it == g_apps->end()) return nullptr;
Expand All @@ -325,7 +325,7 @@ App* GetAnyApp() {
return g_default_app;
}

MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
if (g_apps && !g_apps->empty()) {
return g_apps->begin()->second->app;
}
Expand All @@ -334,7 +334,7 @@ App* GetAnyApp() {

void RemoveApp(App* app) {
assert(app);
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
if (g_apps) {
auto it = g_apps->find(std::string(app->name()));
bool last_app = false;
Expand Down Expand Up @@ -369,7 +369,7 @@ void RemoveApp(App* app) {
void DestroyAllApps() {
std::vector<App*> apps_to_delete;
App* const default_app = GetDefaultApp();
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
if (g_apps) {
for (auto it = g_apps->begin(); it != g_apps->end(); ++it) {
if (it->second->app != default_app)
Expand All @@ -391,15 +391,15 @@ bool IsDefaultAppName(const char* name) {
}

void RegisterLibrary(const char* library, const char* version) {
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
LibraryRegistry* registry = LibraryRegistry::Initialize();
if (registry->RegisterLibrary(library, version)) {
registry->UpdateUserAgent();
}
}

void RegisterLibrariesFromUserAgent(const char* user_agent) {
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
LibraryRegistry* registry = LibraryRegistry::Initialize();
// Copy the string into a vector so that we can safely mutate the string.
std::vector<char> user_agent_vector(user_agent,
Expand Down Expand Up @@ -427,12 +427,12 @@ void RegisterLibrariesFromUserAgent(const char* user_agent) {
}

const char* GetUserAgent() {
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
return LibraryRegistry::Initialize()->GetUserAgent();
}

std::string GetLibraryVersion(const char* library) {
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
return LibraryRegistry::Initialize()->GetLibraryVersion(library);
}

Expand All @@ -442,7 +442,7 @@ void GetOuterMostSdkAndVersion(std::string* sdk, std::string* version) {
sdk->clear();
version->clear();

MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
// Set of library versions to query in order of outer wrapper to inner.
// We're only retrieving the outer-most SDK version here as we can only send
// one component as part of the user agent string to the storage backend.
Expand All @@ -466,7 +466,7 @@ void GetOuterMostSdkAndVersion(std::string* sdk, std::string* version) {
// Find a logger associated with an app by app name.
Logger* FindAppLoggerByName(const char* name) {
assert(name);
MutexLock lock(g_app_mutex);
MutexLock lock(*g_app_mutex);
if (g_apps) {
auto it = g_apps->find(std::string(name));
if (it == g_apps->end()) return nullptr;
Expand Down
12 changes: 6 additions & 6 deletions app/src/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,13 @@ class CallbackDispatcher {

static CallbackDispatcher* g_callback_dispatcher = nullptr;
// Mutex that controls access to g_callback_dispatcher and g_callback_ref_count.
static Mutex g_callback_mutex; // NOLINT
static Mutex* g_callback_mutex = new Mutex();
static int g_callback_ref_count = 0;
static Thread::Id g_callback_thread_id;
static bool g_callback_thread_id_initialized = false;

void Initialize() {
MutexLock lock(g_callback_mutex);
MutexLock lock(*g_callback_mutex);
if (g_callback_ref_count == 0) {
g_callback_dispatcher = new CallbackDispatcher();
}
Expand All @@ -202,7 +202,7 @@ void Initialize() {

// Add a reference to the module if it's already initialized.
static bool InitializeIfInitialized() {
MutexLock lock(g_callback_mutex);
MutexLock lock(*g_callback_mutex);
if (IsInitialized()) {
Initialize();
return true;
Expand All @@ -217,7 +217,7 @@ bool IsInitialized() { return g_callback_ref_count > 0; }
static void Terminate(int number_of_references_to_remove) {
CallbackDispatcher* dispatcher_to_destroy = nullptr;
{
MutexLock lock(g_callback_mutex);
MutexLock lock(*g_callback_mutex);
if (!g_callback_ref_count) {
LogWarning("Callback module already shut down");
return;
Expand All @@ -239,7 +239,7 @@ static void Terminate(int number_of_references_to_remove) {
}

void Terminate(bool flush_all) {
MutexLock lock(g_callback_mutex);
MutexLock lock(*g_callback_mutex);
int ref_count = 1;
// g_callback_ref_count is used to track the number of current references to
// g_callback_dispatcher so we need to decrement the reference count by just
Expand All @@ -253,7 +253,7 @@ void Terminate(bool flush_all) {
}

void* AddCallback(Callback* callback) {
MutexLock lock(g_callback_mutex);
MutexLock lock(*g_callback_mutex);
Initialize();
return g_callback_dispatcher->AddCallback(callback);
}
Expand Down
16 changes: 8 additions & 8 deletions app/src/cleanup_notifier.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@

namespace FIREBASE_NAMESPACE {

Mutex CleanupNotifier::cleanup_notifiers_by_owner_mutex_; // NOLINT
Mutex* CleanupNotifier::cleanup_notifiers_by_owner_mutex_ = new Mutex();
std::map<void *, CleanupNotifier *>
*CleanupNotifier::cleanup_notifiers_by_owner_;

CleanupNotifier::CleanupNotifier() : cleaned_up_(false) {
MutexLock lock(cleanup_notifiers_by_owner_mutex_);
MutexLock lock(*cleanup_notifiers_by_owner_mutex_);
if (!cleanup_notifiers_by_owner_) {
cleanup_notifiers_by_owner_ = new std::map<void *, CleanupNotifier *>();
}
Expand All @@ -41,7 +41,7 @@ CleanupNotifier::~CleanupNotifier() {
CleanupAll();
UnregisterAllOwners();
{
MutexLock lock(cleanup_notifiers_by_owner_mutex_);
MutexLock lock(*cleanup_notifiers_by_owner_mutex_);
if (cleanup_notifiers_by_owner_ && cleanup_notifiers_by_owner_->empty()) {
delete cleanup_notifiers_by_owner_;
cleanup_notifiers_by_owner_ = nullptr;
Expand Down Expand Up @@ -78,14 +78,14 @@ void CleanupNotifier::CleanupAll() {
}

void CleanupNotifier::UnregisterAllOwners() {
MutexLock lock(cleanup_notifiers_by_owner_mutex_);
MutexLock lock(*cleanup_notifiers_by_owner_mutex_);
while (owners_.begin() != owners_.end()) {
UnregisterOwner(this, owners_[0]);
}
}

void CleanupNotifier::RegisterOwner(CleanupNotifier *notifier, void *owner) {
MutexLock lock(cleanup_notifiers_by_owner_mutex_);
MutexLock lock(*cleanup_notifiers_by_owner_mutex_);
assert(cleanup_notifiers_by_owner_);
auto it = cleanup_notifiers_by_owner_->find(owner);
if (it != cleanup_notifiers_by_owner_->end()) UnregisterOwner(it);
Expand All @@ -94,15 +94,15 @@ void CleanupNotifier::RegisterOwner(CleanupNotifier *notifier, void *owner) {
}

void CleanupNotifier::UnregisterOwner(CleanupNotifier *notifier, void *owner) {
MutexLock lock(cleanup_notifiers_by_owner_mutex_);
MutexLock lock(*cleanup_notifiers_by_owner_mutex_);
assert(cleanup_notifiers_by_owner_);
auto it = cleanup_notifiers_by_owner_->find(owner);
if (it != cleanup_notifiers_by_owner_->end()) UnregisterOwner(it);
}

void CleanupNotifier::UnregisterOwner(
std::map<void *, CleanupNotifier *>::iterator it) {
MutexLock lock(cleanup_notifiers_by_owner_mutex_);
MutexLock lock(*cleanup_notifiers_by_owner_mutex_);
assert(cleanup_notifiers_by_owner_);
void *owner = it->first;
CleanupNotifier *notifier = it->second;
Expand All @@ -114,7 +114,7 @@ void CleanupNotifier::UnregisterOwner(
}

CleanupNotifier *CleanupNotifier::FindByOwner(void *owner) {
MutexLock lock(cleanup_notifiers_by_owner_mutex_);
MutexLock lock(*cleanup_notifiers_by_owner_mutex_);
if (!cleanup_notifiers_by_owner_) return nullptr;
auto it = cleanup_notifiers_by_owner_->find(owner);
return it != cleanup_notifiers_by_owner_->end() ? it->second : nullptr;
Expand Down
2 changes: 1 addition & 1 deletion app/src/cleanup_notifier.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class CleanupNotifier {
std::vector<void *> owners_;

// Guards owners_ and cleanup_notifiers_by_owner_.
static Mutex cleanup_notifiers_by_owner_mutex_;
static Mutex* cleanup_notifiers_by_owner_mutex_;
// Global map of cleanup notifiers bucketed by owner object.
static std::map<void *, CleanupNotifier *> *cleanup_notifiers_by_owner_;
};
Expand Down
6 changes: 3 additions & 3 deletions app/src/secure/user_secure_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ namespace secure {

using callback::NewCallback;

Mutex UserSecureManager::s_scheduler_mutex_; // NOLINT
Mutex* UserSecureManager::s_scheduler_mutex_ = new Mutex();
scheduler::Scheduler* UserSecureManager::s_scheduler_;
int32_t UserSecureManager::s_scheduler_ref_count_;

Expand Down Expand Up @@ -184,7 +184,7 @@ Future<void> UserSecureManager::DeleteAllData() {
}

void UserSecureManager::CreateScheduler() {
MutexLock lock(s_scheduler_mutex_);
MutexLock lock(*s_scheduler_mutex_);
if (s_scheduler_ == nullptr) {
s_scheduler_ = new scheduler::Scheduler();
// reset count
Expand All @@ -194,7 +194,7 @@ void UserSecureManager::CreateScheduler() {
}

void UserSecureManager::DestroyScheduler() {
MutexLock lock(s_scheduler_mutex_);
MutexLock lock(*s_scheduler_mutex_);
if (s_scheduler_ == nullptr) {
// reset count
s_scheduler_ref_count_ = 0;
Expand Down
2 changes: 1 addition & 1 deletion app/src/secure/user_secure_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class UserSecureManager {
static void DestroyScheduler();

// Guards static scheduler pointer
static Mutex s_scheduler_mutex_; // NOLINT
static Mutex* s_scheduler_mutex_;
static scheduler::Scheduler* s_scheduler_;
static int32_t s_scheduler_ref_count_;

Expand Down
Loading