From ebac8196a1bd66a7455533b94d5bd38d68e142f3 Mon Sep 17 00:00:00 2001 From: austinbenoit Date: Mon, 26 Oct 2026 15:22:38 +0000 Subject: [PATCH 1/5] Make per-App instance registries safe during static destruction Each product keeps a process-wide map of App -> instance plus a mutex, and both are touched from the instance's destructor via DeleteInternal(). When a user holds an instance in a static (e.g. a static std::unique_ptr) that is destroyed after the SDK's own globals, the destructor accesses an already-destroyed map or mutex. Auth and Remote Config stored the map as a static std::map, which produced a heap-use-after-free in ~Auth()/~RemoteConfig(). Database, Storage, Functions, Installations and App Check already heap-allocated the map but still used a static Mutex, which is destroyed at exit before the lock in DeleteInternal(). Bring every product in line with the layout App and Firestore already use: the map is lazily allocated and freed when the last instance goes away, and the mutex is allocated once and never freed. Fixes #110 --- app_check/src/common/app_check.cc | 11 ++++++--- auth/src/auth.cc | 35 +++++++++++++++++++++------ database/src/common/database.cc | 9 ++++--- functions/src/common/functions.cc | 9 ++++--- installations/src/installations.cc | 16 +++++++----- release_build_files/readme.md | 8 ++++++ remote_config/src/remote_config.cc | 39 +++++++++++++++++++++++------- storage/src/common/storage.cc | 9 ++++--- 8 files changed, 100 insertions(+), 36 deletions(-) diff --git a/app_check/src/common/app_check.cc b/app_check/src/common/app_check.cc index 4e3c065488..538785193c 100644 --- a/app_check/src/common/app_check.cc +++ b/app_check/src/common/app_check.cc @@ -61,7 +61,10 @@ namespace app_check { DEFINE_FIREBASE_VERSION_STRING(FirebaseAppCheck); -Mutex g_app_check_lock; // NOLINT +// Allocated once and never freed so that it is not destroyed by the C++ +// runtime at exit: an AppCheck owned by a static object may be destroyed after +// this file's globals, and its destructor must still be able to lock this. +Mutex* g_app_check_lock = new Mutex(); static std::map<::firebase::App*, AppCheck*>* g_app_check_map = nullptr; // Define the destructors for the virtual listener/provider/factory classes. @@ -86,7 +89,7 @@ namespace internal { AppCheck* GetExistingAppCheckInstance(::firebase::App* app) { if (!app) return nullptr; - MutexLock lock(g_app_check_lock); + MutexLock lock(*g_app_check_lock); if (g_app_check_map) { auto it = g_app_check_map->find(app); if (it != g_app_check_map->end()) { @@ -101,7 +104,7 @@ AppCheck* GetExistingAppCheckInstance(::firebase::App* app) { AppCheck* AppCheck::GetInstance(::firebase::App* app) { if (!app) return nullptr; - MutexLock lock(g_app_check_lock); + MutexLock lock(*g_app_check_lock); if (!g_app_check_map) { g_app_check_map = new std::map<::firebase::App*, AppCheck*>(); } @@ -127,7 +130,7 @@ AppCheck::AppCheck(::firebase::App* app) { AppCheck::~AppCheck() { DeleteInternal(); } void AppCheck::DeleteInternal() { - MutexLock lock(g_app_check_lock); + MutexLock lock(*g_app_check_lock); if (!internal_) return; diff --git a/auth/src/auth.cc b/auth/src/auth.cc index b1417f63f1..90922631dd 100644 --- a/auth/src/auth.cc +++ b/auth/src/auth.cc @@ -59,7 +59,16 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseAuth); // and an Auth requires an App. So, the user acquires the Auth via an App // reference. // TODO(jsanmiya): Ensure all the Auths are destroyed on shutdown. -std::map g_auths; +// +// The map is allocated on first use and freed when the last Auth is deleted. +// The mutex is allocated once and never freed: it cannot be safely deleted +// while another thread may be about to lock it, and it is cheap to keep for +// the lifetime of the process. Neither is a static object with a destructor, +// so an Auth owned by a static (e.g. a static std::unique_ptr) can still +// be destroyed safely after this file's globals would otherwise have been torn +// down by the C++ runtime. +using AuthMap = std::map; +static AuthMap* g_auths = nullptr; Mutex* g_auths_mutex = new Mutex(); // static @@ -85,7 +94,10 @@ Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { // Stick it in the global map so we remember it, and can delete it on // shutdown. - g_auths[app] = auth; + if (!g_auths) { + g_auths = new AuthMap(); + } + (*g_auths)[app] = auth; if (init_result_out) *init_result_out = kInitResultSuccess; return auth; @@ -94,9 +106,10 @@ Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { // static Auth* Auth::FindAuth(App* app) { MutexLock lock(*g_auths_mutex); + if (!g_auths) return nullptr; // Return the Auth if it already exists. - std::map::iterator it = g_auths.find(app); - if (it != g_auths.end()) { + AuthMap::iterator it = g_auths->find(app); + if (it != g_auths->end()) { return it->second; } return nullptr; @@ -148,18 +161,24 @@ void Auth::DeleteInternal() { notifier->UnregisterObject(this); int num_auths_remaining = 0; - { + if (g_auths) { // Remove `this` from the g_auths map. // The mapping is 1:1, so we should only ever delete one. - for (auto it = g_auths.begin(); it != g_auths.end(); ++it) { + for (auto it = g_auths->begin(); it != g_auths->end(); ++it) { if (it->second == this) { LogDebug("Deleting Auth %p for App %p", this, it->first); - g_auths.erase(it); + g_auths->erase(it); break; } } - num_auths_remaining = g_auths.size(); + num_auths_remaining = g_auths->size(); + + // If it's the last one, delete the map. + if (num_auths_remaining == 0) { + delete g_auths; + g_auths = nullptr; + } } auth_data_->ClearListeners(); diff --git a/database/src/common/database.cc b/database/src/common/database.cc index 3d262a9b64..37da4438e0 100644 --- a/database/src/common/database.cc +++ b/database/src/common/database.cc @@ -56,7 +56,10 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseDatabase); using internal::DatabaseInternal; using DatabaseMap = std::map, Database*>; -Mutex g_databases_lock; // NOLINT +// Allocated once and never freed so that it is not destroyed by the C++ +// runtime at exit: a Database owned by a static object may be destroyed after +// this file's globals, and its destructor must still be able to lock this. +Mutex* g_databases_lock = new Mutex(); static DatabaseMap* g_databases = nullptr; namespace { @@ -71,7 +74,7 @@ Database* Database::GetInstance(App* app, const char* url, LogError("Database::GetInstance(): The app must not be null."); return nullptr; } - MutexLock lock(g_databases_lock); + MutexLock lock(*g_databases_lock); if (!g_databases) { g_databases = new DatabaseMap(); } @@ -118,7 +121,7 @@ Database::Database(::firebase::App* app, internal::DatabaseInternal* internal) } void Database::DeleteInternal() { - MutexLock lock(g_databases_lock); + MutexLock lock(*g_databases_lock); if (!internal_) return; diff --git a/functions/src/common/functions.cc b/functions/src/common/functions.cc index ea278b3053..1a353e9751 100644 --- a/functions/src/common/functions.cc +++ b/functions/src/common/functions.cc @@ -54,7 +54,10 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseFunctions); constexpr const char* kDefaultRegion = "us-central1"; -Mutex g_functions_lock; // NOLINT +// Allocated once and never freed so that it is not destroyed by the C++ +// runtime at exit: a Functions owned by a static object may be destroyed after +// this file's globals, and its destructor must still be able to lock this. +Mutex* g_functions_lock = new Mutex(); static std::map, Functions*>* g_functions = nullptr; @@ -65,7 +68,7 @@ Functions* Functions::GetInstance(::firebase::App* app, Functions* Functions::GetInstance(::firebase::App* app, const char* region, InitResult* init_result_out) { - MutexLock lock(g_functions_lock); + MutexLock lock(*g_functions_lock); if (!g_functions) { g_functions = new std::map, Functions*>(); } @@ -118,7 +121,7 @@ Functions::Functions(::firebase::App* app, const char* region) { Functions::~Functions() { DeleteInternal(); } void Functions::DeleteInternal() { - MutexLock lock(g_functions_lock); + MutexLock lock(*g_functions_lock); if (!internal_) return; diff --git a/installations/src/installations.cc b/installations/src/installations.cc index 792e7a846f..0a454e9bf0 100644 --- a/installations/src/installations.cc +++ b/installations/src/installations.cc @@ -24,12 +24,16 @@ namespace firebase { namespace installations { std::map* g_installations = nullptr; // NOLINT -Mutex g_installations_mutex; // NOLINT +// Allocated once and never freed so that it is not destroyed by the C++ +// runtime at exit: an Installations owned by a static object may be destroyed +// after this file's globals, and its destructor must still be able to lock +// this. +Mutex* g_installations_mutex = new Mutex(); using internal::InstallationsInternal; Installations* Installations::GetInstance(App* app) { - MutexLock lock(g_installations_mutex); + MutexLock lock(*g_installations_mutex); // Return the Installations if it already exists. Installations* existing_installations = FindInstallations(app); @@ -70,7 +74,7 @@ Installations* Installations::GetInstance(App* app) { } Installations* Installations::FindInstallations(App* app) { - MutexLock lock(g_installations_mutex); + MutexLock lock(*g_installations_mutex); if (!g_installations) { g_installations = new std::map(); // NOLINT return nullptr; @@ -86,12 +90,12 @@ Installations* Installations::FindInstallations(App* app) { } Installations::Installations(App* app) : app_(app) { - MutexLock lock(g_installations_mutex); + MutexLock lock(*g_installations_mutex); installations_internal_ = new InstallationsInternal(*app); } Installations::~Installations() { - MutexLock lock(g_installations_mutex); + MutexLock lock(*g_installations_mutex); CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app_); if (notifier) { @@ -116,7 +120,7 @@ bool Installations::InitInternal() { } void Installations::DeleteInternal() { - MutexLock lock(g_installations_mutex); + MutexLock lock(*g_installations_mutex); if (!installations_internal_) return; installations_internal_->Cleanup(); diff --git a/release_build_files/readme.md b/release_build_files/readme.md index e1b9847c6e..b4c46ef6e6 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -613,6 +613,14 @@ workflow use only during the development of your app, not for publicly shipping code. ## Release Notes +### Upcoming +- Changes + - General: Fixed a crash at process exit when an `Auth`, `RemoteConfig`, + `Database`, `Storage`, `Functions`, `Installations` or `AppCheck` + instance is owned by a static object (for example a static + `std::unique_ptr`) and destroyed after the SDK's own globals. + ([#110](https://github.com/firebase/firebase-cpp-sdk/issues/110)) + ### 14.1.0 - Changes - General (Android): Update to Firebase Android BoM version 35.1.0. diff --git a/remote_config/src/remote_config.cc b/remote_config/src/remote_config.cc index 0c6b0a3c27..441efc6554 100644 --- a/remote_config/src/remote_config.cc +++ b/remote_config/src/remote_config.cc @@ -39,11 +39,21 @@ namespace firebase { namespace remote_config { -std::map g_rcs; -Mutex g_rc_mutex; // NOLINT +// The global map of Apps to RemoteConfigs. +// +// The map is allocated on first use and freed when the last RemoteConfig is +// deleted. The mutex is allocated once and never freed: it cannot be safely +// deleted while another thread may be about to lock it, and it is cheap to +// keep for the lifetime of the process. Neither is a static object with a +// destructor, so a RemoteConfig owned by a static (e.g. a static +// std::unique_ptr) can still be destroyed safely after this +// file's globals would otherwise have been torn down by the C++ runtime. +using RemoteConfigMap = std::map; +static RemoteConfigMap* g_rcs = nullptr; +Mutex* g_rc_mutex = new Mutex(); RemoteConfig* RemoteConfig::GetInstance(App* app) { - MutexLock lock(g_rc_mutex); + MutexLock lock(*g_rc_mutex); // Return the RemoteConfig if it already exists. RemoteConfig* existing_rc = FindRemoteConfig(app); if (existing_rc) { @@ -70,7 +80,10 @@ RemoteConfig* RemoteConfig::GetInstance(App* app) { // Stick it in the global map so we remember it, and can delete it on // shutdown. - g_rcs[app] = rc; + if (!g_rcs) { + g_rcs = new RemoteConfigMap(); + } + (*g_rcs)[app] = rc; return rc; } @@ -78,17 +91,18 @@ RemoteConfig* RemoteConfig::GetInstance(App* app) { } RemoteConfig* RemoteConfig::FindRemoteConfig(App* app) { - MutexLock lock(g_rc_mutex); + MutexLock lock(*g_rc_mutex); + if (!g_rcs) return nullptr; // Return the RemoteConfig if it already exists. - std::map::iterator it = g_rcs.find(app); - if (it != g_rcs.end()) { + RemoteConfigMap::iterator it = g_rcs->find(app); + if (it != g_rcs->end()) { return it->second; } return nullptr; } void RemoteConfig::DeleteInternal() { - MutexLock lock(g_rc_mutex); + MutexLock lock(*g_rc_mutex); if (!internal_) return; CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app()); @@ -100,7 +114,14 @@ void RemoteConfig::DeleteInternal() { internal_ = nullptr; // Remove rc from the g_rcs map. - g_rcs.erase(app()); + if (g_rcs) { + g_rcs->erase(app()); + // If it's the last one, delete the map. + if (g_rcs->empty()) { + delete g_rcs; + g_rcs = nullptr; + } + } } RemoteConfig::RemoteConfig(App* app) { diff --git a/storage/src/common/storage.cc b/storage/src/common/storage.cc index 33786b4dda..0842c86c9a 100644 --- a/storage/src/common/storage.cc +++ b/storage/src/common/storage.cc @@ -52,7 +52,10 @@ namespace storage { DEFINE_FIREBASE_VERSION_STRING(FirebaseStorage); -Mutex g_storages_lock; // NOLINT +// Allocated once and never freed so that it is not destroyed by the C++ +// runtime at exit: a Storage owned by a static object may be destroyed after +// this file's globals, and its destructor must still be able to lock this. +Mutex* g_storages_lock = new Mutex(); static std::map, Storage*>* g_storages = nullptr; Storage* Storage::GetInstance(::firebase::App* app, @@ -62,7 +65,7 @@ Storage* Storage::GetInstance(::firebase::App* app, Storage* Storage::GetInstance(::firebase::App* app, const char* url, InitResult* init_result_out) { - MutexLock lock(g_storages_lock); + MutexLock lock(*g_storages_lock); if (!g_storages) { g_storages = new std::map, Storage*>(); } @@ -133,7 +136,7 @@ Storage::Storage(::firebase::App* app, const char* url) { } void Storage::DeleteInternal() { - MutexLock lock(g_storages_lock); + MutexLock lock(*g_storages_lock); if (!internal_) return; CleanupNotifier* app_notifier = CleanupNotifier::FindByOwner(app()); From 61dcafb145f80e07426293376671feabf53630f0 Mon Sep 17 00:00:00 2001 From: austinbenoit Date: Mon, 26 Oct 2026 15:46:14 +0000 Subject: [PATCH 2/5] Use function-local statics for the instance registries Replace the namespace-scope `Mutex* g_x = new Mutex();` globals with accessor functions that own a function-local static. Dynamic initialization of a namespace-scope pointer is subject to static initialization order, so a static initializer in another translation unit that called GetInstance() would have found a null mutex. A function-local static is initialized on first use, thread-safely, regardless of translation unit order, and the pointer it holds is still never freed. Auth and Remote Config now also keep their map behind the same kind of accessor instead of lazily allocating and freeing a raw pointer, which removes the null checks and manual lifetime management from the GetInstance/Find/DeleteInternal paths. Callers only ever see references. --- app_check/src/common/app_check.cc | 20 ++++++---- auth/src/auth.cc | 60 +++++++++++++++--------------- database/src/common/database.cc | 18 ++++++--- functions/src/common/functions.cc | 18 ++++++--- installations/src/installations.cc | 25 ++++++++----- remote_config/src/remote_config.cc | 52 +++++++++++++------------- storage/src/common/storage.cc | 18 ++++++--- 7 files changed, 119 insertions(+), 92 deletions(-) diff --git a/app_check/src/common/app_check.cc b/app_check/src/common/app_check.cc index 538785193c..a78c6341b5 100644 --- a/app_check/src/common/app_check.cc +++ b/app_check/src/common/app_check.cc @@ -61,10 +61,16 @@ namespace app_check { DEFINE_FIREBASE_VERSION_STRING(FirebaseAppCheck); -// Allocated once and never freed so that it is not destroyed by the C++ -// runtime at exit: an AppCheck owned by a static object may be destroyed after -// this file's globals, and its destructor must still be able to lock this. -Mutex* g_app_check_lock = new Mutex(); +// A function-local static that is allocated on first use and never freed, +// which keeps it out of static initialization and destruction entirely: it +// is ready no matter which translation unit's initializers run first, and +// an AppCheck owned by a static object can still lock it from its destructor +// after this file's globals would otherwise have been torn down by the C++ +// runtime. +static Mutex& AppCheckLock() { + static Mutex* const mutex = new Mutex(); + return *mutex; +} static std::map<::firebase::App*, AppCheck*>* g_app_check_map = nullptr; // Define the destructors for the virtual listener/provider/factory classes. @@ -89,7 +95,7 @@ namespace internal { AppCheck* GetExistingAppCheckInstance(::firebase::App* app) { if (!app) return nullptr; - MutexLock lock(*g_app_check_lock); + MutexLock lock(AppCheckLock()); if (g_app_check_map) { auto it = g_app_check_map->find(app); if (it != g_app_check_map->end()) { @@ -104,7 +110,7 @@ AppCheck* GetExistingAppCheckInstance(::firebase::App* app) { AppCheck* AppCheck::GetInstance(::firebase::App* app) { if (!app) return nullptr; - MutexLock lock(*g_app_check_lock); + MutexLock lock(AppCheckLock()); if (!g_app_check_map) { g_app_check_map = new std::map<::firebase::App*, AppCheck*>(); } @@ -130,7 +136,7 @@ AppCheck::AppCheck(::firebase::App* app) { AppCheck::~AppCheck() { DeleteInternal(); } void AppCheck::DeleteInternal() { - MutexLock lock(*g_app_check_lock); + MutexLock lock(AppCheckLock()); if (!internal_) return; diff --git a/auth/src/auth.cc b/auth/src/auth.cc index 90922631dd..ae3f55f4c4 100644 --- a/auth/src/auth.cc +++ b/auth/src/auth.cc @@ -60,20 +60,29 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseAuth); // reference. // TODO(jsanmiya): Ensure all the Auths are destroyed on shutdown. // -// The map is allocated on first use and freed when the last Auth is deleted. -// The mutex is allocated once and never freed: it cannot be safely deleted -// while another thread may be about to lock it, and it is cheap to keep for -// the lifetime of the process. Neither is a static object with a destructor, -// so an Auth owned by a static (e.g. a static std::unique_ptr) can still -// be destroyed safely after this file's globals would otherwise have been torn -// down by the C++ runtime. +// The map and its mutex are function-local statics that are allocated on +// first use and never freed, which keeps them out of static initialization +// and destruction entirely. They are ready no matter which translation unit's +// initializers run first, and an Auth owned by a static (e.g. a static +// std::unique_ptr) can still be destroyed safely after this file's +// globals would otherwise have been torn down by the C++ runtime. The mutex in +// particular can never be freed safely, since another thread may be about to +// lock it. using AuthMap = std::map; -static AuthMap* g_auths = nullptr; -Mutex* g_auths_mutex = new Mutex(); + +static AuthMap& Auths() { + static AuthMap* const auths = new AuthMap(); + return *auths; +} + +static Mutex& AuthsMutex() { + static Mutex* const mutex = new Mutex(); + return *mutex; +} // static Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { - MutexLock lock(*g_auths_mutex); + MutexLock lock(AuthsMutex()); // Return the Auth if it already exists. Auth* existing_auth = FindAuth(app); if (existing_auth) { @@ -94,10 +103,7 @@ Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { // Stick it in the global map so we remember it, and can delete it on // shutdown. - if (!g_auths) { - g_auths = new AuthMap(); - } - (*g_auths)[app] = auth; + Auths()[app] = auth; if (init_result_out) *init_result_out = kInitResultSuccess; return auth; @@ -105,11 +111,10 @@ Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { // static Auth* Auth::FindAuth(App* app) { - MutexLock lock(*g_auths_mutex); - if (!g_auths) return nullptr; + MutexLock lock(AuthsMutex()); // Return the Auth if it already exists. - AuthMap::iterator it = g_auths->find(app); - if (it != g_auths->end()) { + AuthMap::iterator it = Auths().find(app); + if (it != Auths().end()) { return it->second; } return nullptr; @@ -142,7 +147,7 @@ Auth::Auth(App* app, void* auth_impl) : auth_data_(new AuthData) { } void Auth::DeleteInternal() { - MutexLock lock(*g_auths_mutex); + MutexLock lock(AuthsMutex()); if (!auth_data_) return; @@ -161,24 +166,19 @@ void Auth::DeleteInternal() { notifier->UnregisterObject(this); int num_auths_remaining = 0; - if (g_auths) { - // Remove `this` from the g_auths map. + { + // Remove `this` from the Auths map. // The mapping is 1:1, so we should only ever delete one. - for (auto it = g_auths->begin(); it != g_auths->end(); ++it) { + AuthMap& auths = Auths(); + for (auto it = auths.begin(); it != auths.end(); ++it) { if (it->second == this) { LogDebug("Deleting Auth %p for App %p", this, it->first); - g_auths->erase(it); + auths.erase(it); break; } } - num_auths_remaining = g_auths->size(); - - // If it's the last one, delete the map. - if (num_auths_remaining == 0) { - delete g_auths; - g_auths = nullptr; - } + num_auths_remaining = auths.size(); } auth_data_->ClearListeners(); diff --git a/database/src/common/database.cc b/database/src/common/database.cc index 37da4438e0..1a302013af 100644 --- a/database/src/common/database.cc +++ b/database/src/common/database.cc @@ -56,10 +56,16 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseDatabase); using internal::DatabaseInternal; using DatabaseMap = std::map, Database*>; -// Allocated once and never freed so that it is not destroyed by the C++ -// runtime at exit: a Database owned by a static object may be destroyed after -// this file's globals, and its destructor must still be able to lock this. -Mutex* g_databases_lock = new Mutex(); +// A function-local static that is allocated on first use and never freed, +// which keeps it out of static initialization and destruction entirely: it +// is ready no matter which translation unit's initializers run first, and +// a Database owned by a static object can still lock it from its destructor +// after this file's globals would otherwise have been torn down by the C++ +// runtime. +static Mutex& DatabasesLock() { + static Mutex* const mutex = new Mutex(); + return *mutex; +} static DatabaseMap* g_databases = nullptr; namespace { @@ -74,7 +80,7 @@ Database* Database::GetInstance(App* app, const char* url, LogError("Database::GetInstance(): The app must not be null."); return nullptr; } - MutexLock lock(*g_databases_lock); + MutexLock lock(DatabasesLock()); if (!g_databases) { g_databases = new DatabaseMap(); } @@ -121,7 +127,7 @@ Database::Database(::firebase::App* app, internal::DatabaseInternal* internal) } void Database::DeleteInternal() { - MutexLock lock(*g_databases_lock); + MutexLock lock(DatabasesLock()); if (!internal_) return; diff --git a/functions/src/common/functions.cc b/functions/src/common/functions.cc index 1a353e9751..c31021f623 100644 --- a/functions/src/common/functions.cc +++ b/functions/src/common/functions.cc @@ -54,10 +54,16 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseFunctions); constexpr const char* kDefaultRegion = "us-central1"; -// Allocated once and never freed so that it is not destroyed by the C++ -// runtime at exit: a Functions owned by a static object may be destroyed after -// this file's globals, and its destructor must still be able to lock this. -Mutex* g_functions_lock = new Mutex(); +// A function-local static that is allocated on first use and never freed, +// which keeps it out of static initialization and destruction entirely: it +// is ready no matter which translation unit's initializers run first, and +// a Functions owned by a static object can still lock it from its destructor +// after this file's globals would otherwise have been torn down by the C++ +// runtime. +static Mutex& FunctionsLock() { + static Mutex* const mutex = new Mutex(); + return *mutex; +} static std::map, Functions*>* g_functions = nullptr; @@ -68,7 +74,7 @@ Functions* Functions::GetInstance(::firebase::App* app, Functions* Functions::GetInstance(::firebase::App* app, const char* region, InitResult* init_result_out) { - MutexLock lock(*g_functions_lock); + MutexLock lock(FunctionsLock()); if (!g_functions) { g_functions = new std::map, Functions*>(); } @@ -121,7 +127,7 @@ Functions::Functions(::firebase::App* app, const char* region) { Functions::~Functions() { DeleteInternal(); } void Functions::DeleteInternal() { - MutexLock lock(*g_functions_lock); + MutexLock lock(FunctionsLock()); if (!internal_) return; diff --git a/installations/src/installations.cc b/installations/src/installations.cc index 0a454e9bf0..c2bbf61cdf 100644 --- a/installations/src/installations.cc +++ b/installations/src/installations.cc @@ -24,16 +24,21 @@ namespace firebase { namespace installations { std::map* g_installations = nullptr; // NOLINT -// Allocated once and never freed so that it is not destroyed by the C++ -// runtime at exit: an Installations owned by a static object may be destroyed -// after this file's globals, and its destructor must still be able to lock -// this. -Mutex* g_installations_mutex = new Mutex(); +// A function-local static that is allocated on first use and never freed, +// which keeps it out of static initialization and destruction entirely: it +// is ready no matter which translation unit's initializers run first, and +// an Installations owned by a static object can still lock it from its +// destructor after this file's globals would otherwise have been torn down by +// the C++ runtime. +static Mutex& InstallationsMutex() { + static Mutex* const mutex = new Mutex(); + return *mutex; +} using internal::InstallationsInternal; Installations* Installations::GetInstance(App* app) { - MutexLock lock(*g_installations_mutex); + MutexLock lock(InstallationsMutex()); // Return the Installations if it already exists. Installations* existing_installations = FindInstallations(app); @@ -74,7 +79,7 @@ Installations* Installations::GetInstance(App* app) { } Installations* Installations::FindInstallations(App* app) { - MutexLock lock(*g_installations_mutex); + MutexLock lock(InstallationsMutex()); if (!g_installations) { g_installations = new std::map(); // NOLINT return nullptr; @@ -90,12 +95,12 @@ Installations* Installations::FindInstallations(App* app) { } Installations::Installations(App* app) : app_(app) { - MutexLock lock(*g_installations_mutex); + MutexLock lock(InstallationsMutex()); installations_internal_ = new InstallationsInternal(*app); } Installations::~Installations() { - MutexLock lock(*g_installations_mutex); + MutexLock lock(InstallationsMutex()); CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app_); if (notifier) { @@ -120,7 +125,7 @@ bool Installations::InitInternal() { } void Installations::DeleteInternal() { - MutexLock lock(*g_installations_mutex); + MutexLock lock(InstallationsMutex()); if (!installations_internal_) return; installations_internal_->Cleanup(); diff --git a/remote_config/src/remote_config.cc b/remote_config/src/remote_config.cc index 441efc6554..5f6bf8415b 100644 --- a/remote_config/src/remote_config.cc +++ b/remote_config/src/remote_config.cc @@ -41,19 +41,28 @@ namespace remote_config { // The global map of Apps to RemoteConfigs. // -// The map is allocated on first use and freed when the last RemoteConfig is -// deleted. The mutex is allocated once and never freed: it cannot be safely -// deleted while another thread may be about to lock it, and it is cheap to -// keep for the lifetime of the process. Neither is a static object with a -// destructor, so a RemoteConfig owned by a static (e.g. a static +// The map and its mutex are function-local statics that are allocated on +// first use and never freed, which keeps them out of static initialization +// and destruction entirely. They are ready no matter which translation unit's +// initializers run first, and a RemoteConfig owned by a static (e.g. a static // std::unique_ptr) can still be destroyed safely after this -// file's globals would otherwise have been torn down by the C++ runtime. +// file's globals would otherwise have been torn down by the C++ runtime. The +// mutex in particular can never be freed safely, since another thread may be +// about to lock it. using RemoteConfigMap = std::map; -static RemoteConfigMap* g_rcs = nullptr; -Mutex* g_rc_mutex = new Mutex(); + +static RemoteConfigMap& RemoteConfigs() { + static RemoteConfigMap* const remote_configs = new RemoteConfigMap(); + return *remote_configs; +} + +static Mutex& RemoteConfigsMutex() { + static Mutex* const mutex = new Mutex(); + return *mutex; +} RemoteConfig* RemoteConfig::GetInstance(App* app) { - MutexLock lock(*g_rc_mutex); + MutexLock lock(RemoteConfigsMutex()); // Return the RemoteConfig if it already exists. RemoteConfig* existing_rc = FindRemoteConfig(app); if (existing_rc) { @@ -80,10 +89,7 @@ RemoteConfig* RemoteConfig::GetInstance(App* app) { // Stick it in the global map so we remember it, and can delete it on // shutdown. - if (!g_rcs) { - g_rcs = new RemoteConfigMap(); - } - (*g_rcs)[app] = rc; + RemoteConfigs()[app] = rc; return rc; } @@ -91,18 +97,17 @@ RemoteConfig* RemoteConfig::GetInstance(App* app) { } RemoteConfig* RemoteConfig::FindRemoteConfig(App* app) { - MutexLock lock(*g_rc_mutex); - if (!g_rcs) return nullptr; + MutexLock lock(RemoteConfigsMutex()); // Return the RemoteConfig if it already exists. - RemoteConfigMap::iterator it = g_rcs->find(app); - if (it != g_rcs->end()) { + RemoteConfigMap::iterator it = RemoteConfigs().find(app); + if (it != RemoteConfigs().end()) { return it->second; } return nullptr; } void RemoteConfig::DeleteInternal() { - MutexLock lock(*g_rc_mutex); + MutexLock lock(RemoteConfigsMutex()); if (!internal_) return; CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app()); @@ -113,15 +118,8 @@ void RemoteConfig::DeleteInternal() { delete internal_; internal_ = nullptr; - // Remove rc from the g_rcs map. - if (g_rcs) { - g_rcs->erase(app()); - // If it's the last one, delete the map. - if (g_rcs->empty()) { - delete g_rcs; - g_rcs = nullptr; - } - } + // Remove rc from the RemoteConfigs map. + RemoteConfigs().erase(app()); } RemoteConfig::RemoteConfig(App* app) { diff --git a/storage/src/common/storage.cc b/storage/src/common/storage.cc index 0842c86c9a..abc722281a 100644 --- a/storage/src/common/storage.cc +++ b/storage/src/common/storage.cc @@ -52,10 +52,16 @@ namespace storage { DEFINE_FIREBASE_VERSION_STRING(FirebaseStorage); -// Allocated once and never freed so that it is not destroyed by the C++ -// runtime at exit: a Storage owned by a static object may be destroyed after -// this file's globals, and its destructor must still be able to lock this. -Mutex* g_storages_lock = new Mutex(); +// A function-local static that is allocated on first use and never freed, +// which keeps it out of static initialization and destruction entirely: it +// is ready no matter which translation unit's initializers run first, and +// a Storage owned by a static object can still lock it from its destructor +// after this file's globals would otherwise have been torn down by the C++ +// runtime. +static Mutex& StoragesLock() { + static Mutex* const mutex = new Mutex(); + return *mutex; +} static std::map, Storage*>* g_storages = nullptr; Storage* Storage::GetInstance(::firebase::App* app, @@ -65,7 +71,7 @@ Storage* Storage::GetInstance(::firebase::App* app, Storage* Storage::GetInstance(::firebase::App* app, const char* url, InitResult* init_result_out) { - MutexLock lock(*g_storages_lock); + MutexLock lock(StoragesLock()); if (!g_storages) { g_storages = new std::map, Storage*>(); } @@ -136,7 +142,7 @@ Storage::Storage(::firebase::App* app, const char* url) { } void Storage::DeleteInternal() { - MutexLock lock(*g_storages_lock); + MutexLock lock(StoragesLock()); if (!internal_) return; CleanupNotifier* app_notifier = CleanupNotifier::FindByOwner(app()); From 1d3b1f7a5ba50acf26dc3fe0192116befecd37e0 Mon Sep 17 00:00:00 2001 From: austinbenoit Date: Mon, 26 Oct 2026 15:57:13 +0000 Subject: [PATCH 3/5] Add NoDestructor and use it for the instance registries Introduce firebase::NoDestructor, a small wrapper (modelled on absl::NoDestructor) that constructs a T in place and never runs its destructor. Use it for the registry mutexes of every product and the registry maps of Auth and Remote Config in place of `static T* const t = new T;`. The objects are now stored inline in the function-local static rather than on the heap, there is no `new` or raw pointer anywhere in the registry code, and leak checkers have nothing to report since nothing is allocated. The semantics are otherwise unchanged: constructed on first use, thread-safely, regardless of translation unit order, and never destroyed. Includes a unit test for NoDestructor. --- app/src/no_destructor.h | 71 +++++++++++++++++++++++++++++ app/tests/CMakeLists.txt | 7 +++ app/tests/no_destructor_test.cc | 73 ++++++++++++++++++++++++++++++ app_check/src/common/app_check.cc | 15 +++--- auth/src/auth.cc | 21 +++++---- database/src/common/database.cc | 15 +++--- functions/src/common/functions.cc | 15 +++--- installations/src/installations.cc | 15 +++--- remote_config/src/remote_config.cc | 21 +++++---- storage/src/common/storage.cc | 15 +++--- 10 files changed, 213 insertions(+), 55 deletions(-) create mode 100644 app/src/no_destructor.h create mode 100644 app/tests/no_destructor_test.cc diff --git a/app/src/no_destructor.h b/app/src/no_destructor.h new file mode 100644 index 0000000000..af247796c8 --- /dev/null +++ b/app/src/no_destructor.h @@ -0,0 +1,71 @@ +/* + * Copyright 2026 Google LLC + * + * 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. + */ + +#ifndef FIREBASE_APP_SRC_NO_DESTRUCTOR_H_ +#define FIREBASE_APP_SRC_NO_DESTRUCTOR_H_ + +#include +#include + +namespace firebase { + +// Wraps a T so that its destructor is never run. +// +// Intended for function-local statics that must outlive static destruction, +// such as the registries and mutexes that Firebase instances touch from their +// own destructors. A plain `static T t;` is destroyed at exit in reverse order +// of construction, so an instance owned by a user's static object can find it +// already gone. A `static T* t = new T;` avoids that but hands callers a raw +// pointer and a heap allocation. NoDestructor keeps the T inline, constructs it +// in place on first use, and simply never destroys it. +// +// static Mutex& RegistryMutex() { +// static NoDestructor mutex; +// return *mutex; +// } +// +// The object is reachable from the static for the lifetime of the process, so +// leak checkers do not report it. +template +class NoDestructor { + public: + // Constructs the wrapped T in place, forwarding any arguments. + template + explicit NoDestructor(Args&&... args) { + new (&storage_) T(std::forward(args)...); + } + + NoDestructor(const NoDestructor&) = delete; + NoDestructor& operator=(const NoDestructor&) = delete; + + // Intentionally does not destroy the wrapped T. + ~NoDestructor() = default; + + T& operator*() { return *get(); } + T* operator->() { return get(); } + T* get() { return reinterpret_cast(&storage_); } + + const T& operator*() const { return *get(); } + const T* operator->() const { return get(); } + const T* get() const { return reinterpret_cast(&storage_); } + + private: + alignas(T) unsigned char storage_[sizeof(T)]; +}; + +} // namespace firebase + +#endif // FIREBASE_APP_SRC_NO_DESTRUCTOR_H_ diff --git a/app/tests/CMakeLists.txt b/app/tests/CMakeLists.txt index 65d5949c03..489e8c1da5 100644 --- a/app/tests/CMakeLists.txt +++ b/app/tests/CMakeLists.txt @@ -161,6 +161,13 @@ firebase_cpp_cc_test(firebase_app_cleanup_notifier_test firebase_app ) +firebase_cpp_cc_test(firebase_app_no_destructor_test + SOURCES + no_destructor_test.cc + DEPENDS + firebase_app +) + firebase_cpp_cc_test(firebase_app_cpp11_thread_test SOURCES thread_test.cc diff --git a/app/tests/no_destructor_test.cc b/app/tests/no_destructor_test.cc new file mode 100644 index 0000000000..7b729086e9 --- /dev/null +++ b/app/tests/no_destructor_test.cc @@ -0,0 +1,73 @@ +/* + * Copyright 2026 Google LLC + * + * 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 "app/src/no_destructor.h" + +#include +#include + +#include "gtest/gtest.h" + +namespace firebase { +namespace { + +// Records how many times it has been constructed and destroyed. +struct Tracked { + explicit Tracked(int value_in) : value(value_in) { ++constructed; } + ~Tracked() { ++destroyed; } + + int value; + static int constructed; + static int destroyed; +}; +int Tracked::constructed = 0; +int Tracked::destroyed = 0; + +TEST(NoDestructorTest, ForwardsConstructorArguments) { + NoDestructor str("hello"); + EXPECT_EQ(*str, "hello"); + EXPECT_EQ(str->size(), 5u); + EXPECT_EQ(str.get(), &*str); +} + +TEST(NoDestructorTest, DefaultConstructs) { + NoDestructor> map; + EXPECT_TRUE(map->empty()); + (*map)[1] = 2; + EXPECT_EQ(map->at(1), 2); +} + +TEST(NoDestructorTest, DoesNotRunDestructorWhenGoingOutOfScope) { + Tracked::constructed = 0; + Tracked::destroyed = 0; + { + NoDestructor tracked(42); + EXPECT_EQ(tracked->value, 42); + EXPECT_EQ(Tracked::constructed, 1); + } + EXPECT_EQ(Tracked::constructed, 1); + EXPECT_EQ(Tracked::destroyed, 0); +} + +TEST(NoDestructorTest, ConstAccess) { + const NoDestructor str("const"); + EXPECT_EQ(*str, "const"); + EXPECT_EQ(str->size(), 5u); + EXPECT_EQ(str.get(), &*str); +} + +} // namespace +} // namespace firebase diff --git a/app_check/src/common/app_check.cc b/app_check/src/common/app_check.cc index a78c6341b5..2c4bc06fb1 100644 --- a/app_check/src/common/app_check.cc +++ b/app_check/src/common/app_check.cc @@ -25,6 +25,7 @@ #include "app/src/include/firebase/internal/platform.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "app_check/src/common/common.h" @@ -61,14 +62,14 @@ namespace app_check { DEFINE_FIREBASE_VERSION_STRING(FirebaseAppCheck); -// A function-local static that is allocated on first use and never freed, -// which keeps it out of static initialization and destruction entirely: it -// is ready no matter which translation unit's initializers run first, and -// an AppCheck owned by a static object can still lock it from its destructor -// after this file's globals would otherwise have been torn down by the C++ -// runtime. +// A function-local static wrapped in NoDestructor, so it is constructed on +// first use and never destroyed. That keeps it out of static initialization +// and destruction entirely: it is ready no matter which translation unit's +// initializers run first, and an AppCheck owned by a static object can still +// lock it from its destructor after this file's globals would otherwise have +// been torn down by the C++ runtime. static Mutex& AppCheckLock() { - static Mutex* const mutex = new Mutex(); + static NoDestructor mutex; return *mutex; } static std::map<::firebase::App*, AppCheck*>* g_app_check_map = nullptr; diff --git a/auth/src/auth.cc b/auth/src/auth.cc index ae3f55f4c4..20461c2f11 100644 --- a/auth/src/auth.cc +++ b/auth/src/auth.cc @@ -32,6 +32,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/internal/platform.h" #include "app/src/include/firebase/version.h" +#include "app/src/no_destructor.h" #include "app/src/semaphore.h" #include "app/src/util.h" #include "auth/src/common.h" @@ -60,23 +61,23 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseAuth); // reference. // TODO(jsanmiya): Ensure all the Auths are destroyed on shutdown. // -// The map and its mutex are function-local statics that are allocated on -// first use and never freed, which keeps them out of static initialization -// and destruction entirely. They are ready no matter which translation unit's -// initializers run first, and an Auth owned by a static (e.g. a static -// std::unique_ptr) can still be destroyed safely after this file's -// globals would otherwise have been torn down by the C++ runtime. The mutex in -// particular can never be freed safely, since another thread may be about to -// lock it. +// The map and its mutex are function-local statics wrapped in NoDestructor, +// so they are constructed on first use and never destroyed. That keeps them +// out of static initialization and destruction entirely: they are ready no +// matter which translation unit's initializers run first, and an Auth owned +// by a static (e.g. a static std::unique_ptr) can still be destroyed +// safely after this file's globals would otherwise have been torn down by the +// C++ runtime. The mutex in particular could never be destroyed safely, since +// another thread may be about to lock it. using AuthMap = std::map; static AuthMap& Auths() { - static AuthMap* const auths = new AuthMap(); + static NoDestructor auths; return *auths; } static Mutex& AuthsMutex() { - static Mutex* const mutex = new Mutex(); + static NoDestructor mutex; return *mutex; } diff --git a/database/src/common/database.cc b/database/src/common/database.cc index 1a302013af..7e8bc23b59 100644 --- a/database/src/common/database.cc +++ b/database/src/common/database.cc @@ -26,6 +26,7 @@ #include "app/src/include/firebase/app.h" #include "app/src/include/firebase/internal/platform.h" #include "app/src/include/firebase/version.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" // DatabaseInternal is defined in these 3 files, one implementation for each OS. @@ -56,14 +57,14 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseDatabase); using internal::DatabaseInternal; using DatabaseMap = std::map, Database*>; -// A function-local static that is allocated on first use and never freed, -// which keeps it out of static initialization and destruction entirely: it -// is ready no matter which translation unit's initializers run first, and -// a Database owned by a static object can still lock it from its destructor -// after this file's globals would otherwise have been torn down by the C++ -// runtime. +// A function-local static wrapped in NoDestructor, so it is constructed on +// first use and never destroyed. That keeps it out of static initialization +// and destruction entirely: it is ready no matter which translation unit's +// initializers run first, and a Database owned by a static object can still +// lock it from its destructor after this file's globals would otherwise have +// been torn down by the C++ runtime. static Mutex& DatabasesLock() { - static Mutex* const mutex = new Mutex(); + static NoDestructor mutex; return *mutex; } static DatabaseMap* g_databases = nullptr; diff --git a/functions/src/common/functions.cc b/functions/src/common/functions.cc index c31021f623..eadfe41fb6 100644 --- a/functions/src/common/functions.cc +++ b/functions/src/common/functions.cc @@ -27,6 +27,7 @@ #include "app/src/include/firebase/internal/platform.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" // QueryInternal is defined in these 3 files, one implementation for each OS. @@ -54,14 +55,14 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseFunctions); constexpr const char* kDefaultRegion = "us-central1"; -// A function-local static that is allocated on first use and never freed, -// which keeps it out of static initialization and destruction entirely: it -// is ready no matter which translation unit's initializers run first, and -// a Functions owned by a static object can still lock it from its destructor -// after this file's globals would otherwise have been torn down by the C++ -// runtime. +// A function-local static wrapped in NoDestructor, so it is constructed on +// first use and never destroyed. That keeps it out of static initialization +// and destruction entirely: it is ready no matter which translation unit's +// initializers run first, and a Functions owned by a static object can still +// lock it from its destructor after this file's globals would otherwise have +// been torn down by the C++ runtime. static Mutex& FunctionsLock() { - static Mutex* const mutex = new Mutex(); + static NoDestructor mutex; return *mutex; } static std::map, Functions*>* g_functions = diff --git a/installations/src/installations.cc b/installations/src/installations.cc index c2bbf61cdf..70dc93b0fc 100644 --- a/installations/src/installations.cc +++ b/installations/src/installations.cc @@ -18,20 +18,21 @@ #include "app/src/cleanup_notifier.h" #include "app/src/include/firebase/internal/mutex.h" +#include "app/src/no_destructor.h" #include "installations/src/installations_internal.h" namespace firebase { namespace installations { std::map* g_installations = nullptr; // NOLINT -// A function-local static that is allocated on first use and never freed, -// which keeps it out of static initialization and destruction entirely: it -// is ready no matter which translation unit's initializers run first, and -// an Installations owned by a static object can still lock it from its -// destructor after this file's globals would otherwise have been torn down by -// the C++ runtime. +// A function-local static wrapped in NoDestructor, so it is constructed on +// first use and never destroyed. That keeps it out of static initialization +// and destruction entirely: it is ready no matter which translation unit's +// initializers run first, and an Installations owned by a static object can +// still lock it from its destructor after this file's globals would otherwise +// have been torn down by the C++ runtime. static Mutex& InstallationsMutex() { - static Mutex* const mutex = new Mutex(); + static NoDestructor mutex; return *mutex; } diff --git a/remote_config/src/remote_config.cc b/remote_config/src/remote_config.cc index 5f6bf8415b..f589e3a2d3 100644 --- a/remote_config/src/remote_config.cc +++ b/remote_config/src/remote_config.cc @@ -20,6 +20,7 @@ #include "app/src/cleanup_notifier.h" #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/internal/platform.h" +#include "app/src/no_destructor.h" #include "app/src/semaphore.h" #include "app/src/time.h" #include "include/firebase/remote_config.h" @@ -41,23 +42,23 @@ namespace remote_config { // The global map of Apps to RemoteConfigs. // -// The map and its mutex are function-local statics that are allocated on -// first use and never freed, which keeps them out of static initialization -// and destruction entirely. They are ready no matter which translation unit's -// initializers run first, and a RemoteConfig owned by a static (e.g. a static -// std::unique_ptr) can still be destroyed safely after this -// file's globals would otherwise have been torn down by the C++ runtime. The -// mutex in particular can never be freed safely, since another thread may be -// about to lock it. +// The map and its mutex are function-local statics wrapped in NoDestructor, +// so they are constructed on first use and never destroyed. That keeps them +// out of static initialization and destruction entirely: they are ready no +// matter which translation unit's initializers run first, and a RemoteConfig +// owned by a static (e.g. a static std::unique_ptr) can still be +// destroyed safely after this file's globals would otherwise have been torn +// down by the C++ runtime. The mutex in particular could never be destroyed +// safely, since another thread may be about to lock it. using RemoteConfigMap = std::map; static RemoteConfigMap& RemoteConfigs() { - static RemoteConfigMap* const remote_configs = new RemoteConfigMap(); + static NoDestructor remote_configs; return *remote_configs; } static Mutex& RemoteConfigsMutex() { - static Mutex* const mutex = new Mutex(); + static NoDestructor mutex; return *mutex; } diff --git a/storage/src/common/storage.cc b/storage/src/common/storage.cc index abc722281a..2e68cabe06 100644 --- a/storage/src/common/storage.cc +++ b/storage/src/common/storage.cc @@ -26,6 +26,7 @@ #include "app/src/include/firebase/app.h" #include "app/src/include/firebase/internal/platform.h" #include "app/src/include/firebase/version.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "storage/src/common/storage_uri_parser.h" @@ -52,14 +53,14 @@ namespace storage { DEFINE_FIREBASE_VERSION_STRING(FirebaseStorage); -// A function-local static that is allocated on first use and never freed, -// which keeps it out of static initialization and destruction entirely: it -// is ready no matter which translation unit's initializers run first, and -// a Storage owned by a static object can still lock it from its destructor -// after this file's globals would otherwise have been torn down by the C++ -// runtime. +// A function-local static wrapped in NoDestructor, so it is constructed on +// first use and never destroyed. That keeps it out of static initialization +// and destruction entirely: it is ready no matter which translation unit's +// initializers run first, and a Storage owned by a static object can still +// lock it from its destructor after this file's globals would otherwise have +// been torn down by the C++ runtime. static Mutex& StoragesLock() { - static Mutex* const mutex = new Mutex(); + static NoDestructor mutex; return *mutex; } static std::map, Storage*>* g_storages = nullptr; From 67a4892d72e3726ca593ffc16813abc63e921875 Mon Sep 17 00:00:00 2001 From: austinbenoit Date: Thu, 29 Oct 2026 16:36:26 +0000 Subject: [PATCH 4/5] Address review feedback on NoDestructor casts and nested locking - In NoDestructor, cast the decayed array pointer `storage_` rather than `&storage_` in placement new and `get()`. - In Auth, RemoteConfig, and Installations, factor out `Find*Locked(App*)` helpers (and inline the internal cleanup in `~Installations()`) so callers that already hold the registry mutex do not recursively re-lock it. --- app/src/no_destructor.h | 6 ++--- auth/src/auth.cc | 18 +++++++++------ installations/src/installations.cc | 37 +++++++++++++++++++----------- remote_config/src/remote_config.cc | 18 +++++++++------ 4 files changed, 48 insertions(+), 31 deletions(-) diff --git a/app/src/no_destructor.h b/app/src/no_destructor.h index af247796c8..14441f3960 100644 --- a/app/src/no_destructor.h +++ b/app/src/no_destructor.h @@ -45,7 +45,7 @@ class NoDestructor { // Constructs the wrapped T in place, forwarding any arguments. template explicit NoDestructor(Args&&... args) { - new (&storage_) T(std::forward(args)...); + new (static_cast(storage_)) T(std::forward(args)...); } NoDestructor(const NoDestructor&) = delete; @@ -56,11 +56,11 @@ class NoDestructor { T& operator*() { return *get(); } T* operator->() { return get(); } - T* get() { return reinterpret_cast(&storage_); } + T* get() { return reinterpret_cast(storage_); } const T& operator*() const { return *get(); } const T* operator->() const { return get(); } - const T* get() const { return reinterpret_cast(&storage_); } + const T* get() const { return reinterpret_cast(storage_); } private: alignas(T) unsigned char storage_[sizeof(T)]; diff --git a/auth/src/auth.cc b/auth/src/auth.cc index 20461c2f11..10e82f5718 100644 --- a/auth/src/auth.cc +++ b/auth/src/auth.cc @@ -81,11 +81,20 @@ static Mutex& AuthsMutex() { return *mutex; } +// Must be called with AuthsMutex() held. +static Auth* FindAuthLocked(App* app) { + AuthMap::iterator it = Auths().find(app); + if (it != Auths().end()) { + return it->second; + } + return nullptr; +} + // static Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { MutexLock lock(AuthsMutex()); // Return the Auth if it already exists. - Auth* existing_auth = FindAuth(app); + Auth* existing_auth = FindAuthLocked(app); if (existing_auth) { if (init_result_out != nullptr) *init_result_out = kInitResultSuccess; // Log heartbeat data when instance getters are called. @@ -113,12 +122,7 @@ Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { // static Auth* Auth::FindAuth(App* app) { MutexLock lock(AuthsMutex()); - // Return the Auth if it already exists. - AuthMap::iterator it = Auths().find(app); - if (it != Auths().end()) { - return it->second; - } - return nullptr; + return FindAuthLocked(app); } // Auth uses the pimpl mechanism to hide internal data types from the interface. diff --git a/installations/src/installations.cc b/installations/src/installations.cc index 70dc93b0fc..a1a170f8ce 100644 --- a/installations/src/installations.cc +++ b/installations/src/installations.cc @@ -38,11 +38,27 @@ static Mutex& InstallationsMutex() { using internal::InstallationsInternal; +// Must be called with InstallationsMutex() held. +static Installations* FindInstallationsLocked(App* app) { + if (!g_installations) { + g_installations = new std::map(); // NOLINT + return nullptr; + } + + // Return the Installations if it already exists. + std::map::iterator it = + g_installations->find(app); // NOLINT + if (it != g_installations->end()) { + return it->second; + } + return nullptr; +} + Installations* Installations::GetInstance(App* app) { MutexLock lock(InstallationsMutex()); // Return the Installations if it already exists. - Installations* existing_installations = FindInstallations(app); + Installations* existing_installations = FindInstallationsLocked(app); if (existing_installations) { // Log heartbeat data when instance getters are called. // See go/firebase-platform-logging-design for more information. @@ -81,18 +97,7 @@ Installations* Installations::GetInstance(App* app) { Installations* Installations::FindInstallations(App* app) { MutexLock lock(InstallationsMutex()); - if (!g_installations) { - g_installations = new std::map(); // NOLINT - return nullptr; - } - - // Return the Installations if it already exists. - std::map::iterator it = - g_installations->find(app); // NOLINT - if (it != g_installations->end()) { - return it->second; - } - return nullptr; + return FindInstallationsLocked(app); } Installations::Installations(App* app) : app_(app) { @@ -108,7 +113,11 @@ Installations::~Installations() { notifier->UnregisterObject(this); } - DeleteInternal(); + if (installations_internal_) { + installations_internal_->Cleanup(); + delete installations_internal_; + installations_internal_ = nullptr; + } if (g_installations) { // Remove installations from the g_installations map. diff --git a/remote_config/src/remote_config.cc b/remote_config/src/remote_config.cc index f589e3a2d3..0e9805905a 100644 --- a/remote_config/src/remote_config.cc +++ b/remote_config/src/remote_config.cc @@ -62,10 +62,19 @@ static Mutex& RemoteConfigsMutex() { return *mutex; } +// Must be called with RemoteConfigsMutex() held. +static RemoteConfig* FindRemoteConfigLocked(App* app) { + RemoteConfigMap::iterator it = RemoteConfigs().find(app); + if (it != RemoteConfigs().end()) { + return it->second; + } + return nullptr; +} + RemoteConfig* RemoteConfig::GetInstance(App* app) { MutexLock lock(RemoteConfigsMutex()); // Return the RemoteConfig if it already exists. - RemoteConfig* existing_rc = FindRemoteConfig(app); + RemoteConfig* existing_rc = FindRemoteConfigLocked(app); if (existing_rc) { return existing_rc; } @@ -99,12 +108,7 @@ RemoteConfig* RemoteConfig::GetInstance(App* app) { RemoteConfig* RemoteConfig::FindRemoteConfig(App* app) { MutexLock lock(RemoteConfigsMutex()); - // Return the RemoteConfig if it already exists. - RemoteConfigMap::iterator it = RemoteConfigs().find(app); - if (it != RemoteConfigs().end()) { - return it->second; - } - return nullptr; + return FindRemoteConfigLocked(app); } void RemoteConfig::DeleteInternal() { From f0bad88b547a0a0cb7e30bf20d8035bb6ec8a99f Mon Sep 17 00:00:00 2001 From: austinbenoit Date: Thu, 29 Oct 2026 23:54:10 +0000 Subject: [PATCH 5/5] Eliminate exit-time destructors and SIOF hazards across App and SDK modules - Harden NoDestructor with SFINAE forwarding-ctor guard, brace-init overloads, and std::launder access - Migrate firebase_app, app/rest, and all product libraries (Analytics, AppCheck, Auth, Database, Firestore, Functions, Installations, Messaging, RemoteConfig, Storage, UMP) across Desktop, Android, and iOS onto function-local NoDestructor accessors - Fix Installations out-of-order App deletion map retention, ABA safety, failed-init cleanup in Installations/RemoteConfig, and O(log N) Auth map removal - Enable -Werror=exit-time-destructors on Clang/AppleClang across all 12 static library targets - Add firebase_app_static_destruction_test, expanded NoDestructor unit tests, and Installations out-of-order teardown regression test --- analytics/CMakeLists.txt | 8 + analytics/src/analytics_desktop.cc | 42 ++- analytics/src/analytics_ios.mm | 30 +- app/CMakeLists.txt | 13 + app/rest/CMakeLists.txt | 6 + app/rest/transport_curl.cc | 12 +- app/rest/util.cc | 15 +- app/src/app_common.cc | 141 +++++----- app/src/app_desktop.cc | 25 +- app/src/app_desktop.h | 5 - app/src/app_ios.mm | 52 ++-- app/src/callback.cc | 25 +- app/src/cleanup_notifier.cc | 88 +++--- app/src/cleanup_notifier.h | 39 ++- app/src/log.cc | 38 ++- app/src/log_stdio.cc | 14 +- app/src/no_destructor.h | 59 ++-- app/src/reference_counted_future_impl.cc | 14 +- app/src/reference_counted_future_impl.h | 19 +- app/src/secure/user_secure_manager.cc | 16 +- app/src/secure/user_secure_manager.h | 2 - app/src/util.cc | 113 ++++---- app/src/util.h | 26 +- app/tests/CMakeLists.txt | 8 + app/tests/future_test.cc | 4 +- app/tests/no_destructor_test.cc | 63 +++++ app/tests/static_destruction_test.cc | 263 ++++++++++++++++++ app_check/CMakeLists.txt | 6 + .../play_integrity_provider_android.cc | 8 +- app_check/src/common/app_check.cc | 49 ++-- app_check/src/common/debug_provider_common.cc | 8 +- .../src/desktop/debug_provider_desktop.cc | 5 +- app_check/src/ios/app_attest_provider_ios.mm | 7 +- .../src/ios/device_check_provider_ios.mm | 8 +- auth/CMakeLists.txt | 6 + auth/src/auth.cc | 13 +- .../desktop/rpcs/get_account_info_response.h | 6 +- database/CMakeLists.txt | 6 + database/src/android/database_android.cc | 12 +- database/src/android/database_android.h | 3 +- database/src/common/common.cc | 16 +- database/src/common/database.cc | 47 ++-- database/src/common/database_reference.cc | 10 +- database/src/desktop/core/operation.cc | 18 +- database/src/desktop/core/operation.h | 4 +- database/src/desktop/core/repo.cc | 10 +- database/src/desktop/core/sync_tree.cc | 8 +- database/src/desktop/core/write_tree.cc | 2 +- database/src/desktop/database_desktop.cc | 2 - database/src/desktop/database_desktop.h | 2 +- .../src/desktop/persistence/prune_forest.cc | 20 +- .../src/desktop/query_params_comparator.cc | 43 ++- .../src/desktop/query_params_comparator.h | 4 +- database/src/desktop/util_desktop.cc | 20 +- database/src/desktop/util_desktop.h | 4 +- database/src/desktop/view/limited_filter.cc | 5 +- database/src/desktop/view/ranged_filter.cc | 3 +- database/src/desktop/view/view_processor.cc | 2 +- database/src/ios/database_ios.h | 2 +- database/src/ios/database_ios.mm | 2 - database/tests/desktop/core/operation_test.cc | 29 +- .../tests/desktop/view/view_processor_test.cc | 43 ++- firestore/CMakeLists.txt | 6 + firestore/src/android/firestore_android.cc | 16 +- firestore/src/common/firestore.cc | 53 ++-- firestore/src/common/util.cc | 6 +- functions/CMakeLists.txt | 6 + functions/src/android/functions_android.cc | 10 +- functions/src/android/functions_android.h | 2 - functions/src/common/functions.cc | 40 ++- installations/CMakeLists.txt | 6 + .../src/android/installations_android.cc | 12 +- .../src/android/installations_android.h | 2 - installations/src/installations.cc | 101 +++---- installations/tests/installations_test.cc | 30 ++ messaging/CMakeLists.txt | 6 + messaging/src/android/cpp/messaging.cc | 29 +- messaging/src/common.cc | 18 +- messaging/src/ios/messaging.mm | 41 +-- messaging/src/stub/messaging.cc | 12 +- release_build_files/readme.md | 7 +- remote_config/CMakeLists.txt | 6 + .../src/android/remote_config_android.cc | 12 +- .../src/android/remote_config_android.h | 1 - remote_config/src/remote_config.cc | 12 +- storage/CMakeLists.txt | 6 + storage/src/android/storage_android.cc | 10 +- storage/src/android/storage_android.h | 1 - storage/src/common/list_result.cc | 13 +- storage/src/common/storage.cc | 40 ++- ump/CMakeLists.txt | 6 + .../android/consent_info_internal_android.cc | 21 +- .../android/consent_info_internal_android.h | 3 +- ump/src/ios/consent_info_internal_ios.h | 1 - ump/src/ios/consent_info_internal_ios.mm | 41 +-- 95 files changed, 1397 insertions(+), 752 deletions(-) create mode 100644 app/tests/static_destruction_test.cc diff --git a/analytics/CMakeLists.txt b/analytics/CMakeLists.txt index f45af19e64..91bb6c7ce6 100644 --- a/analytics/CMakeLists.txt +++ b/analytics/CMakeLists.txt @@ -130,6 +130,14 @@ target_compile_definitions(firebase_analytics -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +# Restricted to CXX so C (`analytics_desktop_dynamic.c`) and Swift sources do +# not receive a C++-only warning flag. +target_compile_options( + firebase_analytics PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) diff --git a/analytics/src/analytics_desktop.cc b/analytics/src/analytics_desktop.cc index 6fc3625173..492a633c85 100644 --- a/analytics/src/analytics_desktop.cc +++ b/analytics/src/analytics_desktop.cc @@ -28,6 +28,7 @@ #include "app/src/include/firebase/future.h" #include "app/src/include/firebase/variant.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "firebase/log.h" #if defined(_WIN32) @@ -48,12 +49,29 @@ static HMODULE g_analytics_module = 0; // Future data for analytics. // This is initialized in `Initialize()` and cleaned up in `Terminate()`. static bool g_initialized = false; -static LogCallback g_log_callback; -static std::mutex g_log_callback_mutex; + +static LogCallback& GetLogCallback() { + static NoDestructor log_callback; + return *log_callback; +} + +static std::mutex& LogCallbackMutex() { + static NoDestructor log_callback_mutex; + return *log_callback_mutex; +} + static int g_fake_instance_id = 0; static bool g_analytics_collection_enabled = true; -static std::string g_app_id; -static std::string g_package_name; + +static std::string& AppId() { + static NoDestructor app_id; + return *app_id; +} + +static std::string& PackageName() { + static NoDestructor package_name; + return *package_name; +} // Initializes the Analytics desktop API. // This function must be called before any other Analytics methods. @@ -91,15 +109,15 @@ void Initialize(const App& app) { LogInfo("Analytics: Loaded Google Analytics module."); // Initialize Google Analytics C API - g_app_id = app.options().app_id(); - g_package_name = app.options().package_name(); + AppId() = app.options().app_id(); + PackageName() = app.options().package_name(); GoogleAnalytics_Options* c_options = GoogleAnalytics_Options_Create(); if (!c_options) { LogError("Analytics: Failed to create GoogleAnalytics_Options."); } else { - c_options->app_id = g_app_id.c_str(); - c_options->package_name = g_package_name.c_str(); + c_options->app_id = AppId().c_str(); + c_options->package_name = PackageName().c_str(); c_options->analytics_collection_enabled_at_first_launch = g_analytics_collection_enabled; @@ -416,8 +434,8 @@ void GoogleAnalyticsWrapperLogCallback(int32_t log_level, const char* message) { LogCallback callback_to_call; { - std::lock_guard lock(g_log_callback_mutex); - callback_to_call = g_log_callback; + std::lock_guard lock(LogCallbackMutex()); + callback_to_call = GetLogCallback(); } if (callback_to_call) { @@ -433,8 +451,8 @@ void SetLogCallback(const LogCallback& callback) { FIREBASE_ASSERT_RETURN_VOID(internal::IsInitialized()); // The C API does not support user data, so we must use a global variable. { - std::lock_guard lock(g_log_callback_mutex); - g_log_callback = callback; + std::lock_guard lock(LogCallbackMutex()); + GetLogCallback() = callback; } if (callback) { GoogleAnalytics_SetLogCallback(GoogleAnalyticsWrapperLogCallback); diff --git a/analytics/src/analytics_ios.mm b/analytics/src/analytics_ios.mm index e064faa665..9c0fe568c4 100644 --- a/analytics/src/analytics_ios.mm +++ b/analytics/src/analytics_ios.mm @@ -32,6 +32,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/thread.h" #include "app/src/time.h" #include "app/src/util.h" @@ -120,13 +121,16 @@ uint64_t GetResetTimeElapsedMilliseconds() const { const uint64_t AnalyticsDataResetter::kResetTimeoutMilliseconds = 5000; static const double kMillisecondsPerSecond = 1000.0; -static Mutex g_mutex; // NOLINT +static Mutex& AnalyticsMutex() { + static NoDestructor mutex; + return *mutex; +} static bool g_initialized = false; static AnalyticsDataResetter* g_resetter = nullptr; // Initialize the API. void Initialize(const ::firebase::App& app) { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); g_initialized = true; g_resetter = new AnalyticsDataResetter(); internal::RegisterTerminateOnDefaultAppDestroy(); @@ -142,7 +146,7 @@ void Initialize(const ::firebase::App& app) { // Terminate the API. void Terminate() { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); internal::FutureData::Destroy(); internal::UnregisterTerminateOnDefaultAppDestroy(); delete g_resetter; @@ -237,7 +241,7 @@ void LogEvent(const char* name) { } Future LogAppleTransaction(const char* transaction_id) { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); FIREBASE_ASSERT_RETURN(Future(), internal::IsInitialized()); auto* api = internal::FutureData::Get()->api(); @@ -251,7 +255,7 @@ void LogEvent(const char* name) { [AppleTransactionVerifier verifyWithTransactionId:SafeString(transaction_id) completion:^(BOOL isFound) { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); if (!internal::IsInitialized()) return; auto* api = internal::FutureData::Get()->api(); @@ -266,7 +270,7 @@ void LogEvent(const char* name) { } Future LogAppleTransactionLastResult() { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); FIREBASE_ASSERT_RETURN(Future(), internal::IsInitialized()); return static_cast&>( internal::FutureData::Get()->api()->LastResult(internal::kAnalyticsFnLogAppleTransaction)); @@ -449,7 +453,7 @@ void SetSessionTimeoutDuration(int64_t milliseconds) { } void ResetAnalyticsData() { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); FIREBASE_ASSERT_RETURN_VOID(internal::IsInitialized()); g_resetter->Reset(); } @@ -467,7 +471,7 @@ void NotifyAppLifecycleChange(AppLifecycleState) {} void SetDesktopDebugMode(bool) {} Future GetAnalyticsInstanceId() { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); FIREBASE_ASSERT_RETURN(Future(), internal::IsInitialized()); auto* api = internal::FutureData::Get()->api(); const auto future_handle = @@ -477,7 +481,7 @@ Thread get_id_thread( [](SafeFutureHandle* handle) { for (;;) { { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); if (!internal::IsInitialized()) break; std::string instance_id = g_resetter->GetInstanceId(); if (!instance_id.empty()) { @@ -495,19 +499,19 @@ Thread get_id_thread( } Future GetAnalyticsInstanceIdLastResult() { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); FIREBASE_ASSERT_RETURN(Future(), internal::IsInitialized()); return static_cast&>( internal::FutureData::Get()->api()->LastResult(internal::kAnalyticsFnGetAnalyticsInstanceId)); } Future GetSessionId() { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); FIREBASE_ASSERT_RETURN(Future(), internal::IsInitialized()); auto* api = internal::FutureData::Get()->api(); const auto future_handle = api->SafeAlloc(internal::kAnalyticsFnGetSessionId); [FIRAnalytics sessionIDWithCompletion:^(int64_t session_id, NSError* _Nullable error) { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); if (!internal::IsInitialized()) return; if (error) { api->Complete(future_handle, -1, util::NSStringToString(error.localizedDescription).c_str()); @@ -519,7 +523,7 @@ Thread get_id_thread( } Future GetSessionIdLastResult() { - MutexLock lock(g_mutex); + MutexLock lock(AnalyticsMutex()); FIREBASE_ASSERT_RETURN(Future(), internal::IsInitialized()); return static_cast&>( internal::FutureData::Get()->api()->LastResult(internal::kAnalyticsFnGetSessionId)); diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index 5c5d735e7e..6f584e9ea2 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -250,6 +250,7 @@ set(utility_common_HDRS src/future_manager.h src/intrusive_list.h src/log.h + src/no_destructor.h src/optional.h src/path.h src/pthread_condvar.h @@ -302,6 +303,12 @@ if(ANDROID OR IOS) ${FLATBUFFERS_SOURCE_DIR}/src/idl_gen_text.cpp ${FLATBUFFERS_SOURCE_DIR}/src/reflection.cpp ${FLATBUFFERS_SOURCE_DIR}/src/util.cpp) + # Third-party FlatBuffers sources define static globals with non-trivial + # destructors; exempt them from the SDK's exit-time destructor check below. + set_source_files_properties( + ${app_flatbuffers_srcs} + PROPERTIES COMPILE_OPTIONS + "$<$:-Wno-exit-time-destructors>") set(app_flatbuffers_lib) else() set(app_flatbuffers_srcs) @@ -332,6 +339,12 @@ else() target_compile_options(firebase_app PRIVATE -fno-exceptions) endif() +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_app PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + target_include_directories(firebase_app PUBLIC ${CMAKE_CURRENT_LIST_DIR}/src/include diff --git a/app/rest/CMakeLists.txt b/app/rest/CMakeLists.txt index 2d806ad231..4c104ec965 100644 --- a/app/rest/CMakeLists.txt +++ b/app/rest/CMakeLists.txt @@ -42,6 +42,12 @@ set(rest_SRCS add_library(firebase_rest_lib STATIC ${rest_SRCS}) +# Reject static/global C++ objects with non-trivial destructors so REST +# transport locks outlive user-owned static Firebase objects at process exit. +target_compile_options( + firebase_rest_lib PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + target_include_directories(firebase_rest_lib PUBLIC ${FIREBASE_CPP_SDK_ROOT_DIR} diff --git a/app/rest/transport_curl.cc b/app/rest/transport_curl.cc index 505132510f..30d6db3c82 100644 --- a/app/rest/transport_curl.cc +++ b/app/rest/transport_curl.cc @@ -26,6 +26,7 @@ #include "app/src/assert.h" #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/internal/platform.h" +#include "app/src/no_destructor.h" #include "app/src/semaphore.h" #include "app/src/thread.h" #include "app/src/util.h" @@ -312,13 +313,16 @@ CurlThread* g_curl_thread = nullptr; // Count initializations that multiple libraries can use this simultaneously. int g_initialize_count = 0; -// Mutex for Curl initialization. -Mutex* g_initialize_mutex = new Mutex(); +// Mutex for Curl initialization and teardown. +Mutex& InitializeMutex() { + static NoDestructor initialize_mutex; + return *initialize_mutex; +} } // namespace void InitTransportCurl() { - MutexLock lock(*g_initialize_mutex); + MutexLock lock(InitializeMutex()); if (g_initialize_count == 0) { // Initialize curl. CURLcode global_init_code = curl_global_init(CURL_GLOBAL_ALL); @@ -334,7 +338,7 @@ void InitTransportCurl() { } void CleanupTransportCurl() { - MutexLock lock(*g_initialize_mutex); + MutexLock lock(InitializeMutex()); assert(g_initialize_count > 0); g_initialize_count--; if (g_initialize_count == 0) { diff --git a/app/rest/util.cc b/app/rest/util.cc index 10bb146b57..d4b0f0375f 100644 --- a/app/rest/util.cc +++ b/app/rest/util.cc @@ -21,6 +21,8 @@ #include #include +#include "app/src/include/firebase/internal/mutex.h" +#include "app/src/no_destructor.h" #include "app/src/variant_util.h" #include "curl/curl.h" #include "flatbuffers/flatbuffers.h" @@ -45,23 +47,26 @@ const char kGet[] = "GET"; const char kPost[] = "POST"; // Mutex for utils initialization and distribution of CURL pointers. -static ::firebase::Mutex* g_util_curl_mutex = new Mutex(); +static ::firebase::Mutex& UtilCurlMutex() { + static NoDestructor<::firebase::Mutex> util_curl_mutex; + return *util_curl_mutex; +} CURL* g_curl_instance = nullptr; int g_init_ref_count = 0; void* CreateCurlPtr() { // NOLINT - MutexLock lock(*g_util_curl_mutex); + MutexLock lock(UtilCurlMutex()); return curl_easy_init(); } void DestroyCurlPtr(void* curl_ptr) { - MutexLock lock(*g_util_curl_mutex); + MutexLock lock(UtilCurlMutex()); curl_easy_cleanup(static_cast(curl_ptr)); } void Initialize() { - MutexLock curl_lock(*g_util_curl_mutex); + MutexLock curl_lock(UtilCurlMutex()); assert(g_init_ref_count >= 0); if (g_init_ref_count == 0) { g_curl_instance = reinterpret_cast(CreateCurlPtr()); @@ -70,7 +75,7 @@ void Initialize() { } void Terminate() { - MutexLock curl_lock(*g_util_curl_mutex); + MutexLock curl_lock(UtilCurlMutex()); --g_init_ref_count; assert(g_init_ref_count >= 0); if (g_init_ref_count == 0) { diff --git a/app/src/app_common.cc b/app/src/app_common.cc index d752bce73e..cff6c24de1 100644 --- a/app/src/app_common.cc +++ b/app/src/app_common.cc @@ -34,6 +34,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/internal/platform.h" #include "app/src/include/firebase/version.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" // strtok_r is strtok_s on Windows. @@ -172,13 +173,18 @@ const char* kBuildSource = "custom_built"; const char* kApiClientHeader = "x-firebase-client"; const char* kXFirebaseGmpIdHeader = "X-Firebase-GMPID"; -SystemLogger g_system_logger; // NOLINT +// Wrapped in NoDestructor so per-App loggers remain valid if static Firebase +// instances log during process exit. +static SystemLogger& GetSystemLogger() { + static NoDestructor system_logger; + return *system_logger; +} // Private cross platform data associated with an app. struct AppData { // TODO(b/140528778): Remove kLogLevelVerbose here and add [GS]etLogLevel // member functions to app. - AppData() : logger(&g_system_logger, kLogLevelVerbose) {} + AppData() : logger(&GetSystemLogger(), kLogLevelVerbose) {} // App associated with this data. App* app; @@ -274,18 +280,29 @@ class LibraryRegistry { static LibraryRegistry* library_registry_; }; -// Guards g_apps and g_default_app. -static Mutex* g_app_mutex = new Mutex(); -static std::map>* g_apps = nullptr; +using AppMap = std::map>; + +// Guards Apps(), g_default_app, and LibraryRegistry. Wrapped in NoDestructor +// so static initialization and exit-time App destruction never access an +// uninitialized or destroyed mutex/map. +static Mutex& AppMutex() { + static NoDestructor app_mutex; + return *app_mutex; +} + +static AppMap& Apps() { + static NoDestructor apps; + return *apps; +} + static App* g_default_app = nullptr; LibraryRegistry* LibraryRegistry::library_registry_ = nullptr; App* AddApp(App* app, std::map* results) { - bool created_first_app = false; assert(app); App* existing_app = FindAppByName(app->name()); FIREBASE_ASSERT_RETURN(nullptr, !existing_app); - MutexLock lock(*g_app_mutex); + MutexLock lock(AppMutex()); if (IsDefaultAppName(app->name())) { assert(!g_default_app); g_default_app = app; @@ -293,12 +310,7 @@ App* AddApp(App* app, std::map* results) { std::unique_ptr app_data = std::make_unique(); app_data->app = app; app_data->cleanup_notifier.RegisterOwner(app); - if (!g_apps) { - g_apps = new std::map>(); - created_first_app = true; - } - (*g_apps)[std::string(app->name())] = std::move(app_data); - // Create a cleanup notifier for the app. + Apps()[std::string(app->name())] = std::move(app_data); { const AppOptions& app_options = app->options(); LogDebug( @@ -316,13 +328,11 @@ App* AddApp(App* app, std::map* results) { App* FindAppByName(const char* name) { assert(name); - MutexLock lock(*g_app_mutex); - if (g_apps) { - auto it = g_apps->find(std::string(name)); - if (it == g_apps->end()) return nullptr; - return it->second->app; - } - return nullptr; + MutexLock lock(AppMutex()); + auto& apps = Apps(); + auto it = apps.find(std::string(name)); + if (it == apps.end()) return nullptr; + return it->second->app; } App* GetDefaultApp() { return g_default_app; } @@ -332,9 +342,10 @@ App* GetAnyApp() { return g_default_app; } - MutexLock lock(*g_app_mutex); - if (g_apps && !g_apps->empty()) { - return g_apps->begin()->second->app; + MutexLock lock(AppMutex()); + auto& apps = Apps(); + if (!apps.empty()) { + return apps.begin()->second->app; } return nullptr; } @@ -342,9 +353,10 @@ App* GetAnyApp() { std::vector GetAllApps() { std::vector result; App* const default_app = GetDefaultApp(); - MutexLock lock(*g_app_mutex); - if (g_apps) { - for (auto it = g_apps->begin(); it != g_apps->end(); ++it) { + MutexLock lock(AppMutex()); + auto& apps = Apps(); + if (!apps.empty()) { + for (auto it = apps.begin(); it != apps.end(); ++it) { if (it->second->app != default_app) result.push_back(it->second->app); } if (default_app) result.push_back(default_app); @@ -354,31 +366,24 @@ std::vector GetAllApps() { void RemoveApp(App* app) { assert(app); - MutexLock lock(*g_app_mutex); - if (g_apps) { - auto it = g_apps->find(std::string(app->name())); - bool last_app = false; - // If app initialization failed AddApp() will not be called. - if (it != g_apps->end()) { - LogDebug("Deleting app %s (0x%08x)", app->name(), - static_cast(reinterpret_cast(app))); - // Notify app callbacks before removing the app from the g_apps - // dictionary, this makes it possible to look up apps by name on - // destruction. - it->second->cleanup_notifier.CleanupAll(); - AppCallback::NotifyAllAppDestroyed(app); - // Remove the app entry from the map, from this point it is no longer - // accessible to other components. - g_apps->erase(it); - if (app == g_default_app) { - g_default_app = nullptr; - } - if (g_apps->empty()) { - last_app = true; - delete g_apps; - g_apps = nullptr; - } + MutexLock lock(AppMutex()); + auto& apps = Apps(); + auto it = apps.find(std::string(app->name())); + // If app initialization failed AddApp() will not be called. + if (it != apps.end()) { + LogDebug("Deleting app %s (0x%08x)", app->name(), + static_cast(reinterpret_cast(app))); + // Notify app callbacks before removing the app from Apps() so callbacks + // can still look up apps by name during destruction. + it->second->cleanup_notifier.CleanupAll(); + AppCallback::NotifyAllAppDestroyed(app); + // Remove the app entry from the map; from this point it is no longer + // accessible to other components. + apps.erase(it); + if (app == g_default_app) { + g_default_app = nullptr; } + const bool last_app = apps.empty(); callback::Terminate(last_app); if (last_app) { LibraryRegistry::Terminate(); @@ -389,17 +394,17 @@ void RemoveApp(App* app) { void DestroyAllApps() { std::vector apps_to_delete; App* const default_app = GetDefaultApp(); - MutexLock lock(*g_app_mutex); - if (g_apps) { - for (auto it = g_apps->begin(); it != g_apps->end(); ++it) { + MutexLock lock(AppMutex()); + auto& apps = Apps(); + if (!apps.empty()) { + for (auto it = apps.begin(); it != apps.end(); ++it) { if (it->second->app != default_app) apps_to_delete.push_back(it->second->app); } if (default_app) apps_to_delete.push_back(default_app); for (auto it = apps_to_delete.begin(); it != apps_to_delete.end(); ++it) { - // As each App is deleted, RemoveApp() is called by the destructor which - // removes the app from the global list of apps (g_apps). When g_apps - // is empty it is deleted by RemoveApp(). + // Deleting each App invokes RemoveApp() via ~App(), removing its entry + // from Apps(). delete *it; } } @@ -411,7 +416,7 @@ bool IsDefaultAppName(const char* name) { } void RegisterLibrary(const char* library, const char* version) { - MutexLock lock(*g_app_mutex); + MutexLock lock(AppMutex()); LibraryRegistry* registry = LibraryRegistry::Initialize(); if (registry->RegisterLibrary(library, version)) { registry->UpdateUserAgent(); @@ -419,7 +424,7 @@ void RegisterLibrary(const char* library, const char* version) { } void RegisterLibrariesFromUserAgent(const char* user_agent) { - MutexLock lock(*g_app_mutex); + MutexLock lock(AppMutex()); LibraryRegistry* registry = LibraryRegistry::Initialize(); // Copy the string into a vector so that we can safely mutate the string. std::vector user_agent_vector(user_agent, @@ -447,7 +452,7 @@ void RegisterLibrariesFromUserAgent(const char* user_agent) { } void RegisterSdkUsage(void* platform_resource) { - MutexLock lock(*g_app_mutex); + MutexLock lock(AppMutex()); // Only register libraries when no C++ apps was created before. if (!LibraryRegistry::IsCommonLibraryRegistered()) { @@ -469,12 +474,12 @@ void RegisterSdkUsage(void* platform_resource) { } const char* GetUserAgent() { - MutexLock lock(*g_app_mutex); + MutexLock lock(AppMutex()); return LibraryRegistry::Initialize()->GetUserAgent(); } std::string GetLibraryVersion(const char* library) { - MutexLock lock(*g_app_mutex); + MutexLock lock(AppMutex()); return LibraryRegistry::Initialize()->GetLibraryVersion(library); } @@ -484,7 +489,7 @@ void GetOuterMostSdkAndVersion(std::string* sdk, std::string* version) { sdk->clear(); version->clear(); - MutexLock lock(*g_app_mutex); + MutexLock lock(AppMutex()); // 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. @@ -508,13 +513,11 @@ 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); - if (g_apps) { - auto it = g_apps->find(std::string(name)); - if (it == g_apps->end()) return nullptr; - return &it->second->logger; - } - return nullptr; + MutexLock lock(AppMutex()); + auto& apps = Apps(); + auto it = apps.find(std::string(name)); + if (it == apps.end()) return nullptr; + return &it->second->logger; } } // namespace app_common diff --git a/app/src/app_desktop.cc b/app/src/app_desktop.cc index 412ef5dede..1bb93e0f28 100644 --- a/app/src/app_desktop.cc +++ b/app/src/app_desktop.cc @@ -32,6 +32,7 @@ #include "app/src/include/firebase/internal/platform.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/semaphore.h" #include "app/src/util.h" @@ -40,6 +41,13 @@ DEFINE_FIREBASE_VERSION_STRING(Firebase); namespace internal { +// Directory prefix searched by AppOptions::LoadDefault when Create() is +// invoked without explicit options. +static std::string& DefaultConfigPath() { + static NoDestructor default_config_path; + return *default_config_path; +} + // Attempts to load a config file from the path specified, and use it to // populate the AppOptions pointer. Returns true on success, false otherwise. bool LoadAppOptionsFromJsonConfigFile(const char* path, AppOptions* options) { @@ -88,7 +96,7 @@ bool LoadAppOptionsFromJsonConfigFile(const char* path, AppOptions* options) { } // namespace internal -// Searches internal::g_default_config_path for filenames matching +// Searches internal::DefaultConfigPath() for filenames matching // kDefaultGoogleServicesNames attempting to load the app options from each // config file in turn. AppOptions* AppOptions::LoadDefault(AppOptions* options) { @@ -106,7 +114,7 @@ AppOptions* AppOptions::LoadDefault(AppOptions* options) { FIREBASE_ARRAYSIZE(kDefaultGoogleServicesNames); for (size_t i = 0; i < number_of_config_filenames; i++) { std::string full_path = - internal::g_default_config_path + kDefaultGoogleServicesNames[i]; + internal::DefaultConfigPath() + kDefaultGoogleServicesNames[i]; if (internal::LoadAppOptionsFromJsonConfigFile(full_path.c_str(), options)) { return options; @@ -150,7 +158,9 @@ App* App::Create(const AppOptions& options, const char* name) { // NOLINT } LogDebug("Creating Firebase App %s for %s", name, kFirebaseVersionString); LogDebug("Validating semaphore creation."); - { firebase::Semaphore sem_test(0); } + { + firebase::Semaphore sem_test(0); + } AppOptions options_with_defaults = options; if (options_with_defaults.PopulateRequiredWithDefaults()) { @@ -200,7 +210,8 @@ void App::RegisterLibrary(const char* library, const char* version, const char* App::GetUserAgent() { return app_common::GetUserAgent(); } void App::SetDefaultConfigPath(const char* path) { - internal::g_default_config_path = path; + std::string& config_path = internal::DefaultConfigPath(); + config_path = path; #if defined(WIN32) const char kSeperator = '\\'; @@ -208,10 +219,10 @@ void App::SetDefaultConfigPath(const char* path) { const char kSeperator = '/'; #endif - if (!internal::g_default_config_path.empty()) { - char last_character = internal::g_default_config_path.back(); + if (!config_path.empty()) { + char last_character = config_path.back(); if (last_character != '\\' && last_character != '/') { - internal::g_default_config_path += kSeperator; + config_path += kSeperator; } } } diff --git a/app/src/app_desktop.h b/app/src/app_desktop.h index e32062b5ca..65c039c83f 100644 --- a/app/src/app_desktop.h +++ b/app/src/app_desktop.h @@ -43,10 +43,5 @@ class AppInternal { firebase::heartbeat::DateProviderImpl date_provider_; }; -// When Create() is invoked without arguments, it checks for a file named -// google-services-desktop.json, to load options from. This specifies the -// path to search. -static std::string g_default_config_path; // NOLINT - } // namespace internal } // namespace firebase diff --git a/app/src/app_ios.mm b/app/src/app_ios.mm index 653c0c2043..f89456516f 100644 --- a/app/src/app_ios.mm +++ b/app/src/app_ios.mm @@ -27,6 +27,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "app/src/util_ios.h" @@ -357,44 +358,39 @@ void SetFirConfigurationLoggerLevel(FIRLoggerLevel level) { } } -// Guards g_partial_apps. -static Mutex* g_partial_apps_mutex = new Mutex(); -// Stores partially initialized apps, indexed by app name -static std::map* g_partial_apps; +using PartialAppMap = std::map; + +// Guards PartialApps(). +static Mutex& PartialAppsMutex() { + static NoDestructor partial_apps_mutex; + return *partial_apps_mutex; +} + +// Stores partially initialized apps, indexed by app name. +static PartialAppMap& PartialApps() { + static NoDestructor partial_apps; + return *partial_apps; +} App* FindPartialAppByName(const char* name) { assert(name); - MutexLock lock(*g_partial_apps_mutex); - if (g_partial_apps) { - auto it = g_partial_apps->find(std::string(name)); - if (it == g_partial_apps->end()) { - return nullptr; - } - return it->second; + MutexLock lock(PartialAppsMutex()); + auto& partial_apps = PartialApps(); + auto it = partial_apps.find(std::string(name)); + if (it == partial_apps.end()) { + return nullptr; } - return nullptr; + return it->second; } void AddPartialApp(App* app) { - MutexLock lock(*g_partial_apps_mutex); - if (!g_partial_apps) { - g_partial_apps = new std::map(); - } - (*g_partial_apps)[std::string(app->name())] = app; + MutexLock lock(PartialAppsMutex()); + PartialApps()[std::string(app->name())] = app; } void RemovePartialApp(App* app) { - MutexLock lock(*g_partial_apps_mutex); - if (g_partial_apps) { - auto it = g_partial_apps->find(std::string(app->name())); - if (it != g_partial_apps->end()) { - g_partial_apps->erase(it); - if (g_partial_apps->empty()) { - delete g_partial_apps; - g_partial_apps = nullptr; - } - } - } + MutexLock lock(PartialAppsMutex()); + PartialApps().erase(std::string(app->name())); } } // namespace internal diff --git a/app/src/callback.cc b/app/src/callback.cc index 7fef81b45c..38d01f62e4 100644 --- a/app/src/callback.cc +++ b/app/src/callback.cc @@ -21,6 +21,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/semaphore.h" #include "app/src/thread.h" @@ -182,14 +183,18 @@ 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 = new Mutex(); static int g_callback_ref_count = 0; static Thread::Id g_callback_thread_id; static bool g_callback_thread_id_initialized = false; +// Mutex that controls access to g_callback_dispatcher and g_callback_ref_count. +static Mutex& CallbackMutex() { + static NoDestructor callback_mutex; + return *callback_mutex; +} + void Initialize() { - MutexLock lock(*g_callback_mutex); + MutexLock lock(CallbackMutex()); if (g_callback_ref_count == 0) { g_callback_dispatcher = new CallbackDispatcher(); } @@ -198,7 +203,7 @@ void Initialize() { // Add a reference to the module if it's already initialized. static bool InitializeIfInitialized() { - MutexLock lock(*g_callback_mutex); + MutexLock lock(CallbackMutex()); if (IsInitialized()) { Initialize(); return true; @@ -213,7 +218,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(CallbackMutex()); if (!g_callback_ref_count) { LogWarning("Callback module already shut down"); return; @@ -235,13 +240,13 @@ static void Terminate(int number_of_references_to_remove) { } void Terminate(bool flush_all) { - MutexLock lock(*g_callback_mutex); + MutexLock lock(CallbackMutex()); 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 - // the outstanding number of items in the queue. In particular, - // PollDispatcher() could be executing at this point since g_callback_mutex - // isn't held by the ref count is > 0 for the duration of the function. + // the outstanding number of items in the queue. In particular, + // PollCallbacks() could be executing at this point since CallbackMutex() + // isn't held while dispatching, though ref_count > 0 for the duration. if (flush_all && g_callback_dispatcher) { ref_count += g_callback_dispatcher->FlushCallbacks(); } @@ -249,7 +254,7 @@ void Terminate(bool flush_all) { } void* AddCallback(Callback* callback) { - MutexLock lock(*g_callback_mutex); + MutexLock lock(CallbackMutex()); Initialize(); return g_callback_dispatcher->AddCallback(callback); } diff --git a/app/src/cleanup_notifier.cc b/app/src/cleanup_notifier.cc index a02114b701..0f82fcc1d6 100644 --- a/app/src/cleanup_notifier.cc +++ b/app/src/cleanup_notifier.cc @@ -20,32 +20,36 @@ #include +#include "app/src/no_destructor.h" + namespace firebase { +namespace { -Mutex *CleanupNotifier::cleanup_notifiers_by_owner_mutex_ = new Mutex(); -std::map - *CleanupNotifier::cleanup_notifiers_by_owner_; +using CleanupNotifierMap = std::map; -CleanupNotifier::CleanupNotifier() : cleaned_up_(false) { - MutexLock lock(*cleanup_notifiers_by_owner_mutex_); - if (!cleanup_notifiers_by_owner_) { - cleanup_notifiers_by_owner_ = new std::map(); - } +// Guards CleanupNotifiersByOwner() and CleanupNotifier::owners_. Wrapped in +// NoDestructor so static initialization and exit-time destructor unregistration +// never observe an uninitialized or destroyed mutex/map. +Mutex& CleanupNotifiersMutex() { + static NoDestructor mutex; + return *mutex; +} + +CleanupNotifierMap& CleanupNotifiersByOwner() { + static NoDestructor notifiers; + return *notifiers; } +} // namespace + +CleanupNotifier::CleanupNotifier() : cleaned_up_(false) {} + CleanupNotifier::~CleanupNotifier() { CleanupAll(); UnregisterAllOwners(); - { - 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; - } - } } -void CleanupNotifier::RegisterObject(void *object, CleanupCallback callback) { +void CleanupNotifier::RegisterObject(void* object, CleanupCallback callback) { MutexLock lock(mutex_); auto i = callbacks_.find(object); if (i != callbacks_.end()) { @@ -55,7 +59,7 @@ void CleanupNotifier::RegisterObject(void *object, CleanupCallback callback) { } } -void CleanupNotifier::UnregisterObject(void *object) { +void CleanupNotifier::UnregisterObject(void* object) { MutexLock lock(mutex_); callbacks_.erase(object); } @@ -64,7 +68,7 @@ void CleanupNotifier::CleanupAll() { MutexLock lock(mutex_); if (!cleaned_up_) { while (callbacks_.begin() != callbacks_.end()) { - std::pair object_and_callback = + std::pair object_and_callback = *callbacks_.begin(); object_and_callback.second(object_and_callback.first); UnregisterObject(object_and_callback.first); @@ -74,46 +78,46 @@ void CleanupNotifier::CleanupAll() { } void CleanupNotifier::UnregisterAllOwners() { - MutexLock lock(*cleanup_notifiers_by_owner_mutex_); + MutexLock lock(CleanupNotifiersMutex()); while (owners_.begin() != owners_.end()) { UnregisterOwner(this, owners_[0]); } } -void CleanupNotifier::RegisterOwner(CleanupNotifier *notifier, void *owner) { - 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); - (*cleanup_notifiers_by_owner_)[owner] = notifier; +void CleanupNotifier::RegisterOwner(CleanupNotifier* notifier, void* owner) { + MutexLock lock(CleanupNotifiersMutex()); + auto& notifiers = CleanupNotifiersByOwner(); + auto it = notifiers.find(owner); + if (it != notifiers.end()) UnregisterOwner(it); + notifiers[owner] = notifier; notifier->owners_.push_back(owner); } -void CleanupNotifier::UnregisterOwner(CleanupNotifier *notifier, void *owner) { - 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(CleanupNotifier* /*notifier*/, + void* owner) { + MutexLock lock(CleanupNotifiersMutex()); + auto& notifiers = CleanupNotifiersByOwner(); + auto it = notifiers.find(owner); + if (it != notifiers.end()) UnregisterOwner(it); } void CleanupNotifier::UnregisterOwner( - std::map::iterator it) { - MutexLock lock(*cleanup_notifiers_by_owner_mutex_); - assert(cleanup_notifiers_by_owner_); - void *owner = it->first; - CleanupNotifier *notifier = it->second; - cleanup_notifiers_by_owner_->erase(it); - auto *owners = ¬ifier->owners_; + std::map::iterator it) { + MutexLock lock(CleanupNotifiersMutex()); + void* owner = it->first; + CleanupNotifier* notifier = it->second; + CleanupNotifiersByOwner().erase(it); + auto* owners = ¬ifier->owners_; auto owner_it = std::find(owners->begin(), owners->end(), owner); assert(owner_it != owners->end()); owners->erase(owner_it); } -CleanupNotifier *CleanupNotifier::FindByOwner(void *owner) { - 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; +CleanupNotifier* CleanupNotifier::FindByOwner(void* owner) { + MutexLock lock(CleanupNotifiersMutex()); + auto& notifiers = CleanupNotifiersByOwner(); + auto it = notifiers.find(owner); + return it != notifiers.end() ? it->second : nullptr; } // NOLINTNEXTLINE - allow namespace overridden diff --git a/app/src/cleanup_notifier.h b/app/src/cleanup_notifier.h index 45846c47b1..d0082e8439 100644 --- a/app/src/cleanup_notifier.h +++ b/app/src/cleanup_notifier.h @@ -41,7 +41,7 @@ namespace firebase { // owner (which is about to be deleted). class CleanupNotifier { public: - typedef void (*CleanupCallback)(void *object); + typedef void (*CleanupCallback)(void* object); // Default constructor. CleanupNotifier(); @@ -52,11 +52,11 @@ class CleanupNotifier { // Register a callback to be called on a given object when it's time for // cleanup. If this object already has a callback registered, the previous // callback will be overwritten. - void RegisterObject(void *object, CleanupCallback callback); + void RegisterObject(void* object, CleanupCallback callback); // Unregister an object. This will remove it from the cleanup list without // calling the cleanup callback. - void UnregisterObject(void *object); + void UnregisterObject(void* object); // Call all cleanup callbacks, clearing the list. You can call this manually // rather than using the destructor if you want more control over when it @@ -64,43 +64,38 @@ class CleanupNotifier { void CleanupAll(); // Register an owner with the notifier. - void RegisterOwner(void *owner) { RegisterOwner(this, owner); } + void RegisterOwner(void* owner) { RegisterOwner(this, owner); } // Unregister an owner from the notifier. - void UnregisterOwner(void *owner) { UnregisterOwner(this, owner); } + void UnregisterOwner(void* owner) { UnregisterOwner(this, owner); } // Find a cleanup notifier by owner object. // If a cleanup notifier was registered with an owner object using // RegisterOwner(). - static CleanupNotifier *FindByOwner(void *owner); + static CleanupNotifier* FindByOwner(void* owner); private: // Register the notifier's association with the owner object. // Notifiers can be register to multiple owner objects. - static void RegisterOwner(CleanupNotifier *notifier, void *owner); + static void RegisterOwner(CleanupNotifier* notifier, void* owner); // Unregister a notifier from an owner object. - static void UnregisterOwner(CleanupNotifier *notifier, void *owner); + static void UnregisterOwner(CleanupNotifier* notifier, void* owner); // Unregister this notifier with all owner objects. void UnregisterAllOwners(); // Unregister a notifier from an owner object. - static void UnregisterOwner(std::map::iterator it); + static void UnregisterOwner(std::map::iterator it); private: // Guards callbacks_ and cleaned_up_. Mutex mutex_; - std::map callbacks_; + std::map callbacks_; bool cleaned_up_; - // List of owners of this notifier. - // This is the inverse of cleanup_notifiers_by_owner_ for a notifier. - std::vector owners_; - - // Guards owners_ and cleanup_notifiers_by_owner_. - static Mutex *cleanup_notifiers_by_owner_mutex_; - // Global map of cleanup notifiers bucketed by owner object. - static std::map *cleanup_notifiers_by_owner_; + // List of owners registered for this notifier (guarded by + // CleanupNotifiersMutex() in cleanup_notifier.cc). + std::vector owners_; }; // Typed wrapper for CleanupNotifier. Helpful if you only need to clean @@ -108,19 +103,19 @@ class CleanupNotifier { template class TypedCleanupNotifier { public: - typedef void (*CleanupCallback)(T *object); + typedef void (*CleanupCallback)(T* object); - void RegisterObject(T *object, CleanupCallback callback) { + void RegisterObject(T* object, CleanupCallback callback) { notifier_.RegisterObject( object, reinterpret_cast(callback)); } - void UnregisterObject(T *object) { notifier_.UnregisterObject(object); } + void UnregisterObject(T* object) { notifier_.UnregisterObject(object); } void CleanupAll() { notifier_.CleanupAll(); } // Get the underlying notifier. - CleanupNotifier &cleanup_notifier() { return notifier_; } + CleanupNotifier& cleanup_notifier() { return notifier_; } private: CleanupNotifier notifier_; diff --git a/app/src/log.cc b/app/src/log.cc index acfc302680..0a75d8af74 100644 --- a/app/src/log.cc +++ b/app/src/log.cc @@ -25,6 +25,7 @@ #include "app/src/assert.h" #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/internal/platform.h" +#include "app/src/no_destructor.h" #if !defined(FIREBASE_LOG_DEBUG) #define FIREBASE_LOG_DEBUG 0 @@ -52,20 +53,19 @@ const LogLevel kDefaultLogLevel = kLogLevelInfo; LogLevel g_log_level = kDefaultLogLevel; LogCallback g_log_callback = DefaultLogCallback; void* g_log_callback_data = nullptr; -// Mutex which controls access to the log buffer used to format callback -// log messages. -Mutex* g_log_mutex = nullptr; - -// Initialize g_log_mutex when static constructors are called. -// This class makes sure g_log_mutex is initialized typically initialized -// before the first log function is called. -class InitializeLogMutex { - public: - InitializeLogMutex() { - if (!g_log_mutex) g_log_mutex = new Mutex(); - } -}; -InitializeLogMutex g_log_mutex_initializer; + +namespace { + +// Serializes formatting into the shared callback log buffer. Using a +// function-local NoDestructor guarantees thread-safe initialization on +// first log call (even from pre-main static constructors) and keeps the mutex +// alive for exit-time destructors that log warnings or debug messages. +Mutex& LogMutex() { + static NoDestructor log_mutex; + return *log_mutex; +} + +} // namespace // Forward a log message to LogMessageV(). static void InternalLogMessage(LogLevel log_level, const char* message, ...) { @@ -111,7 +111,7 @@ static void LogToFile(LogLevel log_level, const char* format, va_list args) { fflush(log_file); } } else { - MutexLock lock(*g_log_mutex); + MutexLock lock(LogMutex()); if (!log_file) { #if FIREBASE_PLATFORM_WINDOWS log_file = _wfopen(FIREBASE_LOG_FILENAME_W, L"wt"); @@ -132,13 +132,7 @@ static void LogToFile(LogLevel log_level, const char* format, va_list args) { // Log a firebase message (implemented by the platform specific logger). void LogMessageWithCallbackV(LogLevel log_level, const char* format, va_list args) { - // We create the mutex on the heap as this can be called before the C++ - // runtime is initialized on iOS. This ensures the Mutex class is - // constructed before we attempt to use it. Of course, this isn't thread - // safe but the first time this is called on any platform it will be from - // a single thread to initialize the API. - if (!g_log_mutex) g_log_mutex = new Mutex(); - MutexLock lock(*g_log_mutex); + MutexLock lock(LogMutex()); LogInitialize(); #if FIREBASE_LOG_TO_FILE diff --git a/app/src/log_stdio.cc b/app/src/log_stdio.cc index 7625d9df81..9bdd01383d 100644 --- a/app/src/log_stdio.cc +++ b/app/src/log_stdio.cc @@ -28,6 +28,7 @@ #endif // FIREBASE_PLATFORM_WINDOWS #include "app/src/include/firebase/internal/mutex.h" +#include "app/src/no_destructor.h" namespace firebase { @@ -42,9 +43,14 @@ static const char* kLogLevelPrefix[] = { }; #if FIREBASE_PLATFORM_WINDOWS -// Guards the log buffer on Windows. -static Mutex g_log_mutex; // NOLINT -#endif // FIREBASE_PLATFORM_WINDOWS +// Guards the OutputDebugString log buffer on Windows. Wrapped in NoDestructor +// so logging from static destructors at process exit does not lock a closed +// Windows mutex handle. +static Mutex& PlatformLogMutex() { + static NoDestructor log_mutex; + return *log_mutex; +} +#endif // FIREBASE_PLATFORM_WINDOWS // Initializes the logging module. void LogInitialize() {} @@ -62,7 +68,7 @@ void LogMessageV(LogLevel log_level, const char* format, va_list args) { // Platform specific logging. #if FIREBASE_PLATFORM_WINDOWS { - MutexLock lock(g_log_mutex); + MutexLock lock(PlatformLogMutex()); static char log_buffer[1024]; size_t prefix_length = strlen(prefix); strcpy(log_buffer, prefix); // // NOLINT diff --git a/app/src/no_destructor.h b/app/src/no_destructor.h index 14441f3960..aedde395b8 100644 --- a/app/src/no_destructor.h +++ b/app/src/no_destructor.h @@ -18,54 +18,81 @@ #define FIREBASE_APP_SRC_NO_DESTRUCTOR_H_ #include +#include #include namespace firebase { +namespace internal { -// Wraps a T so that its destructor is never run. +// Wraps an instance of T in inline aligned storage whose destructor is never +// called. // -// Intended for function-local statics that must outlive static destruction, -// such as the registries and mutexes that Firebase instances touch from their -// own destructors. A plain `static T t;` is destroyed at exit in reverse order -// of construction, so an instance owned by a user's static object can find it -// already gone. A `static T* t = new T;` avoids that but hands callers a raw -// pointer and a heap allocation. NoDestructor keeps the T inline, constructs it -// in place on first use, and simply never destroys it. +// Intended for function-local statics that must outlive process teardown, such +// as global registries, mutexes, and loggers accessed when user-owned static +// Firebase objects run their destructors during exit(): // +// - A plain `static T value;` registers an exit-time destructor (`__cxa_atexit` +// / `atexit`) that runs in reverse construction order, so destructors of +// other static objects constructed earlier can observe a destroyed `T`. +// - A namespace-scope `static T* value = new T();` avoids exit-time +// destruction, but introduces dynamic initialization order hazards across +// translation units before `main()`. +// - A function-local `static NoDestructor value;` is initialized thread- +// safely on first call (C++11 [stmt.dcl]), stores `T` inline with no extra +// heap allocation, has a trivial destructor so no exit-time destructor is +// registered, and remains reachable via static storage so leak checkers do +// not report it as leaked. +// +// Example: // static Mutex& RegistryMutex() { // static NoDestructor mutex; // return *mutex; // } -// -// The object is reachable from the static for the lifetime of the process, so -// leak checkers do not report it. template class NoDestructor { public: // Constructs the wrapped T in place, forwarding any arguments. - template + // Disabled when decaying a single argument yields NoDestructor itself so + // non-const lvalue copy attempts hit the deleted copy constructor instead of + // forwarding into T's constructors (for example, firebase::Variant). + template ::type&...), + void(NoDestructor&)>::value, + int>::type = 0> explicit NoDestructor(Args&&... args) { - new (static_cast(storage_)) T(std::forward(args)...); + new (storage_) T(std::forward(args)...); } + // Explicit copy/move overloads for T so brace-enclosed initializers (such as + // `NoDestructor>({1, 2})`) can deduce T. + explicit NoDestructor(const T& x) { new (storage_) T(x); } + explicit NoDestructor(T&& x) { new (storage_) T(std::move(x)); } + NoDestructor(const NoDestructor&) = delete; NoDestructor& operator=(const NoDestructor&) = delete; - // Intentionally does not destroy the wrapped T. + // Trivially destructible: intentionally never destroys the wrapped T. ~NoDestructor() = default; T& operator*() { return *get(); } T* operator->() { return get(); } - T* get() { return reinterpret_cast(storage_); } + T* get() { return std::launder(reinterpret_cast(storage_)); } const T& operator*() const { return *get(); } const T* operator->() const { return get(); } - const T* get() const { return reinterpret_cast(storage_); } + const T* get() const { + return std::launder(reinterpret_cast(storage_)); + } private: alignas(T) unsigned char storage_[sizeof(T)]; }; +} // namespace internal + +using internal::NoDestructor; + } // namespace firebase #endif // FIREBASE_APP_SRC_NO_DESTRUCTOR_H_ diff --git a/app/src/reference_counted_future_impl.cc b/app/src/reference_counted_future_impl.cc index 96026fcfef..b58197b4ee 100644 --- a/app/src/reference_counted_future_impl.cc +++ b/app/src/reference_counted_future_impl.cc @@ -26,6 +26,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/intrusive_list.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" // Set this to 1 to enable verbose logging in this module. #if !defined(FIREBASE_FUTURE_TRACE_ENABLE) @@ -47,9 +48,10 @@ static_assert(sizeof(FutureBase) == sizeof(Future), typedef void DataDeleteFn(void* data_to_delete); typedef std::pair BackingPair; -// NOLINTNEXTLINE -const FutureHandle ReferenceCountedFutureImpl::kInvalidHandle( - kInvalidFutureHandle); +const FutureHandle& ReferenceCountedFutureImpl::InvalidHandle() { + static const NoDestructor invalid_handle(kInvalidFutureHandle); + return *invalid_handle; +} namespace { @@ -74,7 +76,7 @@ class FutureProxyManager { MutexLock lock(mutex_); for (FutureHandle& h : clients_) { api_->ForceReleaseFuture(h); - h = ReferenceCountedFutureImpl::kInvalidHandle; + h = ReferenceCountedFutureImpl::InvalidHandle(); } clients_.clear(); } @@ -110,7 +112,7 @@ class FutureProxyManager { MutexLock lock(mutex_); for (FutureHandle& h : clients_) { if (h == handle) { - h = ReferenceCountedFutureImpl::kInvalidHandle; + h = ReferenceCountedFutureImpl::InvalidHandle(); // Release one reference. This can delete subject_, which in turn will // delete `this`, as the subject owns the proxy. This is expected and // fine; as long as we don't do anything after the ReleaseFuture call. @@ -123,7 +125,7 @@ class FutureProxyManager { void CompleteClients(int error, const char* error_msg) { MutexLock lock(mutex_); for (const FutureHandle& h : clients_) { - if (h != ReferenceCountedFutureImpl::kInvalidHandle) { + if (h != ReferenceCountedFutureImpl::InvalidHandle()) { api_->Complete(h, error, error_msg); } } diff --git a/app/src/reference_counted_future_impl.h b/app/src/reference_counted_future_impl.h index 0ca87712d7..b7d7c477f7 100644 --- a/app/src/reference_counted_future_impl.h +++ b/app/src/reference_counted_future_impl.h @@ -27,6 +27,7 @@ #include "app/src/include/firebase/future.h" #include "app/src/include/firebase/internal/common.h" #include "app/src/include/firebase/internal/mutex.h" +#include "app/src/no_destructor.h" namespace firebase { @@ -66,16 +67,17 @@ class SafeFutureHandle { // See FutureHandle::Detach. void Detach() { handle_.Detach(); } - static const SafeFutureHandle kInvalidHandle; + // Type-safe version of kInvalidFutureHandle. Wrapped in NoDestructor so ODR + // uses of SafeFutureHandle never register an exit-time destructor. + static const SafeFutureHandle& InvalidHandle() { + static const NoDestructor invalid_handle; + return *invalid_handle; + } private: FutureHandle handle_; }; -// Type-safe version of kInvalidFutureHandle. -template -const SafeFutureHandle SafeFutureHandle::kInvalidHandle; - /// @brief Backing class for Futures that allows a Future to have multiple /// copies. When no copies remain, the Future is invalidated. /// @@ -98,9 +100,10 @@ const SafeFutureHandle SafeFutureHandle::kInvalidHandle; // unsafe methods. class ReferenceCountedFutureImpl : public detail::FutureApiInterface { public: - /// This handle will never be returned by @ref Alloc, so you can use it - /// to signify an uninitialized or invalid value. - static const FutureHandle kInvalidHandle; + /// Returns a handle that will never be returned by @ref Alloc, used to + /// signify an uninitialized or invalid value without registering a static + /// exit-time destructor. + static const FutureHandle& InvalidHandle(); /// Returned by @ref GetFutureError when the passed in handle is invalid. static constexpr int kErrorFutureIsNoLongerValid = -1; diff --git a/app/src/secure/user_secure_manager.cc b/app/src/secure/user_secure_manager.cc index 0f24c53c4a..4967719871 100644 --- a/app/src/secure/user_secure_manager.cc +++ b/app/src/secure/user_secure_manager.cc @@ -20,6 +20,7 @@ #include "app/src/base64.h" #include "app/src/callback.h" #include "app/src/include/firebase/internal/platform.h" +#include "app/src/no_destructor.h" #include "app/src/secure/user_secure_internal.h" // Only build this implementation on desktop. If building for iOS or Android, @@ -53,7 +54,16 @@ namespace secure { using callback::NewCallback; -Mutex* UserSecureManager::s_scheduler_mutex_ = new Mutex(); +namespace { + +// Guards UserSecureManager::s_scheduler_ and s_scheduler_ref_count_. +Mutex& SchedulerMutex() { + static NoDestructor scheduler_mutex; + return *scheduler_mutex; +} + +} // namespace + scheduler::Scheduler* UserSecureManager::s_scheduler_; int32_t UserSecureManager::s_scheduler_ref_count_; @@ -187,7 +197,7 @@ Future UserSecureManager::DeleteAllData() { } void UserSecureManager::CreateScheduler() { - MutexLock lock(*s_scheduler_mutex_); + MutexLock lock(SchedulerMutex()); if (s_scheduler_ == nullptr) { s_scheduler_ = new scheduler::Scheduler(); // reset count @@ -197,7 +207,7 @@ void UserSecureManager::CreateScheduler() { } void UserSecureManager::DestroyScheduler() { - MutexLock lock(*s_scheduler_mutex_); + MutexLock lock(SchedulerMutex()); if (s_scheduler_ == nullptr) { // reset count s_scheduler_ref_count_ = 0; diff --git a/app/src/secure/user_secure_manager.h b/app/src/secure/user_secure_manager.h index 5894e2f7d9..ab1abd9cd2 100644 --- a/app/src/secure/user_secure_manager.h +++ b/app/src/secure/user_secure_manager.h @@ -76,8 +76,6 @@ class UserSecureManager { static void CreateScheduler(); static void DestroyScheduler(); - // Guards static scheduler pointer - static Mutex* s_scheduler_mutex_; static scheduler::Scheduler* s_scheduler_; static int32_t s_scheduler_ref_count_; diff --git a/app/src/util.cc b/app/src/util.cc index d74fb83d08..57efafe0db 100644 --- a/app/src/util.cc +++ b/app/src/util.cc @@ -32,6 +32,7 @@ #endif // FIREBASE_PLATFORM_ANDROID #include "app/src/include/firebase/internal/mutex.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/reference_counted_future_impl.h" #include "app/src/util.h" @@ -173,17 +174,43 @@ Future ModuleInitializer::InitializeLastResult() { data_->future_impl.LastResult(kModuleInitializerInitialize)); } -std::map* AppCallback::callbacks_; -Mutex* AppCallback::callbacks_mutex_ = new Mutex(); +namespace { + +using AppCallbackMap = std::map; +using StaticFutureDataMap = std::map; + +// Guards Callbacks() after static registration completes. +Mutex& CallbacksMutex() { + static NoDestructor callbacks_mutex; + return *callbacks_mutex; +} + +AppCallbackMap& Callbacks() { + static NoDestructor callbacks; + return *callbacks; +} + +// Guards FutureDatas(). Wrapped in NoDestructor so module teardown (such as +// Auth::DeleteInternal -> CleanupCredentialFutureImpl) remains safe during +// static destruction. +Mutex& FuturesMutex() { + static NoDestructor futures_mutex; + return *futures_mutex; +} + +StaticFutureDataMap& FutureDatas() { + static NoDestructor future_datas; + return *future_datas; +} + +} // namespace void AppCallback::NotifyAllAppCreated( firebase::App* app, std::map* results) { if (results) results->clear(); - MutexLock lock(*callbacks_mutex_); - if (!callbacks_) return; - for (std::map::const_iterator it = - callbacks_->begin(); - it != callbacks_->end(); ++it) { + MutexLock lock(CallbacksMutex()); + const auto& callbacks = Callbacks(); + for (auto it = callbacks.begin(); it != callbacks.end(); ++it) { const AppCallback* callback = it->second; if (callback->enabled()) { InitResult result = callback->NotifyAppCreated(app); @@ -193,11 +220,9 @@ void AppCallback::NotifyAllAppCreated( } void AppCallback::NotifyAllAppDestroyed(firebase::App* app) { - MutexLock lock(*callbacks_mutex_); - if (!callbacks_) return; - for (std::map::const_iterator it = - callbacks_->begin(); - it != callbacks_->end(); ++it) { + MutexLock lock(CallbacksMutex()); + const auto& callbacks = Callbacks(); + for (auto it = callbacks.begin(); it != callbacks.end(); ++it) { const AppCallback* callback = it->second; if (callback->enabled()) callback->NotifyAppDestroyed(app); } @@ -205,11 +230,10 @@ void AppCallback::NotifyAllAppDestroyed(firebase::App* app) { // Determine whether a module callback is enabled, by name. bool AppCallback::GetEnabledByName(const char* name) { - MutexLock lock(*callbacks_mutex_); - if (!callbacks_) return false; - std::map::const_iterator it = - callbacks_->find(std::string(name)); - if (it == callbacks_->end()) { + MutexLock lock(CallbacksMutex()); + const auto& callbacks = Callbacks(); + auto it = callbacks.find(std::string(name)); + if (it == callbacks.end()) { return false; } return it->second->enabled(); @@ -217,11 +241,10 @@ bool AppCallback::GetEnabledByName(const char* name) { // Enable a module callback by name. void AppCallback::SetEnabledByName(const char* name, bool enable) { - MutexLock lock(*callbacks_mutex_); - if (!callbacks_) return; - std::map::const_iterator it = - callbacks_->find(std::string(name)); - if (it == callbacks_->end()) { + MutexLock lock(CallbacksMutex()); + const auto& callbacks = Callbacks(); + auto it = callbacks.find(std::string(name)); + if (it == callbacks.end()) { LogDebug("App initializer %s not found, failed to enable.", name); return; } @@ -230,23 +253,20 @@ void AppCallback::SetEnabledByName(const char* name, bool enable) { } void AppCallback::SetEnabledAll(bool enable) { - MutexLock lock(*callbacks_mutex_); - if (!callbacks_) return; + MutexLock lock(CallbacksMutex()); + const auto& callbacks = Callbacks(); + if (callbacks.empty()) return; LogDebug("%s all app initializers", enable ? "Enabling" : "Disabling"); - for (std::map::const_iterator it = - callbacks_->begin(); - it != callbacks_->end(); ++it) { + for (auto it = callbacks.begin(); it != callbacks.end(); ++it) { LogDebug("%s %s", enable ? "Enable" : "Disable", it->second->module_name()); it->second->set_enabled(enable); } } void AppCallback::AddCallback(AppCallback* callback) { - if (!callbacks_) { - callbacks_ = new std::map(); - } + auto& callbacks = Callbacks(); std::string name = callback->module_name(); - if (callbacks_->find(name) != callbacks_->end()) { + if (callbacks.find(name) != callbacks.end()) { LogWarning( "%s is already registered for callbacks on app initialization, " "ignoring.", @@ -254,40 +274,31 @@ void AppCallback::AddCallback(AppCallback* callback) { } else { LogDebug("Registered app initializer %s (enabled: %d)", name.c_str(), callback->enabled() ? 1 : 0); - (*callbacks_)[name] = callback; + callbacks[name] = callback; } } -Mutex* StaticFutureData::s_futures_mutex_ = new Mutex(); -std::map* StaticFutureData::s_future_datas_; - // static void StaticFutureData::CleanupFutureDataForModule( const void* module_identifier) { - MutexLock lock(*s_futures_mutex_); - - if (s_future_datas_ == nullptr) return; - - auto it = s_future_datas_->find(module_identifier); - if (it != s_future_datas_->end()) { + MutexLock lock(FuturesMutex()); + auto& future_datas = FutureDatas(); + auto it = future_datas.find(module_identifier); + if (it != future_datas.end()) { StaticFutureData* existing_data = it->second; if (existing_data != nullptr) delete existing_data; - s_future_datas_->erase(it); + future_datas.erase(it); } } // static StaticFutureData* StaticFutureData::GetFutureDataForModule( const void* module_identifier, int num_functions) { - MutexLock lock(*s_futures_mutex_); - - if (s_future_datas_ == nullptr) { - s_future_datas_ = new std::map; - } - - auto found_value = s_future_datas_->find(module_identifier); - if (found_value != s_future_datas_->end()) { + MutexLock lock(FuturesMutex()); + auto& future_datas = FutureDatas(); + auto found_value = future_datas.find(module_identifier); + if (found_value != future_datas.end()) { StaticFutureData* existing_data = found_value->second; if (existing_data != nullptr) { return existing_data; @@ -295,7 +306,7 @@ StaticFutureData* StaticFutureData::GetFutureDataForModule( } StaticFutureData* new_data = CreateNewData(module_identifier, num_functions); - (*s_future_datas_)[module_identifier] = new_data; + future_datas[module_identifier] = new_data; return new_data; } diff --git a/app/src/util.h b/app/src/util.h index f25817a209..2fe9abe25c 100644 --- a/app/src/util.h +++ b/app/src/util.h @@ -55,7 +55,9 @@ namespace firebase { #else // !FIREBASE_PLATFORM_ANDROID // No-ops on other platforms. Consume the variables to prevent warnings. #define FIREBASE_UTIL_RETURN_FAILURE_IF_GOOGLE_PLAY_UNAVAILABLE(x) \ - { (void)(x); } + { \ + (void)(x); \ + } #define FIREBASE_UTIL_RETURN_NULL_IF_GOOGLE_PLAY_UNAVAILABLE(x, y) \ { \ (void)(x); \ @@ -146,9 +148,11 @@ class AppCallback { if (destroyed_) destroyed_(app); } - // Add a callback class to the list. - // NOTE: This is *not* synchronized as it's not possible to use Mutex - // from static constructors on some platforms. + // Adds a callback during FIREBASE_APP_REGISTER_CALLBACKS static + // initialization. Intentionally leaves CallbacksMutex() unlocked because + // registration runs during single-threaded dynamic initialization before + // main() and avoids constructing platform Mutex primitives in early static + // constructors on platforms with restricted pre-main runtime state. static void AddCallback(AppCallback* callback); private: @@ -160,13 +164,6 @@ class AppCallback { Destroyed destroyed_; // Whether this callback is enabled. bool enabled_; - - // Global set of callbacks. - static std::map* callbacks_; - // Mutex which controls access to callbacks during notification. - // NOTE: This violates Google's C++ style guide as we do not have access to - // module initializers in //base so this replicates part of that behavior. - static Mutex* callbacks_mutex_; }; // Register app callbacks for a module. @@ -223,13 +220,6 @@ class StaticFutureData { private: ReferenceCountedFutureImpl api_; - // Mutex which controls access to s_future_datas_ - static Mutex* s_futures_mutex_; - - // Static storage of StaticFutureDatas per module. Keyed on a void pointer - // that should be declared in the module to serve as a unique identifier. - static std::map* s_future_datas_; - static StaticFutureData* CreateNewData(const void* module_identifier, int num_functions); }; diff --git a/app/tests/CMakeLists.txt b/app/tests/CMakeLists.txt index 489e8c1da5..88f98fba4e 100644 --- a/app/tests/CMakeLists.txt +++ b/app/tests/CMakeLists.txt @@ -168,6 +168,14 @@ firebase_cpp_cc_test(firebase_app_no_destructor_test firebase_app ) +firebase_cpp_cc_test(firebase_app_static_destruction_test + SOURCES + static_destruction_test.cc + DEPENDS + firebase_app + flatbuffers +) + firebase_cpp_cc_test(firebase_app_cpp11_thread_test SOURCES thread_test.cc diff --git a/app/tests/future_test.cc b/app/tests/future_test.cc index c9b98d0ff5..d9f1ece7f8 100644 --- a/app/tests/future_test.cc +++ b/app/tests/future_test.cc @@ -951,14 +951,14 @@ TEST_F(FutureTest, VerifyNotLeakedWhenOverridden) { SafeFutureHandle last_result_handle; last_result_handle = future_impl_.SafeAlloc(0); EXPECT_THAT(last_result_handle.get(), - Ne(SafeFutureHandle::kInvalidHandle.get())); + Ne(SafeFutureHandle::InvalidHandle().get())); EXPECT_TRUE(future_impl_.ValidFuture(last_result_handle)); id = last_result_handle.get().id(); } { auto new_last_result_handle = future_impl_.SafeAlloc(0); EXPECT_THAT(new_last_result_handle.get(), - Ne(SafeFutureHandle::kInvalidHandle.get())); + Ne(SafeFutureHandle::InvalidHandle().get())); EXPECT_FALSE(future_impl_.ValidFuture(id)); } } diff --git a/app/tests/no_destructor_test.cc b/app/tests/no_destructor_test.cc index 7b729086e9..15e3b794a2 100644 --- a/app/tests/no_destructor_test.cc +++ b/app/tests/no_destructor_test.cc @@ -16,9 +16,13 @@ #include "app/src/no_destructor.h" +#include #include #include +#include +#include +#include "app/src/include/firebase/variant.h" #include "gtest/gtest.h" namespace firebase { @@ -36,6 +40,42 @@ struct Tracked { int Tracked::constructed = 0; int Tracked::destroyed = 0; +struct alignas(64) OverAligned { + explicit OverAligned(uint64_t seed_in) : seed(seed_in) { ++constructed; } + ~OverAligned() { ++destroyed; } + + uint64_t seed; + static int constructed; + static int destroyed; +}; +int OverAligned::constructed = 0; +int OverAligned::destroyed = 0; + +// NoDestructor must be trivially destructible so function-local statics do +// not register __cxa_atexit / atexit handlers, and must remain non-copyable and +// non-movable even when T has broad converting constructors (such as Variant). +static_assert(std::is_trivially_destructible>::value, + "NoDestructor must be trivially destructible."); +static_assert(!std::is_copy_constructible>::value, + "NoDestructor must not be copy constructible."); +static_assert(!std::is_move_constructible>::value, + "NoDestructor must not be move constructible."); +static_assert(!std::is_copy_assignable>::value, + "NoDestructor must not be copy assignable."); +static_assert(!std::is_move_assignable>::value, + "NoDestructor must not be move assignable."); +static_assert( + !std::is_constructible, + NoDestructor&>::value, + "Non-const lvalue NoDestructor must not hijack forwarding ctor."); +static_assert( + !std::is_constructible, + const NoDestructor&>::value, + "Const lvalue NoDestructor must not be copy constructible."); +static_assert(!std::is_constructible, + NoDestructor&&>::value, + "Rvalue NoDestructor must not be move constructible."); + TEST(NoDestructorTest, ForwardsConstructorArguments) { NoDestructor str("hello"); EXPECT_EQ(*str, "hello"); @@ -50,6 +90,14 @@ TEST(NoDestructorTest, DefaultConstructs) { EXPECT_EQ(map->at(1), 2); } +TEST(NoDestructorTest, BraceEnclosedInitializerList) { + NoDestructor> values({10, 20, 30}); + ASSERT_EQ(values->size(), 3u); + EXPECT_EQ((*values)[0], 10); + EXPECT_EQ((*values)[1], 20); + EXPECT_EQ((*values)[2], 30); +} + TEST(NoDestructorTest, DoesNotRunDestructorWhenGoingOutOfScope) { Tracked::constructed = 0; Tracked::destroyed = 0; @@ -62,6 +110,21 @@ TEST(NoDestructorTest, DoesNotRunDestructorWhenGoingOutOfScope) { EXPECT_EQ(Tracked::destroyed, 0); } +TEST(NoDestructorTest, PreservesOverAlignedStorage) { + OverAligned::constructed = 0; + OverAligned::destroyed = 0; + { + NoDestructor aligned(0x12345678ULL); + EXPECT_EQ(reinterpret_cast(aligned.get()) % alignof(OverAligned), + 0u); + EXPECT_EQ(aligned->seed, 0x12345678ULL); + const NoDestructor& const_aligned = aligned; + EXPECT_EQ(const_aligned->seed, 0x12345678ULL); + } + EXPECT_EQ(OverAligned::constructed, 1); + EXPECT_EQ(OverAligned::destroyed, 0); +} + TEST(NoDestructorTest, ConstAccess) { const NoDestructor str("const"); EXPECT_EQ(*str, "const"); diff --git a/app/tests/static_destruction_test.cc b/app/tests/static_destruction_test.cc new file mode 100644 index 0000000000..5e755e7075 --- /dev/null +++ b/app/tests/static_destruction_test.cc @@ -0,0 +1,263 @@ +/* + * Copyright 2026 Google LLC + * + * 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 +#include +#include +#include + +#include "app/src/callback.h" +#include "app/src/cleanup_notifier.h" +#include "app/src/include/firebase/app.h" +#include "app/src/include/firebase/future.h" +#include "app/src/log.h" +#include "app/src/reference_counted_future_impl.h" +#include "app/src/util.h" +#include "gtest/gtest.h" + +namespace firebase { +namespace { + +// GoogleTest has already computed its exit status by the time static +// destructors run after main() returns, so post-main checks must abort directly +// on failure to fail CTest reliably. +void AbortIfFalse(bool condition, const char* description) { + if (!condition) { + std::fprintf(stderr, "StaticDestructionTest post-main check failed: %s\n", + description); + std::fflush(stderr); + std::abort(); + } +} + +AppOptions MakeTestAppOptions() { + AppOptions options; + options.set_app_id("1:1234567890:android:abcdef123456"); + options.set_api_key("not_a_real_api_key"); + options.set_project_id("static-destruction-test-project"); + options.set_database_url( + "https://static-destruction-test-project.firebaseio.com"); + options.set_storage_bucket("static-destruction-test-project.appspot.com"); + return options; +} + +// Simulates a Firebase product instance registered with an App's +// CleanupNotifier so both normal teardown (subscriber before App) and +// out-of-order teardown (App before subscriber) can be exercised. +class ScopedCleanupSubscriber { + public: + explicit ScopedCleanupSubscriber(App* app) : app_(app) { + CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app_); + AbortIfFalse(notifier != nullptr, + "CleanupNotifier::FindByOwner(app) returned nullptr"); + notifier->RegisterObject(this, &ScopedCleanupSubscriber::OnOwnerCleanedUp); + } + + ~ScopedCleanupSubscriber() { Cleanup(); } + + bool cleaned_up() const { return cleaned_up_; } + + private: + static void OnOwnerCleanedUp(void* obj) { + static_cast(obj)->Cleanup(); + } + + void Cleanup() { + if (cleaned_up_) return; + cleaned_up_ = true; + CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app_); + if (notifier != nullptr) { + notifier->UnregisterObject(this); + } + app_ = nullptr; + } + + App* app_; + bool cleaned_up_ = false; +}; + +int g_post_main_callback_runs = 0; +bool g_exit_time_holder_destroyed = false; + +void RecordPostMainCallback() { ++g_post_main_callback_runs; } + +// Defined BEFORE g_exit_time_sdk_holder in this translation unit so C++ +// constructs it first and destroys it AFTER ~ExitTimeSdkHolder() runs during +// process exit ([basic.start.term]). Verifies that all NoDestructor-backed SDK +// registries, mutexes, callbacks, futures, and loggers remain usable even for +// the final static destructor in the binary. +struct FinalPostMainVerifier { + bool armed = false; + + ~FinalPostMainVerifier() { + if (!armed) return; + + AbortIfFalse(g_exit_time_holder_destroyed, + "ExitTimeSdkHolder must be destroyed before " + "FinalPostMainVerifier"); + AbortIfFalse(App::GetApps().empty(), + "All Apps should be unregistered after ExitTimeSdkHolder " + "teardown"); + AbortIfFalse(App::GetInstance() == nullptr, + "Default App should be nullptr after ExitTimeSdkHolder " + "teardown"); + AbortIfFalse( + CleanupNotifier::FindByOwner(reinterpret_cast(0x1)) == nullptr, + "CleanupNotifier registry lookup failed during late exit"); + AbortIfFalse(ReferenceCountedFutureImpl::InvalidHandle().id() == + kInvalidFutureHandle, + "ReferenceCountedFutureImpl::InvalidHandle() corrupted"); + AbortIfFalse(SafeFutureHandle::InvalidHandle().get().id() == + ReferenceCountedFutureImpl::InvalidHandle().id(), + "SafeFutureHandle::InvalidHandle() mismatch"); + + const int before = g_post_main_callback_runs; + callback::AddCallback(new callback::CallbackVoid(&RecordPostMainCallback)); + callback::PollCallbacks(); + AbortIfFalse(g_post_main_callback_runs == before + 1, + "callback::PollCallbacks() did not run queued callback during " + "process exit"); + + LogInfo("StaticDestructionTest: post-main static teardown verified."); + } +}; + +// Defined AFTER g_final_post_main_verifier so its destructor runs during real +// __cxa_atexit / atexit processing after main() returns, tearing down live App +// instances, CleanupNotifier subscribers (in both orders), and Future backing +// state. +struct ExitTimeSdkHolder { + bool armed = false; + std::unique_ptr default_app; + std::unique_ptr subscriber_deleted_before_app; + std::unique_ptr secondary_app; + std::unique_ptr subscriber_outliving_app; + std::unique_ptr future_impl; + Future exit_future; + SafeFutureHandle exit_future_handle; + + ~ExitTimeSdkHolder() { + if (!armed) return; + + // 1. Complete and release a live Future during static destruction. + AbortIfFalse(future_impl != nullptr, "future_impl must be initialized"); + future_impl->CompleteWithResult(exit_future_handle, 0, "", 77); + AbortIfFalse(exit_future.status() == kFutureStatusComplete, + "exit_future did not complete during static destruction"); + AbortIfFalse(exit_future.result() != nullptr && *exit_future.result() == 77, + "exit_future result mismatch during static destruction"); + exit_future.Release(); + future_impl.reset(); + + // 2. Normal destruction order during exit(): subscriber before its App. + subscriber_deleted_before_app.reset(); + default_app.reset(); + + // 3. Reverse destruction order during exit(): App destroyed while its + // dependent subscriber still exists (the exact pattern triggered when a + // user's static App is destroyed before a static product wrapper). + secondary_app.reset(); + AbortIfFalse(subscriber_outliving_app != nullptr && + subscriber_outliving_app->cleaned_up(), + "CleanupNotifier failed to clean up subscriber when App was " + "destroyed during process exit"); + subscriber_outliving_app.reset(); + + g_exit_time_holder_destroyed = true; + } +}; + +FinalPostMainVerifier g_final_post_main_verifier; +ExitTimeSdkHolder g_exit_time_sdk_holder; + +int g_in_test_callback_runs = 0; +void RecordInTestCallback() { ++g_in_test_callback_runs; } + +TEST(StaticDestructionTest, OutOfOrderCleanupAndRepeatedAppReinitialization) { + const AppOptions options = MakeTestAppOptions(); + + // Exercise repeated create -> destroy -> recreate cycles in a single process + // (for example, plugin/domain reloads without restarting the host process). + for (int cycle = 0; cycle < 3; ++cycle) { + std::unique_ptr default_app(App::Create(options)); + ASSERT_NE(default_app, nullptr); + EXPECT_EQ(App::GetInstance(), default_app.get()); + + const std::string secondary_name = + "cycle_secondary_" + std::to_string(cycle); + std::unique_ptr secondary_app( + App::Create(options, secondary_name.c_str())); + ASSERT_NE(secondary_app, nullptr); + EXPECT_EQ(App::GetInstance(secondary_name.c_str()), secondary_app.get()); + EXPECT_EQ(App::GetApps().size(), 2u); + + auto subscriber_first = + std::make_unique(default_app.get()); + auto subscriber_late = + std::make_unique(secondary_app.get()); + + const int callbacks_before = g_in_test_callback_runs; + callback::AddCallback(new callback::CallbackVoid(&RecordInTestCallback)); + callback::PollCallbacks(); + EXPECT_EQ(g_in_test_callback_runs, callbacks_before + 1); + + // Normal order: subscriber destroyed before App. + subscriber_first.reset(); + default_app.reset(); + EXPECT_EQ(App::GetInstance(), nullptr); + + // Out-of-order: App destroyed before subscriber. + void* destroyed_app_address = secondary_app.get(); + secondary_app.reset(); + EXPECT_TRUE(subscriber_late->cleaned_up()); + EXPECT_EQ(CleanupNotifier::FindByOwner(destroyed_app_address), nullptr); + subscriber_late.reset(); + + EXPECT_TRUE(App::GetApps().empty()); + } +} + +TEST(StaticDestructionTest, SurvivesPostMainStaticDestruction) { + const AppOptions options = MakeTestAppOptions(); + + g_exit_time_sdk_holder.default_app.reset(App::Create(options)); + ASSERT_NE(g_exit_time_sdk_holder.default_app, nullptr); + g_exit_time_sdk_holder.subscriber_deleted_before_app = + std::make_unique( + g_exit_time_sdk_holder.default_app.get()); + + g_exit_time_sdk_holder.secondary_app.reset( + App::Create(options, "post_main_secondary_app")); + ASSERT_NE(g_exit_time_sdk_holder.secondary_app, nullptr); + g_exit_time_sdk_holder.subscriber_outliving_app = + std::make_unique( + g_exit_time_sdk_holder.secondary_app.get()); + + g_exit_time_sdk_holder.future_impl = + std::make_unique(1); + g_exit_time_sdk_holder.exit_future_handle = + g_exit_time_sdk_holder.future_impl->SafeAlloc(0); + g_exit_time_sdk_holder.exit_future = + Future(g_exit_time_sdk_holder.future_impl.get(), + g_exit_time_sdk_holder.exit_future_handle.get()); + EXPECT_EQ(g_exit_time_sdk_holder.exit_future.status(), kFutureStatusPending); + + g_exit_time_sdk_holder.armed = true; + g_final_post_main_verifier.armed = true; +} + +} // namespace +} // namespace firebase diff --git a/app_check/CMakeLists.txt b/app_check/CMakeLists.txt index f27cc4a870..723cab4c71 100644 --- a/app_check/CMakeLists.txt +++ b/app_check/CMakeLists.txt @@ -162,6 +162,12 @@ target_compile_definitions(firebase_app_check -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_app_check PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(app_check) elseif(IOS) diff --git a/app_check/src/android/play_integrity_provider_android.cc b/app_check/src/android/play_integrity_provider_android.cc index 94b669176b..2b27fe97a2 100644 --- a/app_check/src/android/play_integrity_provider_android.cc +++ b/app_check/src/android/play_integrity_provider_android.cc @@ -121,8 +121,12 @@ AppCheckProvider* PlayIntegrityProviderFactoryInternal::CreateProvider( } // namespace internal PlayIntegrityProviderFactory* PlayIntegrityProviderFactory::GetInstance() { - static PlayIntegrityProviderFactory g_play_integrity_provider_factory; - return &g_play_integrity_provider_factory; + // Heap-allocated function-local singleton (PlayIntegrityProviderFactory has a + // private constructor) so the factory is never destroyed during exit() while + // AppCheckInternal still references it or its created providers. + static PlayIntegrityProviderFactory* g_play_integrity_provider_factory = + new PlayIntegrityProviderFactory(); + return g_play_integrity_provider_factory; } PlayIntegrityProviderFactory::PlayIntegrityProviderFactory() diff --git a/app_check/src/common/app_check.cc b/app_check/src/common/app_check.cc index 2c4bc06fb1..d57c958f98 100644 --- a/app_check/src/common/app_check.cc +++ b/app_check/src/common/app_check.cc @@ -62,17 +62,23 @@ namespace app_check { DEFINE_FIREBASE_VERSION_STRING(FirebaseAppCheck); -// A function-local static wrapped in NoDestructor, so it is constructed on -// first use and never destroyed. That keeps it out of static initialization -// and destruction entirely: it is ready no matter which translation unit's -// initializers run first, and an AppCheck owned by a static object can still -// lock it from its destructor after this file's globals would otherwise have -// been torn down by the C++ runtime. +using AppCheckMap = std::map<::firebase::App*, AppCheck*>; + +// The map and its mutex are function-local statics wrapped in NoDestructor, so +// they are constructed on first use and never destroyed. That keeps them out +// of static initialization and destruction entirely: they are ready no matter +// which translation unit's initializers run first, and an AppCheck owned by a +// static object can still lock the mutex and erase itself from its destructor +// during process exit. static Mutex& AppCheckLock() { static NoDestructor mutex; return *mutex; } -static std::map<::firebase::App*, AppCheck*>* g_app_check_map = nullptr; + +static AppCheckMap& AppChecks() { + static NoDestructor app_checks; + return *app_checks; +} // Define the destructors for the virtual listener/provider/factory classes. AppCheckListener::~AppCheckListener() {} @@ -97,11 +103,10 @@ AppCheck* GetExistingAppCheckInstance(::firebase::App* app) { if (!app) return nullptr; MutexLock lock(AppCheckLock()); - if (g_app_check_map) { - auto it = g_app_check_map->find(app); - if (it != g_app_check_map->end()) { - return it->second; - } + AppCheckMap& app_checks = AppChecks(); + auto it = app_checks.find(app); + if (it != app_checks.end()) { + return it->second; } return nullptr; } @@ -112,17 +117,14 @@ AppCheck* AppCheck::GetInstance(::firebase::App* app) { if (!app) return nullptr; MutexLock lock(AppCheckLock()); - if (!g_app_check_map) { - g_app_check_map = new std::map<::firebase::App*, AppCheck*>(); - } - - auto it = g_app_check_map->find(app); - if (it != g_app_check_map->end()) { + AppCheckMap& app_checks = AppChecks(); + auto it = app_checks.find(app); + if (it != app_checks.end()) { return it->second; } AppCheck* app_check = new AppCheck(app); - g_app_check_map->insert(std::make_pair(app, app_check)); + app_checks.insert(std::make_pair(app, app_check)); return app_check; } @@ -141,15 +143,10 @@ void AppCheck::DeleteInternal() { if (!internal_) return; - // Remove it from the map - g_app_check_map->erase(app()); + // Remove it from the map. + AppChecks().erase(app()); delete internal_; internal_ = nullptr; - // If it is the last one, delete the map. - if (g_app_check_map->empty()) { - delete g_app_check_map; - g_app_check_map = nullptr; - } } ::firebase::App* AppCheck::app() { diff --git a/app_check/src/common/debug_provider_common.cc b/app_check/src/common/debug_provider_common.cc index e01aba203a..af2353eca9 100644 --- a/app_check/src/common/debug_provider_common.cc +++ b/app_check/src/common/debug_provider_common.cc @@ -30,8 +30,12 @@ namespace firebase { namespace app_check { DebugAppCheckProviderFactory* DebugAppCheckProviderFactory::GetInstance() { - static DebugAppCheckProviderFactory g_debug_app_check_provider_factory; - return &g_debug_app_check_provider_factory; + // Heap-allocated function-local singleton (DebugAppCheckProviderFactory has a + // private constructor) so the factory is never destroyed during exit() while + // AppCheckInternal still references it or its created providers. + static DebugAppCheckProviderFactory* g_debug_app_check_provider_factory = + new DebugAppCheckProviderFactory(); + return g_debug_app_check_provider_factory; } DebugAppCheckProviderFactory::DebugAppCheckProviderFactory() diff --git a/app_check/src/desktop/debug_provider_desktop.cc b/app_check/src/desktop/debug_provider_desktop.cc index b9ce99c6a0..15503eddf7 100644 --- a/app_check/src/desktop/debug_provider_desktop.cc +++ b/app_check/src/desktop/debug_provider_desktop.cc @@ -28,6 +28,7 @@ #include "app/rest/transport_curl.h" #include "app/rest/util.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/scheduler.h" #include "app/src/uuid.h" #include "app_check/src/desktop/debug_token_request.h" @@ -135,12 +136,12 @@ void DebugAppCheckProvider::GetTokenInternal( bool limited_use, std::function completion_callback) { - static std::mutex token_mutex; + static NoDestructor token_mutex; static bool logged_token = false; std::string message_to_log; { - std::lock_guard lock(token_mutex); + std::lock_guard lock(*token_mutex); if (debug_token_.empty()) { if (const char* env_token = std::getenv("APP_CHECK_DEBUG_TOKEN")) { debug_token_ = env_token; diff --git a/app_check/src/ios/app_attest_provider_ios.mm b/app_check/src/ios/app_attest_provider_ios.mm index 222fb01777..64a9251e76 100644 --- a/app_check/src/ios/app_attest_provider_ios.mm +++ b/app_check/src/ios/app_attest_provider_ios.mm @@ -99,8 +99,11 @@ virtual void GetLimitedUseToken( } // namespace internal AppAttestProviderFactory* AppAttestProviderFactory::GetInstance() { - static AppAttestProviderFactory g_app_attest_provider_factory; - return &g_app_attest_provider_factory; + // Heap-allocated function-local singleton (AppAttestProviderFactory has a + // private constructor) so the factory is never destroyed during exit() while + // AppCheckInternal still references it or its created providers. + static AppAttestProviderFactory* g_app_attest_provider_factory = new AppAttestProviderFactory(); + return g_app_attest_provider_factory; } AppAttestProviderFactory::AppAttestProviderFactory() diff --git a/app_check/src/ios/device_check_provider_ios.mm b/app_check/src/ios/device_check_provider_ios.mm index 3f660fb639..877bc61541 100644 --- a/app_check/src/ios/device_check_provider_ios.mm +++ b/app_check/src/ios/device_check_provider_ios.mm @@ -103,8 +103,12 @@ virtual void GetLimitedUseToken( } // namespace internal DeviceCheckProviderFactory* DeviceCheckProviderFactory::GetInstance() { - static DeviceCheckProviderFactory g_device_check_check_provider_factory; - return &g_device_check_check_provider_factory; + // Heap-allocated function-local singleton (DeviceCheckProviderFactory has a + // private constructor) so the factory is never destroyed during exit() while + // AppCheckInternal still references it or its created providers. + static DeviceCheckProviderFactory* g_device_check_provider_factory = + new DeviceCheckProviderFactory(); + return g_device_check_provider_factory; } DeviceCheckProviderFactory::DeviceCheckProviderFactory() diff --git a/auth/CMakeLists.txt b/auth/CMakeLists.txt index 93055b0edd..87d0f9a143 100644 --- a/auth/CMakeLists.txt +++ b/auth/CMakeLists.txt @@ -184,6 +184,12 @@ target_compile_definitions(firebase_auth -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_auth PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(auth) elseif(IOS) diff --git a/auth/src/auth.cc b/auth/src/auth.cc index 10e82f5718..e6b8d759e5 100644 --- a/auth/src/auth.cc +++ b/auth/src/auth.cc @@ -172,15 +172,12 @@ void Auth::DeleteInternal() { int num_auths_remaining = 0; { - // Remove `this` from the Auths map. - // The mapping is 1:1, so we should only ever delete one. + // Remove `this` from the Auths map by its owning App key. AuthMap& auths = Auths(); - for (auto it = auths.begin(); it != auths.end(); ++it) { - if (it->second == this) { - LogDebug("Deleting Auth %p for App %p", this, it->first); - auths.erase(it); - break; - } + auto it = auths.find(auth_data_->app); + if (it != auths.end()) { + LogDebug("Deleting Auth %p for App %p", this, it->first); + auths.erase(it); } num_auths_remaining = auths.size(); diff --git a/auth/src/desktop/rpcs/get_account_info_response.h b/auth/src/desktop/rpcs/get_account_info_response.h index eb9b5d80bc..137f3f6e67 100644 --- a/auth/src/desktop/rpcs/get_account_info_response.h +++ b/auth/src/desktop/rpcs/get_account_info_response.h @@ -23,6 +23,7 @@ #include #include +#include "app/src/no_destructor.h" #include "auth/src/desktop/rpcs/auth_response.h" #include "auth/src/desktop/visual_studio_compatibility.h" @@ -92,9 +93,10 @@ class GetAccountInfoResponse : public AuthResponse { if (!application_data_->users.empty()) { return application_data_->users[0]->providerUserInfo; } - static std::vector> + static const NoDestructor< + std::vector>> fallback; - return fallback; + return *fallback; } }; diff --git a/database/CMakeLists.txt b/database/CMakeLists.txt index 1df98b1f3e..83f48f6c0d 100644 --- a/database/CMakeLists.txt +++ b/database/CMakeLists.txt @@ -217,6 +217,12 @@ target_compile_definitions(firebase_database ${additional_DEFINES} ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_database PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(database) elseif(IOS) diff --git a/database/src/android/database_android.cc b/database/src/android/database_android.cc index ee5d72051a..3380e1157c 100644 --- a/database/src/android/database_android.cc +++ b/database/src/android/database_android.cc @@ -23,6 +23,7 @@ #include "app/src/include/firebase/future.h" #include "app/src/include/firebase/internal/common.h" #include "app/src/include/firebase/log.h" +#include "app/src/no_destructor.h" #include "app/src/reference_counted_future_impl.h" #include "app/src/util.h" #include "app/src/util_android.h" @@ -39,8 +40,6 @@ namespace firebase { namespace database { namespace internal { -Mutex g_database_reference_constructor_mutex; // NOLINT - const char kApiIdentifier[] = "Database"; // clang-format off @@ -164,7 +163,10 @@ METHOD_LOOKUP_DEFINITION( "com/google/firebase/database/internal/cpp/CppChildEventListener", CPP_CHILD_EVENT_LISTENER_METHODS) -Mutex DatabaseInternal::init_mutex_; // NOLINT +static Mutex& InitMutex() { + static NoDestructor init_mutex; + return *init_mutex; +} int DatabaseInternal::initialize_count_ = 0; std::map* DatabaseInternal::java_error_to_cpp_ = nullptr; @@ -253,7 +255,7 @@ const char* kCppLogLevelToLoggerLevelName[] = { }; bool DatabaseInternal::Initialize(App* app) { - MutexLock init_lock(init_mutex_); + MutexLock init_lock(InitMutex()); if (initialize_count_ == 0) { JNIEnv* env = app->GetJNIEnv(); jobject activity = app->activity(); @@ -385,7 +387,7 @@ void DatabaseInternal::ReleaseClasses(App* app) { } void DatabaseInternal::Terminate(App* app) { - MutexLock init_lock(init_mutex_); + MutexLock init_lock(InitMutex()); assert(initialize_count_ > 0); initialize_count_--; if (initialize_count_ == 0) { diff --git a/database/src/android/database_android.h b/database/src/android/database_android.h index 5056e41ed4..ad260e52d8 100644 --- a/database/src/android/database_android.h +++ b/database/src/android/database_android.h @@ -41,7 +41,7 @@ namespace database { namespace internal { // For constructing, copying or moving DatabaseReferences atomically. -extern Mutex g_database_reference_constructor_mutex; +Mutex& DatabaseReferenceConstructorMutex(); // This is the Android implementation of Database. class DatabaseInternal { @@ -192,7 +192,6 @@ class DatabaseInternal { // Initialize classes loaded from embedded files. static bool InitializeEmbeddedClasses(App* app); - static Mutex init_mutex_; static int initialize_count_; static std::map* java_error_to_cpp_; App* app_; diff --git a/database/src/common/common.cc b/database/src/common/common.cc index dd2daa212c..e52d054ce4 100644 --- a/database/src/common/common.cc +++ b/database/src/common/common.cc @@ -15,6 +15,7 @@ #include "database/src/include/firebase/database/common.h" #include "app/src/include/firebase/internal/common.h" +#include "app/src/no_destructor.h" namespace firebase { namespace database { @@ -57,16 +58,13 @@ const char* GetErrorMessage(Error error) { : ""; } -static const Variant* g_server_value_timestamp = nullptr; const Variant& ServerTimestamp() { - if (g_server_value_timestamp == nullptr) { - // The Firebase server defines a ServerValue for Timestamp as a map with the - // key ".sv" and the value "timestamp". - std::map server_value; - server_value.insert(std::make_pair(".sv", "timestamp")); - g_server_value_timestamp = new Variant(server_value); - } - return *g_server_value_timestamp; + // The Firebase server defines a ServerValue for Timestamp as a map with the + // key ".sv" and the value "timestamp". Function-local NoDestructor ensures + // thread-safe one-time initialization with no exit-time destructor. + static const NoDestructor server_value_timestamp( + std::map{std::make_pair(".sv", "timestamp")}); + return *server_value_timestamp; } } // namespace database diff --git a/database/src/common/database.cc b/database/src/common/database.cc index 5fe1b01548..9677ec3967 100644 --- a/database/src/common/database.cc +++ b/database/src/common/database.cc @@ -57,17 +57,29 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseDatabase); using internal::DatabaseInternal; using DatabaseMap = std::map, Database*>; -// A function-local static wrapped in NoDestructor, so it is constructed on -// first use and never destroyed. That keeps it out of static initialization -// and destruction entirely: it is ready no matter which translation unit's -// initializers run first, and a Database owned by a static object can still -// lock it from its destructor after this file's globals would otherwise have -// been torn down by the C++ runtime. + +// The map and its mutex are function-local statics wrapped in NoDestructor, so +// they are constructed on first use and never destroyed. That keeps them out +// of static initialization and destruction entirely: they are ready no matter +// which translation unit's initializers run first, and a Database owned by a +// static object can still lock the mutex and erase itself from its destructor +// during process exit. static Mutex& DatabasesLock() { static NoDestructor mutex; return *mutex; } -static DatabaseMap* g_databases = nullptr; + +static DatabaseMap& Databases() { + static NoDestructor databases; + return *databases; +} + +namespace internal { +Mutex& DatabaseReferenceConstructorMutex() { + static NoDestructor mutex; + return *mutex; +} +} // namespace internal namespace { DatabaseMap::key_type MakeKey(App* app, const std::string& url) { @@ -82,13 +94,11 @@ Database* Database::GetInstance(App* app, const char* url, return nullptr; } MutexLock lock(DatabasesLock()); - if (!g_databases) { - g_databases = new DatabaseMap(); - } + DatabaseMap& databases = Databases(); DatabaseMap::key_type key = MakeKey(app, std::string(url ? url : "")); - DatabaseMap::iterator it = g_databases->find(key); - if (it != g_databases->end()) { + DatabaseMap::iterator it = databases.find(key); + if (it != databases.end()) { if (init_result_out != nullptr) *init_result_out = kInitResultSuccess; return it->second; } @@ -100,7 +110,7 @@ Database* Database::GetInstance(App* app, const char* url, delete database; return nullptr; } - g_databases->insert(std::make_pair(key, database)); + databases.insert(std::make_pair(key, database)); if (init_result_out) *init_result_out = kInitResultSuccess; return database; } @@ -133,7 +143,7 @@ void Database::DeleteInternal() { if (!internal_) return; // An instance whose initialization failed was never registered with the - // App's CleanupNotifier or added to g_databases, and app() may be null. + // App's CleanupNotifier or added to Databases(), and app() may be null. if (!internal_->initialized()) { delete internal_; internal_ = nullptr; @@ -148,19 +158,14 @@ void Database::DeleteInternal() { app_notifier->UnregisterObject(this); { - MutexLock db_ref_lock(internal::g_database_reference_constructor_mutex); + MutexLock db_ref_lock(internal::DatabaseReferenceConstructorMutex()); // Force cleanup to happen first. internal_->cleanup().CleanupAll(); } delete internal_; internal_ = nullptr; // If a Database is explicitly deleted, remove it from our cache. - g_databases->erase(MakeKey(my_app, url)); - // If it's the last one, delete the map. - if (g_databases->empty()) { - delete g_databases; - g_databases = nullptr; - } + Databases().erase(MakeKey(my_app, url)); } Database::~Database() { DeleteInternal(); } diff --git a/database/src/common/database_reference.cc b/database/src/common/database_reference.cc index c1617e902e..62920b332d 100644 --- a/database/src/common/database_reference.cc +++ b/database/src/common/database_reference.cc @@ -80,14 +80,14 @@ void DatabaseReference::SwitchCleanupRegistrationBackToQuery() { DatabaseReference::DatabaseReference(DatabaseReferenceInternal* internal) : Query(internal), internal_(internal) { - MutexLock lock(internal::g_database_reference_constructor_mutex); + MutexLock lock(internal::DatabaseReferenceConstructorMutex()); SwitchCleanupRegistrationToDatabaseReference(); } DatabaseReference::DatabaseReference(const DatabaseReference& reference) : Query(), internal_(nullptr) { - MutexLock lock(internal::g_database_reference_constructor_mutex); + MutexLock lock(internal::DatabaseReferenceConstructorMutex()); internal_ = reference.internal_ ? new DatabaseReferenceInternal(*reference.internal_) @@ -98,7 +98,7 @@ DatabaseReference::DatabaseReference(const DatabaseReference& reference) DatabaseReference& DatabaseReference::operator=( const DatabaseReference& reference) { - MutexLock lock(internal::g_database_reference_constructor_mutex); + MutexLock lock(internal::DatabaseReferenceConstructorMutex()); internal_ = reference.internal_ ? new DatabaseReferenceInternal(*reference.internal_) @@ -112,7 +112,7 @@ DatabaseReference& DatabaseReference::operator=( #if defined(FIREBASE_USE_MOVE_OPERATORS) || defined(DOXYGEN) DatabaseReference::DatabaseReference(DatabaseReference&& reference) : Query(), internal_(reference.internal_) { - MutexLock lock(internal::g_database_reference_constructor_mutex); + MutexLock lock(internal::DatabaseReferenceConstructorMutex()); reference.internal_ = nullptr; Query::operator=(std::move(reference)); @@ -120,7 +120,7 @@ DatabaseReference::DatabaseReference(DatabaseReference&& reference) } DatabaseReference& DatabaseReference::operator=(DatabaseReference&& reference) { - MutexLock lock(internal::g_database_reference_constructor_mutex); + MutexLock lock(internal::DatabaseReferenceConstructorMutex()); internal_ = reference.internal_; reference.internal_ = nullptr; diff --git a/database/src/desktop/core/operation.cc b/database/src/desktop/core/operation.cc index 70923c62c6..28307938ab 100644 --- a/database/src/desktop/core/operation.cc +++ b/database/src/desktop/core/operation.cc @@ -17,6 +17,7 @@ #include #include "app/src/assert.h" +#include "app/src/no_destructor.h" #include "app/src/path.h" #include "database/src/desktop/util_desktop.h" @@ -24,10 +25,17 @@ namespace firebase { namespace database { namespace internal { -const OperationSource OperationSource::kUser(OperationSource::kSourceUser, - Optional(), false); -const OperationSource OperationSource::kServer(OperationSource::kSourceServer, - Optional(), false); +const OperationSource& OperationSource::User() { + static const NoDestructor user( + OperationSource::kSourceUser, Optional(), false); + return *user; +} + +const OperationSource& OperationSource::Server() { + static const NoDestructor server( + OperationSource::kSourceServer, Optional(), false); + return *server; +} OperationSource OperationSource::ForServerTaggedQuery( const QueryParams& params) { @@ -50,7 +58,7 @@ Operation Operation::Merge(const OperationSource& source, const Path& path, Operation Operation::AckUserWrite(const Path& path, const Tree& affected_tree, AckStatus status) { - return Operation(Operation::kTypeAckUserWrite, OperationSource::kUser, path, + return Operation(Operation::kTypeAckUserWrite, OperationSource::User(), path, Variant(), CompoundWrite(), affected_tree, status); } diff --git a/database/src/desktop/core/operation.h b/database/src/desktop/core/operation.h index 8ed5d521f3..a7367535a9 100644 --- a/database/src/desktop/core/operation.h +++ b/database/src/desktop/core/operation.h @@ -56,8 +56,8 @@ struct OperationSource { static OperationSource ForServerTaggedQuery(const QueryParams& params); - static const OperationSource kUser; - static const OperationSource kServer; + static const OperationSource& User(); + static const OperationSource& Server(); }; enum AckStatus { diff --git a/database/src/desktop/core/repo.cc b/database/src/desktop/core/repo.cc index 31598208c9..15b4107340 100644 --- a/database/src/desktop/core/repo.cc +++ b/database/src/desktop/core/repo.cc @@ -24,6 +24,7 @@ #include "app/src/function_registry.h" #include "app/src/include/firebase/internal/mutex.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/scheduler.h" #include "app/src/variant_util.h" #include "database/src/desktop/connection/persistent_connection.h" @@ -53,7 +54,10 @@ using callback::NewCallback; namespace database { namespace internal { -static Mutex g_scheduler_mutex; // NOLINT +static Mutex& SchedulerMutex() { + static NoDestructor scheduler_mutex; + return *scheduler_mutex; +} static int g_scheduler_ref_count = 0; scheduler::Scheduler* Repo::s_scheduler_; @@ -106,7 +110,7 @@ Repo::Repo(App* app, DatabaseInternal* database, const char* url, url_ = host_info_.ToString(); { - MutexLock lock(g_scheduler_mutex); + MutexLock lock(SchedulerMutex()); g_scheduler_ref_count++; if (s_scheduler_ == nullptr) s_scheduler_ = new scheduler::Scheduler(); } @@ -133,7 +137,7 @@ Repo::~Repo() { safe_this_.ClearReference(); connection_.reset(nullptr); { - MutexLock lock(g_scheduler_mutex); + MutexLock lock(SchedulerMutex()); if (g_scheduler_ref_count) g_scheduler_ref_count--; if (g_scheduler_ref_count == 0) { delete s_scheduler_; diff --git a/database/src/desktop/core/sync_tree.cc b/database/src/desktop/core/sync_tree.cc index d6d4b306d9..f9040e199e 100644 --- a/database/src/desktop/core/sync_tree.cc +++ b/database/src/desktop/core/sync_tree.cc @@ -299,7 +299,7 @@ std::vector SyncTree::ApplyListenComplete(const Path& path) { persistence_manager_->RunInTransaction([&, this]() -> bool { this->persistence_manager_->SetQueryComplete(QuerySpec(path)); results = this->ApplyOperationToSyncPoints( - Operation::ListenComplete(OperationSource::kServer, path)); + Operation::ListenComplete(OperationSource::Server(), path)); return true; }); return results; @@ -312,7 +312,7 @@ std::vector SyncTree::ApplyServerMerge( CompoundWrite merge = CompoundWrite::FromPathMerge(changed_children); this->persistence_manager_->UpdateServerCache(path, merge); results = this->ApplyOperationToSyncPoints( - Operation::Merge(OperationSource::kServer, path, merge)); + Operation::Merge(OperationSource::Server(), path, merge)); return true; }); return results; @@ -447,7 +447,7 @@ std::vector SyncTree::ApplyUserMerge( } pending_write_tree_->AddMerge(path, children, write_id); results = ApplyOperationToSyncPoints( - Operation::Merge(OperationSource::kUser, path, children)); + Operation::Merge(OperationSource::User(), path, children)); return true; }); return results; @@ -468,7 +468,7 @@ std::vector SyncTree::ApplyUserOverwrite( pending_write_tree_->AddOverwrite(path, new_data, write_id, visibility); if (visibility == kOverwriteVisible) { events = this->ApplyOperationToSyncPoints( - Operation::Overwrite(OperationSource::kUser, path, new_data)); + Operation::Overwrite(OperationSource::User(), path, new_data)); } return true; }); diff --git a/database/src/desktop/core/write_tree.cc b/database/src/desktop/core/write_tree.cc index 735198f8f6..a7426c218e 100644 --- a/database/src/desktop/core/write_tree.cc +++ b/database/src/desktop/core/write_tree.cc @@ -168,7 +168,7 @@ Optional WriteTree::CalcCompleteEventCache( if (complete_server_cache != nullptr) { layered_cache = complete_server_cache; } else { - layered_cache = &kNullVariant; + layered_cache = &GetNullVariant(); } return Optional(sub_merge.Apply(*layered_cache)); } diff --git a/database/src/desktop/database_desktop.cc b/database/src/desktop/database_desktop.cc index a7e0a5e544..54a9b87731 100644 --- a/database/src/desktop/database_desktop.cc +++ b/database/src/desktop/database_desktop.cc @@ -41,8 +41,6 @@ using callback::NewCallback; namespace database { namespace internal { -Mutex g_database_reference_constructor_mutex; // NOLINT - SingleValueListener::SingleValueListener(DatabaseInternal* database, const QuerySpec& query_spec, ReferenceCountedFutureImpl* future, diff --git a/database/src/desktop/database_desktop.h b/database/src/desktop/database_desktop.h index 8cd3d24388..f049d41d37 100644 --- a/database/src/desktop/database_desktop.h +++ b/database/src/desktop/database_desktop.h @@ -45,7 +45,7 @@ namespace database { namespace internal { // For constructing, copying or moving DatabaseReferences atomically. -extern Mutex g_database_reference_constructor_mutex; +Mutex& DatabaseReferenceConstructorMutex(); typedef int64_t WriteId; diff --git a/database/src/desktop/persistence/prune_forest.cc b/database/src/desktop/persistence/prune_forest.cc index 15ab043762..449bc343f9 100644 --- a/database/src/desktop/persistence/prune_forest.cc +++ b/database/src/desktop/persistence/prune_forest.cc @@ -19,6 +19,7 @@ #include #include "app/src/assert.h" +#include "app/src/no_destructor.h" #include "app/src/path.h" #include "database/src/desktop/core/tree.h" @@ -26,8 +27,15 @@ namespace firebase { namespace database { namespace internal { -static const Tree kPruneTree(true); // NOLINT -static const Tree kKeepTree(false); // NOLINT +static const Tree& PruneTree() { + static const NoDestructor> prune_tree(true); + return *prune_tree; +} + +static const Tree& KeepTree() { + static const NoDestructor> keep_tree(false); + return *keep_tree; +} static bool KeepPredicate(bool prune) { return !prune; } static bool PrunePredicate(bool prune) { return prune; } @@ -84,7 +92,7 @@ void PruneForestRef::Prune(const Path& path) { if (prune_forest_->RootMostValueMatching(path, PrunePredicate) != nullptr) { // This path will already be pruned } else { - *prune_forest_->GetOrMakeSubtree(path) = kPruneTree; + *prune_forest_->GetOrMakeSubtree(path) = PruneTree(); } } @@ -92,7 +100,7 @@ void PruneForestRef::Keep(const Path& path) { if (prune_forest_->RootMostValueMatching(path, KeepPredicate) != nullptr) { // This path will already be kept } else { - *prune_forest_->GetOrMakeSubtree(path) = kKeepTree; + *prune_forest_->GetOrMakeSubtree(path) = KeepTree(); } } @@ -101,7 +109,7 @@ void PruneForestRef::KeepAll(const Path& path, if (prune_forest_->RootMostValueMatching(path, KeepPredicate) != nullptr) { // This path will already be kept. } else { - DoAll(path, children, kKeepTree); + DoAll(path, children, KeepTree()); } } @@ -114,7 +122,7 @@ void PruneForestRef::PruneAll(const Path& path, if (prune_forest_->RootMostValueMatching(path, PrunePredicate) != nullptr) { // This path will already be pruned. } else { - DoAll(path, children, kPruneTree); + DoAll(path, children, PruneTree()); } } diff --git a/database/src/desktop/query_params_comparator.cc b/database/src/desktop/query_params_comparator.cc index 803c18a971..636dafda3a 100644 --- a/database/src/desktop/query_params_comparator.cc +++ b/database/src/desktop/query_params_comparator.cc @@ -19,6 +19,7 @@ #include #include "app/src/assert.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "database/src/desktop/util_desktop.h" @@ -33,20 +34,34 @@ const char QueryParamsComparator::kMaxKey[] = "[MAX_KEY]"; // down values with these types. These are never compared to other variants // directly - they should only be used with the QueryParamsComparator, which // uses the Variant::type() to do comparisons. -static const Variant kMinVariant = Variant::FromStaticBlob( // NOLINT - QueryParamsComparator::kMinKey, sizeof(QueryParamsComparator::kMinKey)); -static const Variant kMaxVariant = Variant::FromStaticBlob( // NOLINT - QueryParamsComparator::kMaxKey, sizeof(QueryParamsComparator::kMaxKey)); +static const Variant& MinVariant() { + static const NoDestructor min_variant(Variant::FromStaticBlob( + QueryParamsComparator::kMinKey, sizeof(QueryParamsComparator::kMinKey))); + return *min_variant; +} + +static const Variant& MaxVariant() { + static const NoDestructor max_variant(Variant::FromStaticBlob( + QueryParamsComparator::kMaxKey, sizeof(QueryParamsComparator::kMaxKey))); + return *max_variant; +} -const std::pair QueryParamsComparator::kMinNode = // NOLINT - std::make_pair(QueryParamsComparator::kMinKey, kMinVariant); -const std::pair QueryParamsComparator::kMaxNode = // NOLINT - std::make_pair(QueryParamsComparator::kMaxKey, kMaxVariant); +const std::pair& QueryParamsComparator::MinNode() { + static const NoDestructor> min_node( + std::make_pair(QueryParamsComparator::kMinKey, MinVariant())); + return *min_node; +} + +const std::pair& QueryParamsComparator::MaxNode() { + static const NoDestructor> max_node( + std::make_pair(QueryParamsComparator::kMaxKey, MaxVariant())); + return *max_node; +} static int VariantIsSentinel(const Variant& key, const Variant& value) { - if (key == QueryParamsComparator::kMinKey && value == kMinVariant) { + if (key == QueryParamsComparator::kMinKey && value == MinVariant()) { return -1; - } else if (key == QueryParamsComparator::kMaxKey && value == kMaxVariant) { + } else if (key == QueryParamsComparator::kMaxKey && value == MaxVariant()) { return 1; } else { return 0; @@ -179,14 +194,14 @@ int QueryParamsComparator::CompareValues(const Variant& variant_a, // If we encounted a special sentinel value, figure out what we actually want // the precendence to be. if (precedence_a == kPrecedenceSentinel) { - assert(*value_a == kMinVariant || *value_a == kMaxVariant); + assert(*value_a == MinVariant() || *value_a == MaxVariant()); precedence_a = - (*value_a == kMinVariant) ? kPrecedenceFirst : kPrecedenceLast; + (*value_a == MinVariant()) ? kPrecedenceFirst : kPrecedenceLast; } if (precedence_b == kPrecedenceSentinel) { - assert(*value_b == kMinVariant || *value_b == kMaxVariant); + assert(*value_b == MinVariant() || *value_b == MaxVariant()); precedence_b = - (*value_b == kMinVariant) ? kPrecedenceFirst : kPrecedenceLast; + (*value_b == MinVariant()) ? kPrecedenceFirst : kPrecedenceLast; } // Values coming down from the server should never contain blobs or vectors. diff --git a/database/src/desktop/query_params_comparator.h b/database/src/desktop/query_params_comparator.h index c1a4fdb8c0..f8e26398c5 100644 --- a/database/src/desktop/query_params_comparator.h +++ b/database/src/desktop/query_params_comparator.h @@ -88,8 +88,8 @@ class QueryParamsComparator { // These values will always be sorted before or after all over values. static const char kMinKey[]; static const char kMaxKey[]; - static const std::pair kMinNode; - static const std::pair kMaxNode; + static const std::pair& MinNode(); + static const std::pair& MaxNode(); private: int CompareChildren(const Variant& value_a, const Variant& value_b) const; diff --git a/database/src/desktop/util_desktop.cc b/database/src/desktop/util_desktop.cc index ac2f10868b..67c22694e9 100644 --- a/database/src/desktop/util_desktop.cc +++ b/database/src/desktop/util_desktop.cc @@ -26,6 +26,7 @@ #include "app/src/assert.h" #include "app/src/include/firebase/variant.h" +#include "app/src/no_destructor.h" #include "database/src/common/query_spec.h" #include "database/src/desktop/view/indexed_filter.h" #include "database/src/desktop/view/limited_filter.h" @@ -47,7 +48,10 @@ namespace firebase { namespace database { namespace internal { -const Variant kNullVariant = Variant::Null(); // NOLINT +const Variant& GetNullVariant() { + static const NoDestructor null_variant(Variant::Null()); + return *null_variant; +} const char kValueKey[] = ".value"; const char kPriorityKey[] = ".priority"; @@ -93,14 +97,14 @@ static const Variant& VariantGetImmediateChild(const Variant* variant, if (IsPriorityKey(key)) { return GetVariantPriority(*variant); } else { - return kNullVariant; + return GetNullVariant(); } } else { if (IsPriorityKey(key)) { return GetVariantPriority(*variant); } else { const Variant* result = MapGet(&variant->map(), key); - return result ? *result : kNullVariant; + return result ? *result : GetNullVariant(); } } } @@ -112,7 +116,7 @@ const Variant& VariantGetChild(const Variant* variant, const Path& path) { } else if (IsPriorityKey(path.FrontDirectory().str())) { return GetVariantPriority(*variant); } else { - return kNullVariant; + return GetNullVariant(); } } else { std::string front = path.FrontDirectory().str(); @@ -625,11 +629,11 @@ Variant* GetVariantValue(Variant* variant) { const Variant& GetVariantPriority(const Variant& variant) { if (!variant.is_map()) { - return kNullVariant; + return GetNullVariant(); } auto iter = variant.map().find(kPriorityKey); if (iter == variant.map().end()) { - return kNullVariant; + return GetNullVariant(); } return iter->second; } @@ -1075,7 +1079,7 @@ std::pair GetStartPost(const QueryParams& params) { if (HasStart(params)) { return MakePost(params, GetStartName(params), GetStartValue(params)); } else { - return QueryParamsComparator::kMinNode; + return QueryParamsComparator::MinNode(); } } @@ -1083,7 +1087,7 @@ std::pair GetEndPost(const QueryParams& params) { if (HasEnd(params)) { return MakePost(params, GetEndName(params), GetEndValue(params)); } else { - return QueryParamsComparator::kMaxNode; + return QueryParamsComparator::MaxNode(); } } diff --git a/database/src/desktop/util_desktop.h b/database/src/desktop/util_desktop.h index ef168ad87c..c6e47d0901 100644 --- a/database/src/desktop/util_desktop.h +++ b/database/src/desktop/util_desktop.h @@ -42,7 +42,7 @@ class VariantFilter; // Rather than construct a null_variant in the current scope any place where // this happens, it can be helpful to have a persistent one that can be used // from anywhere. -extern const Variant kNullVariant; +const Variant& GetNullVariant(); // The virtual key for the value. This has special meaning to the database, and // for all intents and purposes the value at this location in the map should be @@ -87,7 +87,7 @@ bool PatchVariant(const Variant& patch_data, Variant* out_data); // Get a child of a given variant, respecting .value and .priorty virtual keys // appropriately. This will always return some value. If there is a value at the // given path, a reference to that value will be returned. If there is not, a -// reference to kNullVariant will be returned. +// reference to GetNullVariant() will be returned. // // This function is designed to perfectly mimic the behavior of Node.getChild in // the Java API. This should be used in place of GetInternalVariant, which is diff --git a/database/src/desktop/view/limited_filter.cc b/database/src/desktop/view/limited_filter.cc index 9c65f26d6e..e03b121100 100644 --- a/database/src/desktop/view/limited_filter.cc +++ b/database/src/desktop/view/limited_filter.cc @@ -43,8 +43,9 @@ IndexedVariant LimitedFilter::UpdateChild( const CompleteChildSource* source, ChildChangeAccumulator* opt_change_accumulator) const { const Variant& variant = - ranged_filter_->Matches(std::make_pair(key, new_child)) ? new_child - : kNullVariant; + ranged_filter_->Matches(std::make_pair(key, new_child)) + ? new_child + : GetNullVariant(); const Variant* child = GetInternalVariant(&indexed_variant.variant(), key); if (child && (*child == variant)) { diff --git a/database/src/desktop/view/ranged_filter.cc b/database/src/desktop/view/ranged_filter.cc index a68c3d5017..7ff26e851a 100644 --- a/database/src/desktop/view/ranged_filter.cc +++ b/database/src/desktop/view/ranged_filter.cc @@ -41,7 +41,8 @@ IndexedVariant RangedFilter::UpdateChild( const Variant& new_child, const Path& affected_path, const CompleteChildSource* source, ChildChangeAccumulator* opt_change_accumulator) const { - const Variant& variant = Matches(key, new_child) ? new_child : kNullVariant; + const Variant& variant = + Matches(key, new_child) ? new_child : GetNullVariant(); return indexed_filter_->UpdateChild(indexed_variant, key, variant, affected_path, source, opt_change_accumulator); diff --git a/database/src/desktop/view/view_processor.cc b/database/src/desktop/view/view_processor.cc index 2db179b8a3..c6d80a9cec 100644 --- a/database/src/desktop/view/view_processor.cc +++ b/database/src/desktop/view/view_processor.cc @@ -658,7 +658,7 @@ ViewCache ViewProcessor::AckUserWrite(const ViewCache& view_cache, server_cache.IsCompleteForPath(ack_path)) { const Variant* variant = GetInternalVariant(&server_cache.variant(), ack_path); - if (!variant) variant = &kNullVariant; + if (!variant) variant = &GetNullVariant(); return ApplyServerOverwrite(view_cache, ack_path, *variant, writes_cache, opt_complete_cache, filter_server_node, accumulator); diff --git a/database/src/ios/database_ios.h b/database/src/ios/database_ios.h index 08072cd062..4671aaff52 100644 --- a/database/src/ios/database_ios.h +++ b/database/src/ios/database_ios.h @@ -39,7 +39,7 @@ namespace database { namespace internal { // For constructing, copying or moving DatabaseReferences atomically. -extern Mutex g_database_reference_constructor_mutex; +Mutex& DatabaseReferenceConstructorMutex(); // This defines the class FIRDatabasePointer, which is a C++-compatible wrapper // around the FIRDatabase Obj-C class. diff --git a/database/src/ios/database_ios.mm b/database/src/ios/database_ios.mm index 50f0879bb5..ac54579158 100644 --- a/database/src/ios/database_ios.mm +++ b/database/src/ios/database_ios.mm @@ -27,8 +27,6 @@ namespace database { namespace internal { -Mutex g_database_reference_constructor_mutex; // NOLINT - DatabaseInternal::DatabaseInternal(App* app) : app_(app), logger_(app_common::FindAppLoggerByName(app->name())) { @try { diff --git a/database/tests/desktop/core/operation_test.cc b/database/tests/desktop/core/operation_test.cc index a1eb76eee6..db32beb4d3 100644 --- a/database/tests/desktop/core/operation_test.cc +++ b/database/tests/desktop/core/operation_test.cc @@ -97,7 +97,7 @@ TEST(OperationSource, ForServerTaggedQuery) { } TEST(Operation, Overwrite) { - Operation op = Operation::Overwrite(OperationSource::kServer, Path("A/B/C"), + Operation op = Operation::Overwrite(OperationSource::Server(), Path("A/B/C"), Variant(100)); EXPECT_EQ(op.type, Operation::kTypeOverwrite); EXPECT_EQ(op.source.source, OperationSource::kSourceServer); @@ -115,7 +115,7 @@ TEST(Operation, Merge) { }; CompoundWrite write = CompoundWrite::FromPathMerge(merge); Operation op = - Operation::Merge(OperationSource::kServer, Path("A/B/C"), write); + Operation::Merge(OperationSource::Server(), Path("A/B/C"), write); EXPECT_EQ(op.type, Operation::kTypeMerge); EXPECT_EQ(op.source.source, OperationSource::kSourceServer); @@ -153,7 +153,7 @@ TEST(Operation, AckUserWrite) { TEST(Operation, ListenComplete) { Operation op = - Operation::ListenComplete(OperationSource::kServer, Path("A/B/C")); + Operation::ListenComplete(OperationSource::Server(), Path("A/B/C")); EXPECT_EQ(op.type, Operation::kTypeListenComplete); EXPECT_EQ(op.source.source, OperationSource::kSourceServer); EXPECT_FALSE(op.source.query_params.has_value()); @@ -168,8 +168,9 @@ TEST(OperationDeathTest, DISABLED_ListenCompleteWithWrongSource) { TEST(OperationDeathTest, ListenCompleteWithWrongSource) { #endif // ListenCompletes must come from the server, not the user. - EXPECT_DEATH(Operation::ListenComplete(OperationSource::kUser, Path("A/B/C")), - DEATHTEST_SIGABRT); + EXPECT_DEATH( + Operation::ListenComplete(OperationSource::User(), Path("A/B/C")), + DEATHTEST_SIGABRT); } TEST(Operation, OperationForChildOverwriteEmptyPath) { @@ -178,7 +179,7 @@ TEST(Operation, OperationForChildOverwriteEmptyPath) { std::make_pair("bbb", 200), std::make_pair("ccc", 300), }; - Operation op = Operation::Overwrite(OperationSource::kServer, Path(), + Operation op = Operation::Overwrite(OperationSource::Server(), Path(), Variant(variant_data)); Optional result = OperationForChild(op, "aaa"); EXPECT_TRUE(result.has_value()); @@ -195,7 +196,7 @@ TEST(Operation, OperationForChildOverwriteNonEmptyPath) { std::make_pair("bbb", 200), std::make_pair("ccc", 300), }; - Operation op = Operation::Overwrite(OperationSource::kServer, Path("A/B/C"), + Operation op = Operation::Overwrite(OperationSource::Server(), Path("A/B/C"), Variant(variant_data)); Optional result = OperationForChild(op, "A"); EXPECT_TRUE(result.has_value()); @@ -214,7 +215,7 @@ TEST(Operation, OperationForChildMergeEmptyPath) { std::make_pair(Path("ccc/ddd"), 300), }; CompoundWrite write = CompoundWrite::FromPathMerge(merge_data); - Operation op = Operation::Merge(OperationSource::kServer, Path(), write); + Operation op = Operation::Merge(OperationSource::Server(), Path(), write); Optional result = OperationForChild(op, "zzz"); @@ -227,7 +228,7 @@ TEST(Operation, OperationForChildMergeEmptyPath) { std::make_pair(Path("ccc/ddd"), 300), }; CompoundWrite write = CompoundWrite::FromPathMerge(merge_data); - Operation op = Operation::Merge(OperationSource::kServer, Path(), write); + Operation op = Operation::Merge(OperationSource::Server(), Path(), write); Optional result = OperationForChild(op, "aaa"); @@ -245,7 +246,7 @@ TEST(Operation, OperationForChildMergeEmptyPath) { std::make_pair(Path("ccc/ddd"), 300), }; CompoundWrite write = CompoundWrite::FromPathMerge(merge_data); - Operation op = Operation::Merge(OperationSource::kServer, Path(), write); + Operation op = Operation::Merge(OperationSource::Server(), Path(), write); Optional result = OperationForChild(op, "ccc"); @@ -266,7 +267,7 @@ TEST(Operation, OperationForChildMergeNonEmptyPath) { }; CompoundWrite write = CompoundWrite::FromPathMerge(merge_data); Operation op = - Operation::Merge(OperationSource::kServer, Path("A/B/C"), write); + Operation::Merge(OperationSource::Server(), Path("A/B/C"), write); Optional result = OperationForChild(op, "A"); @@ -288,7 +289,7 @@ TEST(Operation, OperationForChildMergeNonEmptyPath) { }; CompoundWrite write = CompoundWrite::FromPathMerge(merge_data); Operation op = - Operation::Merge(OperationSource::kServer, Path("A/B/C"), write); + Operation::Merge(OperationSource::Server(), Path("A/B/C"), write); Optional result = OperationForChild(op, "Z"); @@ -419,7 +420,7 @@ TEST(Operation, } TEST(Operation, OperationForChildListenCompleteEmptyPath) { - Operation op = Operation::ListenComplete(OperationSource::kServer, Path()); + Operation op = Operation::ListenComplete(OperationSource::Server(), Path()); Optional result = OperationForChild(op, "Z"); @@ -433,7 +434,7 @@ TEST(Operation, OperationForChildListenCompleteEmptyPath) { TEST(Operation, OperationForChildListenCompleteNonEmptyPath) { Operation op = - Operation::ListenComplete(OperationSource::kServer, Path("A/B/C")); + Operation::ListenComplete(OperationSource::Server(), Path("A/B/C")); Optional result = OperationForChild(op, "A"); diff --git a/database/tests/desktop/view/view_processor_test.cc b/database/tests/desktop/view/view_processor_test.cc index b07721a57d..34b007ed22 100644 --- a/database/tests/desktop/view/view_processor_test.cc +++ b/database/tests/desktop/view/view_processor_test.cc @@ -57,7 +57,7 @@ TEST(ViewProcessor, ApplyOperationUserOverwrite_WithEmptyPath) { // Create a user-initiated overwrite with an empty path to change a value. Operation operation = - Operation::Overwrite(OperationSource::kUser, Path(), Variant("apples")); + Operation::Overwrite(OperationSource::User(), Path(), Variant("apples")); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path()); @@ -94,7 +94,7 @@ TEST(ViewProcessor, ApplyOperationUserOverwrite_WithPriorityPath) { ViewCache old_view_cache(initial_local_cache, initial_server_cache); // Create a user-initiated overwrite with an empty path to change a value. - Operation operation = Operation::Overwrite(OperationSource::kUser, + Operation operation = Operation::Overwrite(OperationSource::User(), Path(".priority"), Variant(100)); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path()); @@ -133,7 +133,7 @@ TEST(ViewProcessor, ApplyOperationUserOverwrite_WithRegularPath) { // Create a user-initiated overwrite with a non-empty path to change a value. Operation operation = Operation::Overwrite( - OperationSource::kUser, Path("aaa/bbb"), Variant("apples")); + OperationSource::User(), Path("aaa/bbb"), Variant("apples")); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path("aaa/bbb")); @@ -186,8 +186,8 @@ TEST(ViewProcessor, ApplyOperationServerOverwrite_WithEmptyPath) { ViewCache old_view_cache(initial_local_cache, initial_server_cache); // Create a server-initiated overwrite with an empty path to change a value. - Operation operation = - Operation::Overwrite(OperationSource::kServer, Path(), Variant("apples")); + Operation operation = Operation::Overwrite(OperationSource::Server(), Path(), + Variant("apples")); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path()); Variant complete_cache; @@ -223,7 +223,7 @@ TEST(ViewProcessor, ApplyOperationServerOverwrite_RegularPath) { ViewCache old_view_cache(initial_local_cache, initial_server_cache); Operation operation = - Operation::Overwrite(OperationSource::kServer, Path("aaa"), + Operation::Overwrite(OperationSource::Server(), Path("aaa"), Variant(std::map{ std::make_pair("bbb", "apples"), })); @@ -274,18 +274,17 @@ TEST(ViewProcessor, ApplyOperationServerOverwrite_DistantDescendantChange) { // Set up some dummy data. CacheNode initial_cache(Variant(std::map{std::make_pair( - "aaa", - std::map{std::make_pair( - "bbb", - std::map{ - std::make_pair("ccc", 1000), - })})}), + "aaa", std::map{std::make_pair( + "bbb", + std::map{ + std::make_pair("ccc", 1000), + })})}), true, false); ViewCache old_view_cache(initial_cache, initial_cache); // Make sure the data being updated is deeply nested in the variant. Operation operation = Operation::Overwrite( - OperationSource::kServer, Path("aaa/bbb/ccc"), Variant(-9999)); + OperationSource::Server(), Path("aaa/bbb/ccc"), Variant(-9999)); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path()); @@ -300,12 +299,11 @@ TEST(ViewProcessor, ApplyOperationServerOverwrite_DistantDescendantChange) { // Both caches are expected to be the same. CacheNode expected_cache(Variant(std::map{std::make_pair( - "aaa", - std::map{std::make_pair( - "bbb", - std::map{ - std::make_pair("ccc", -9999), - })})}), + "aaa", std::map{std::make_pair( + "bbb", + std::map{ + std::make_pair("ccc", -9999), + })})}), true, false); ViewCache expected_view_cache(expected_cache, expected_cache); @@ -355,7 +353,8 @@ TEST(ViewProcessor, ApplyOperationUserMerge) { .AddWrite(Path("aaa/bbb/ccc"), Variant("apples")) .AddWrite(Path("aaa/ddd"), Variant("bananas")) .AddWrite(Path("aaa/eee/fff"), Variant("vegetables")); - Operation operation = Operation::Merge(OperationSource::kUser, Path(), write); + Operation operation = + Operation::Merge(OperationSource::User(), Path(), write); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path()); Variant complete_cache; @@ -450,7 +449,7 @@ TEST(ViewProcessor, ApplyOperationServerMerge) { .AddWrite(Path("bbb/ddd"), Variant("bananas")) .AddWrite(Path("bbb/eee/fff"), Variant("vegetables")); Operation operation = - Operation::Merge(OperationSource::kServer, Path("aaa"), write); + Operation::Merge(OperationSource::Server(), Path("aaa"), write); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path("aaa")); @@ -696,7 +695,7 @@ TEST(ViewProcessor, ApplyOperationListenComplete) { // Create a server-initiated listen complete with an empty path to change a // value. Operation operation = - Operation::ListenComplete(OperationSource::kServer, Path()); + Operation::ListenComplete(OperationSource::Server(), Path()); WriteTree writes_cache; WriteTreeRef writes_cache_ref = writes_cache.ChildWrites(Path()); diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index a278e26c96..40514fbdb5 100644 --- a/firestore/CMakeLists.txt +++ b/firestore/CMakeLists.txt @@ -414,6 +414,12 @@ target_compile_definitions(firebase_firestore PUBLIC -DPB_FIELD_32BIT -DPB_ENABLE_MALLOC -DPB_NO_PACKED_STRUCTS ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_firestore PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(firestore) diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index db2c74d4a1..611bd8500a 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -22,6 +22,7 @@ #include "app/src/assert.h" #include "app/src/embedded_file.h" #include "app/src/include/firebase/future.h" +#include "app/src/no_destructor.h" #include "app/src/reference_counted_future_impl.h" #include "firestore/firestore_resources.h" #include "firestore/src/android/aggregate_query_android.h" @@ -234,8 +235,11 @@ class JavaFirestoreMap { Global firestores_; }; -// init_mutex protects all the globals below. -Mutex init_mutex; // NOLINT +// InitMutex() protects all the globals below. +Mutex& InitMutex() { + static NoDestructor init_mutex; + return *init_mutex; +} int initialize_count = 0; Loader* global_loader = nullptr; @@ -296,7 +300,7 @@ FirestoreInternal::FirestoreInternal(App* app, const std::string& database_id) { /* static */ bool FirestoreInternal::Initialize(App* app) { - MutexLock init_lock(init_mutex); + MutexLock init_lock(InitMutex()); if (initialize_count == 0) { jni::Initialize(app->java_vm()); @@ -381,7 +385,7 @@ bool FirestoreInternal::Initialize(App* app) { /* static */ void FirestoreInternal::ReleaseClassesLocked(Env& env) { - // Assumes `init_mutex` is held. + // Assumes `InitMutex()` is held. String::Terminate(env); delete global_loader; @@ -390,7 +394,7 @@ void FirestoreInternal::ReleaseClassesLocked(Env& env) { /* static */ void FirestoreInternal::Terminate(App* app) { - MutexLock init_lock(init_mutex); + MutexLock init_lock(InitMutex()); FIREBASE_ASSERT(initialize_count > 0); initialize_count--; if (initialize_count == 0) { @@ -676,7 +680,7 @@ void Firestore::set_log_level(LogLevel log_level) { bool logging_enabled = log_level < LogLevel::kLogLevelInfo; { - MutexLock lock(init_mutex); + MutexLock lock(InitMutex()); // Set the initial_log_state on every invocation, just in case Firestore // is terminated for long enough to unload the Firestore classes within diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 805ee38519..3742735a6a 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -23,8 +23,10 @@ #include #include "app/src/cleanup_notifier.h" +#include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "firestore/src/common/compiler_info.h" #include "firestore/src/common/exception_common.h" @@ -72,30 +74,29 @@ FirestoreMap::key_type MakeKey(App* app, std::string database_id) { return std::make_pair(app, std::move(database_id)); } -Mutex* g_firestores_lock = new Mutex(); -FirestoreMap* g_firestores = nullptr; +// The cache and its mutex are function-local statics wrapped in NoDestructor, +// so they are constructed on first use regardless of translation-unit init +// order and remain valid if a static Firestore instance is destroyed at exit. +Mutex& FirestoresLock() { + static NoDestructor lock; + return *lock; +} -// Ensures that the cache is initialized. -// Prerequisite: `g_firestores_lock` must be locked before calling this -// function. -FirestoreMap* FirestoreCache() { - if (!g_firestores) { - g_firestores = new FirestoreMap(); - } - return g_firestores; +// Prerequisite: `FirestoresLock()` must be locked before calling this function. +FirestoreMap& FirestoreCache() { + static NoDestructor firestores; + return *firestores; } -// Prerequisite: `g_firestores_lock` must be locked before calling this -// function. +// Prerequisite: `FirestoresLock()` must be locked before calling this function. Firestore* FindFirestoreInCache(App* app, const std::string& database_id, InitResult* init_result_out) { - auto* cache = FirestoreCache(); - + FirestoreMap& cache = FirestoreCache(); FirestoreMap::key_type key = MakeKey(app, database_id); - FirestoreMap::iterator found = g_firestores->find(key); + FirestoreMap::iterator found = cache.find(key); - if (found != cache->end()) { + if (found != cache.end()) { if (init_result_out) *init_result_out = kInitResultSuccess; return found->second; } @@ -162,7 +163,7 @@ Firestore* Firestore::GetInstance(App* app, ValidateApp(app); ValidateDatabase(db_name); - MutexLock lock(*g_firestores_lock); + MutexLock lock(FirestoresLock()); Firestore* from_cache = FindFirestoreInCache(app, db_name, init_result_out); if (from_cache) { return from_cache; @@ -178,7 +179,7 @@ Firestore* Firestore::CreateFirestore(App* app, SIMPLE_HARD_ASSERT(internal != nullptr, "Provided FirestoreInternal must not be null."); - MutexLock lock(*g_firestores_lock); + MutexLock lock(FirestoresLock()); const std::string& database_id = internal->database_name(); Firestore* from_cache = @@ -202,7 +203,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, FirestoreMap::key_type key = MakeKey(firestore->app(), firestore->internal_->database_name()); - FirestoreCache()->emplace(std::move(key), firestore); + FirestoreCache().emplace(std::move(key), firestore); return firestore; } @@ -238,7 +239,7 @@ Firestore::Firestore(FirestoreInternal* internal) Firestore::~Firestore() { DeleteInternal(); } void Firestore::DeleteInternal() { - MutexLock lock(*g_firestores_lock); + MutexLock lock(FirestoresLock()); if (!internal_) return; @@ -270,11 +271,7 @@ void Firestore::DeleteInternal() { // If a Firestore is explicitly deleted, remove it from our cache. FirestoreMap::key_type key = MakeKey(my_app, database_id); - FirestoreCache()->erase(key); - if (g_firestores->empty()) { - delete g_firestores; - g_firestores = nullptr; - } + FirestoreCache().erase(key); } const App* Firestore::app() const { @@ -382,8 +379,10 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); FirestoreMap::key_type key = MakeKey(app(), internal_->database_name()); - - FirestoreCache()->erase(key); + { + MutexLock lock(FirestoresLock()); + FirestoreCache().erase(key); + } return internal_->Terminate(); } diff --git a/firestore/src/common/util.cc b/firestore/src/common/util.cc index 67a795e586..e8d67bffdf 100644 --- a/firestore/src/common/util.cc +++ b/firestore/src/common/util.cc @@ -16,12 +16,14 @@ #include "firestore/src/common/util.h" +#include "app/src/no_destructor.h" + namespace firebase { namespace firestore { const std::string& EmptyString() { - static const std::string kEmptyString; - return kEmptyString; + static const NoDestructor empty_string; + return *empty_string; } } // namespace firestore diff --git a/functions/CMakeLists.txt b/functions/CMakeLists.txt index 62d3b0fe06..debe980a6d 100644 --- a/functions/CMakeLists.txt +++ b/functions/CMakeLists.txt @@ -82,6 +82,12 @@ target_compile_definitions(firebase_functions -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_functions PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(functions) elseif(IOS) diff --git a/functions/src/android/functions_android.cc b/functions/src/android/functions_android.cc index 0d2df8a711..93b6dfd15f 100644 --- a/functions/src/android/functions_android.cc +++ b/functions/src/android/functions_android.cc @@ -19,6 +19,7 @@ #include #include "app/src/include/firebase/app.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "app/src/util_android.h" #include "functions/src/android/callable_reference_android.h" @@ -102,7 +103,10 @@ METHOD_LOOKUP_DEFINITION( "com/google/firebase/functions/FirebaseFunctionsException$Code", FUNCTIONS_EXCEPTION_CODE_METHODS) -Mutex FunctionsInternal::init_mutex_; // NOLINT +static Mutex& InitMutex() { + static NoDestructor init_mutex; + return *init_mutex; +} int FunctionsInternal::initialize_count_ = 0; FunctionsInternal::FunctionsInternal(App* app, const char* region) @@ -142,7 +146,7 @@ FunctionsInternal::~FunctionsInternal() { } bool FunctionsInternal::Initialize(App* app) { - MutexLock init_lock(init_mutex_); + MutexLock init_lock(InitMutex()); if (initialize_count_ == 0) { JNIEnv* env = app->GetJNIEnv(); jobject activity = app->activity(); @@ -162,7 +166,7 @@ bool FunctionsInternal::Initialize(App* app) { } void FunctionsInternal::Terminate(App* app) { - MutexLock init_lock(init_mutex_); + MutexLock init_lock(InitMutex()); assert(initialize_count_ > 0); initialize_count_--; if (initialize_count_ == 0) { diff --git a/functions/src/android/functions_android.h b/functions/src/android/functions_android.h index 21a9f234e2..999507f606 100644 --- a/functions/src/android/functions_android.h +++ b/functions/src/android/functions_android.h @@ -92,9 +92,7 @@ class FunctionsInternal { // Initialize classes loaded from embedded files. static bool InitializeEmbeddedClasses(App* app); - static Mutex init_mutex_; static int initialize_count_; - static std::map java_error_to_cpp_; App* app_; std::string region_; diff --git a/functions/src/common/functions.cc b/functions/src/common/functions.cc index 4166d4b968..da5aee6660 100644 --- a/functions/src/common/functions.cc +++ b/functions/src/common/functions.cc @@ -55,18 +55,23 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseFunctions); constexpr const char* kDefaultRegion = "us-central1"; -// A function-local static wrapped in NoDestructor, so it is constructed on -// first use and never destroyed. That keeps it out of static initialization -// and destruction entirely: it is ready no matter which translation unit's -// initializers run first, and a Functions owned by a static object can still -// lock it from its destructor after this file's globals would otherwise have -// been torn down by the C++ runtime. +using FunctionsMap = std::map, Functions*>; + +// The map and its mutex are function-local statics wrapped in NoDestructor, so +// they are constructed on first use and never destroyed. That keeps them out +// of static initialization and destruction entirely: they are ready no matter +// which translation unit's initializers run first, and a Functions owned by a +// static object can still lock the mutex and erase itself from its destructor +// during process exit. static Mutex& FunctionsLock() { static NoDestructor mutex; return *mutex; } -static std::map, Functions*>* g_functions = - nullptr; + +static FunctionsMap& FunctionsByKey() { + static NoDestructor functions; + return *functions; +} Functions* Functions::GetInstance(::firebase::App* app, InitResult* init_result_out) { @@ -76,9 +81,6 @@ Functions* Functions::GetInstance(::firebase::App* app, Functions* Functions::GetInstance(::firebase::App* app, const char* region, InitResult* init_result_out) { MutexLock lock(FunctionsLock()); - if (!g_functions) { - g_functions = new std::map, Functions*>(); - } // Region we use for our global index of Functions instances. // If a null region is given, we use the default region. @@ -89,8 +91,9 @@ Functions* Functions::GetInstance(::firebase::App* app, const char* region, region_idx = std::string(kDefaultRegion); } - auto it = g_functions->find(std::make_pair(app, region_idx)); - if (it != g_functions->end()) { + FunctionsMap& functions_map = FunctionsByKey(); + auto it = functions_map.find(std::make_pair(app, region_idx)); + if (it != functions_map.end()) { if (init_result_out != nullptr) *init_result_out = kInitResultSuccess; return it->second; } @@ -101,7 +104,7 @@ Functions* Functions::GetInstance(::firebase::App* app, const char* region, delete functions; return nullptr; } - g_functions->insert( + functions_map.insert( std::make_pair(std::make_pair(app, region_idx), functions)); if (init_result_out) *init_result_out = kInitResultSuccess; return functions; @@ -133,7 +136,7 @@ void Functions::DeleteInternal() { if (!internal_) return; // An instance whose initialization failed was never registered with the - // App's CleanupNotifier or added to g_functions, and app() may be null. + // App's CleanupNotifier or added to FunctionsByKey(), and app() may be null. if (!internal_->initialized()) { delete internal_; internal_ = nullptr; @@ -148,14 +151,9 @@ void Functions::DeleteInternal() { // If a Functions is explicitly deleted, remove it from our cache. std::string region = internal_->region(); std::string region_idx = region.empty() ? kDefaultRegion : region; - g_functions->erase(std::make_pair(app(), region_idx)); + FunctionsByKey().erase(std::make_pair(app(), region_idx)); delete internal_; internal_ = nullptr; - // If it's the last one, delete the map. - if (g_functions->empty()) { - delete g_functions; - g_functions = nullptr; - } } ::firebase::App* Functions::app() { diff --git a/installations/CMakeLists.txt b/installations/CMakeLists.txt index 1ead2dfd92..0cf64a904b 100644 --- a/installations/CMakeLists.txt +++ b/installations/CMakeLists.txt @@ -65,6 +65,12 @@ target_compile_definitions(firebase_installations -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_installations PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(installations) elseif(IOS) diff --git a/installations/src/android/installations_android.cc b/installations/src/android/installations_android.cc index 73344b19ce..c6d3861b71 100644 --- a/installations/src/android/installations_android.cc +++ b/installations/src/android/installations_android.cc @@ -14,6 +14,7 @@ #include "installations/src/android/installations_android.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "app/src/util_android.h" #include "installations/src/common.h" @@ -55,7 +56,10 @@ METHOD_LOOKUP_DEFINITION( using firebase::internal::ReferenceCount; using firebase::internal::ReferenceCountLock; -ReferenceCount internal::InstallationsInternal::initializer_; // NOLINT +static ReferenceCount& Initializer() { + static NoDestructor initializer; + return *initializer; +} template struct FISDataHandle { @@ -134,8 +138,8 @@ void TokenResultCallback(JNIEnv* env, jobject result, } InstallationsInternal::InstallationsInternal(const firebase::App& app) - : app_(app), future_impl_(kInstallationsFnCount) { - ReferenceCountLock lock(&initializer_); + : app_(app), future_impl_(kInstallationsFnCount), internal_obj_(nullptr) { + ReferenceCountLock lock(&Initializer()); static const char* kApiIdentifier = "Installations"; LogDebug("%s API Initializing", kApiIdentifier); JNIEnv* env = app_.GetJNIEnv(); @@ -183,7 +187,7 @@ bool InstallationsInternal::Initialized() const { } void InstallationsInternal::Cleanup() { - ReferenceCountLock lock(&initializer_); + ReferenceCountLock lock(&Initializer()); if (lock.RemoveReference() == 1) { JNIEnv* env = app_.GetJNIEnv(); ReleaseClasses(env); diff --git a/installations/src/android/installations_android.h b/installations/src/android/installations_android.h index d709d378ea..897b9a90af 100644 --- a/installations/src/android/installations_android.h +++ b/installations/src/android/installations_android.h @@ -54,8 +54,6 @@ class InstallationsInternal { void Cleanup(); private: - static firebase::internal::ReferenceCount initializer_; - const firebase::App& app_; /// Handle calls from Futures that the API returns. diff --git a/installations/src/installations.cc b/installations/src/installations.cc index a1a170f8ce..881c407687 100644 --- a/installations/src/installations.cc +++ b/installations/src/installations.cc @@ -24,13 +24,19 @@ namespace firebase { namespace installations { -std::map* g_installations = nullptr; // NOLINT -// A function-local static wrapped in NoDestructor, so it is constructed on -// first use and never destroyed. That keeps it out of static initialization -// and destruction entirely: it is ready no matter which translation unit's -// initializers run first, and an Installations owned by a static object can -// still lock it from its destructor after this file's globals would otherwise -// have been torn down by the C++ runtime. +using InstallationsMap = std::map; + +// The map and its mutex are function-local statics wrapped in NoDestructor, so +// they are constructed on first use and never destroyed. That keeps them out +// of static initialization and destruction entirely: they are ready regardless +// of translation-unit initialization order, and an Installations instance +// owned by a static object can still lock the mutex and erase itself from its +// destructor during process exit. +static InstallationsMap& InstallationsByApp() { + static NoDestructor installations; + return *installations; +} + static Mutex& InstallationsMutex() { static NoDestructor mutex; return *mutex; @@ -40,15 +46,9 @@ using internal::InstallationsInternal; // Must be called with InstallationsMutex() held. static Installations* FindInstallationsLocked(App* app) { - if (!g_installations) { - g_installations = new std::map(); // NOLINT - return nullptr; - } - - // Return the Installations if it already exists. - std::map::iterator it = - g_installations->find(app); // NOLINT - if (it != g_installations->end()) { + InstallationsMap& installations = InstallationsByApp(); + InstallationsMap::iterator it = installations.find(app); + if (it != installations.end()) { return it->second; } return nullptr; @@ -86,12 +86,11 @@ Installations* Installations::GetInstance(App* app) { installations_local->DeleteInternal(); }); - // Stick it in the global map so we remember it, and can delete it on - // shutdown. - (*g_installations)[app] = installations; + InstallationsByApp()[app] = installations; return installations; } + delete installations; return nullptr; } @@ -100,33 +99,15 @@ Installations* Installations::FindInstallations(App* app) { return FindInstallationsLocked(app); } +// Private constructor called only from GetInstance() while InstallationsMutex() +// is already held. Installations::Installations(App* app) : app_(app) { - MutexLock lock(InstallationsMutex()); + FIREBASE_ASSERT(app != nullptr); installations_internal_ = new InstallationsInternal(*app); } Installations::~Installations() { - MutexLock lock(InstallationsMutex()); - - CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app_); - if (notifier) { - notifier->UnregisterObject(this); - } - - if (installations_internal_) { - installations_internal_->Cleanup(); - delete installations_internal_; - installations_internal_ = nullptr; - } - - if (g_installations) { - // Remove installations from the g_installations map. - g_installations->erase(app_); - if (g_installations->empty()) { - delete g_installations; - g_installations = nullptr; - } - } + DeleteInternal(); app_ = nullptr; } @@ -138,33 +119,59 @@ void Installations::DeleteInternal() { MutexLock lock(InstallationsMutex()); if (!installations_internal_) return; + // An instance whose initialization failed was never registered with the + // App's CleanupNotifier, added to InstallationsByApp(), or left holding + // JNI/iOS class references. + if (!installations_internal_->Initialized()) { + delete installations_internal_; + installations_internal_ = nullptr; + return; + } + + CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app_); + if (notifier) { + notifier->UnregisterObject(this); + } + installations_internal_->Cleanup(); delete installations_internal_; installations_internal_ = nullptr; + + // Remove `this` from InstallationsByApp() on both App-destruction cleanup and + // ~Installations() so a recreated App at the same address never finds a + // gutted Installations shell. + InstallationsByApp().erase(app_); } Future Installations::GetId() { - return installations_internal_->GetId(); + return installations_internal_ ? installations_internal_->GetId() + : Future(); } Future Installations::GetIdLastResult() { - return installations_internal_->GetIdLastResult(); + return installations_internal_ ? installations_internal_->GetIdLastResult() + : Future(); } Future Installations::GetToken(bool forceRefresh) { - return installations_internal_->GetToken(forceRefresh); + return installations_internal_ + ? installations_internal_->GetToken(forceRefresh) + : Future(); } Future Installations::GetTokenLastResult() { - return installations_internal_->GetTokenLastResult(); + return installations_internal_ ? installations_internal_->GetTokenLastResult() + : Future(); } Future Installations::Delete() { - return installations_internal_->Delete(); + return installations_internal_ ? installations_internal_->Delete() + : Future(); } Future Installations::DeleteLastResult() { - return installations_internal_->DeleteLastResult(); + return installations_internal_ ? installations_internal_->DeleteLastResult() + : Future(); } } // namespace installations diff --git a/installations/tests/installations_test.cc b/installations/tests/installations_test.cc index 247be8ac69..ed06b70664 100644 --- a/installations/tests/installations_test.cc +++ b/installations/tests/installations_test.cc @@ -144,5 +144,35 @@ TEST_F(InstallationsTest, GetTokenForceRefresh) { delete installations; } +TEST_F(InstallationsTest, DestroyAppBeforeInstallationsClearsCachedInstance) { + Installations* installations1 = Installations::GetInstance(firebase_app_); + ASSERT_THAT(installations1, NotNull()); + CheckSuccessWithValue(installations1->GetId(), std::string("FakeId")); + + // Deleting the App first runs CleanupNotifier -> DeleteInternal(), which + // tears down installations_internal_ and removes the entry from the per-App + // map so a recreated App cannot observe a gutted Installations shell. + delete firebase_app_; + firebase_app_ = nullptr; + EXPECT_EQ(installations1->GetId().status(), kFutureStatusInvalid); + + firebase_app_ = testing::CreateApp(); + ASSERT_THAT(firebase_app_, NotNull()); + + Installations* installations2 = Installations::GetInstance(firebase_app_); + ASSERT_THAT(installations2, NotNull()); + EXPECT_NE(installations2, installations1); + CheckSuccessWithValue(installations2->GetId(), std::string("FakeId")); + + // Deleting the already-cleaned-up installations1 (for example from a + // late-running static std::unique_ptr destructor) must be a no-op and must + // not evict installations2 even if the allocator reused the App* address. + delete installations1; + EXPECT_EQ(Installations::GetInstance(firebase_app_), installations2); + CheckSuccessWithValue(installations2->GetId(), std::string("FakeId")); + + delete installations2; +} + } // namespace installations } // namespace firebase diff --git a/messaging/CMakeLists.txt b/messaging/CMakeLists.txt index a45ace818e..84d622d081 100644 --- a/messaging/CMakeLists.txt +++ b/messaging/CMakeLists.txt @@ -101,6 +101,12 @@ target_compile_definitions(firebase_messaging -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_messaging PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(messaging) diff --git a/messaging/src/android/cpp/messaging.cc b/messaging/src/android/cpp/messaging.cc index 74a8f2ea46..bdf26f33ff 100644 --- a/messaging/src/android/cpp/messaging.cc +++ b/messaging/src/android/cpp/messaging.cc @@ -35,6 +35,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/reference_counted_future_impl.h" #include "app/src/util.h" #include "app/src/util_android.h" @@ -72,7 +73,10 @@ static const ::firebase::App* g_app = nullptr; // Mutex used to arbitrate access to g_app between the thread calling this // module and MessageProcessingThread(). -static Mutex g_app_mutex; +static Mutex& AppMutex() { + static NoDestructor app_mutex; + return *app_mutex; +} // Global reference to the Firebase Cloud Messaging instance. // This is initialized in messaging::Initialize() and never released @@ -126,7 +130,10 @@ static std::string* g_lockfile_path; static Mutex* g_file_locker_mutex; // Mutex for managing if FutureData is safe to use, or if it has been deleted. -static Mutex g_future_data_mutex; +static Mutex& FutureDataMutex() { + static NoDestructor future_data_mutex; + return *future_data_mutex; +} static const char* kMessagingNotInitializedError = "Messaging not initialized."; @@ -312,7 +319,7 @@ static void ConsumeEvents() { // thread knows that it is time to quit. static bool TerminateRequested() { bool result; - MutexLock lock(g_app_mutex); + MutexLock lock(AppMutex()); // If the app is removed, Terminate() has been called. result = !g_app; return result; @@ -352,7 +359,7 @@ Future RequestPermissionLastResult() { void ProcessMessages() { JNIEnv* env; { - MutexLock lock(g_app_mutex); + MutexLock lock(AppMutex()); env = g_app ? g_app->GetJNIEnv() : nullptr; } if (HasListener() && env) { @@ -370,7 +377,7 @@ void ProcessMessages() { static void* MessageProcessingThread(void*) { JavaVM* jvm; { - MutexLock lock(g_app_mutex); + MutexLock lock(AppMutex()); jvm = g_app ? g_app->java_vm() : nullptr; if (jvm == nullptr) return nullptr; } @@ -513,7 +520,7 @@ static void FireIntentMessage(JNIEnv* env) { g_intent_message_fired = true; jobject activity; { - MutexLock lock(g_app_mutex); + MutexLock lock(AppMutex()); if (!g_app) return; activity = env->NewLocalRef(g_app->activity()); assert(env->ExceptionCheck() == false); @@ -601,7 +608,7 @@ InitResult Initialize(const ::firebase::App& app, Listener* listener, } { - MutexLock lock(g_app_mutex); + MutexLock lock(AppMutex()); g_app = &app; } g_registration_token_mutex = new Mutex(); @@ -699,7 +706,7 @@ void Terminate() { util::CancelCallbacks(env, kApiIdentifier); // Dereference the app. { - MutexLock lock(g_app_mutex); + MutexLock lock(AppMutex()); g_app = nullptr; } TerminateMessageProcessingThread(); @@ -726,7 +733,7 @@ void Terminate() { ReleaseClasses(env); util::Terminate(env); { - MutexLock lock(g_future_data_mutex); + MutexLock lock(FutureDataMutex()); FutureData::Destroy(); } } @@ -853,7 +860,7 @@ static void CompleteVoidCallback(JNIEnv* env, jobject result, FutureHandle handle(future_id); Error error = (result_code == util::kFutureResultSuccess) ? kErrorNone : kErrorUnknown; - MutexLock lock(g_future_data_mutex); + MutexLock lock(FutureDataMutex()); if (FutureData::Get() && FutureData::Get()->api()) { ReferenceCountedFutureImpl* api = FutureData::Get()->api(); api->Complete(handle, error, status_message); @@ -875,7 +882,7 @@ static void CompleteStringCallback(JNIEnv* env, jobject result, SafeFutureHandle* handle = reinterpret_cast*>(callback_data); Error error = success ? kErrorNone : kErrorUnknown; - MutexLock lock(g_future_data_mutex); + MutexLock lock(FutureDataMutex()); if (FutureData::Get() && FutureData::Get()->api()) { ReferenceCountedFutureImpl* api = FutureData::Get()->api(); api->CompleteWithResult(*handle, error, status_message, result_value); diff --git a/messaging/src/common.cc b/messaging/src/common.cc index bd4c46fa01..3c62a029f7 100644 --- a/messaging/src/common.cc +++ b/messaging/src/common.cc @@ -22,6 +22,7 @@ #include "app/src/cleanup_notifier.h" #include "app/src/include/firebase/internal/common.h" #include "app/src/include/firebase/internal/mutex.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "messaging/src/include/firebase/messaging.h" @@ -44,7 +45,10 @@ FIREBASE_APP_REGISTER_CALLBACKS( namespace firebase { namespace messaging { -static Mutex g_listener_lock; // NOLINT +static Mutex& ListenerLock() { + static NoDestructor listener_lock; + return *listener_lock; +} static Listener* g_listener = nullptr; // Keep track of the most recent token received. @@ -93,7 +97,7 @@ void UnregisterTerminateOnDefaultAppDestroy() { } // namespace internal Listener* SetListener(Listener* listener) { - MutexLock lock(g_listener_lock); + MutexLock lock(ListenerLock()); Listener* previous_listener = g_listener; if (listener && !g_prev_token_received) { g_prev_token_received = new std::string; @@ -134,7 +138,7 @@ Listener* SetListener(Listener* listener) { // Determine whether a listener is present. bool HasListener() { - MutexLock lock(g_listener_lock); + MutexLock lock(ListenerLock()); return g_listener != nullptr; } @@ -143,12 +147,12 @@ Listener* SetListenerIfNotNull(Listener* listener) { } void NotifyListenerOnMessage(const Message& message) { - MutexLock lock(g_listener_lock); + MutexLock lock(ListenerLock()); if (g_listener) g_listener->OnMessage(message); } void NotifyListenerOnTokenReceived(const char* token) { - MutexLock lock(g_listener_lock); + MutexLock lock(ListenerLock()); // If we have a previous token that we've notified any listener about, check // to ensure no repeat. if (g_prev_token_received && *g_prev_token_received == token) { @@ -171,7 +175,7 @@ void NotifyListenerOnRegistrationReceived(const char* installationId) { if (!installationId || installationId[0] == '\0') { return; } - MutexLock lock(g_listener_lock); + MutexLock lock(ListenerLock()); if (!g_prev_registration_received) g_prev_registration_received = new std::string; *g_prev_registration_received = installationId; @@ -187,7 +191,7 @@ void NotifyListenerOnUnregistrationReceived(const char* installationId) { if (!installationId || installationId[0] == '\0') { return; } - MutexLock lock(g_listener_lock); + MutexLock lock(ListenerLock()); // If the unregistration is for a pending registration that we haven't sent to // a listener yet, clear the pending state. if (g_prev_registration_received && diff --git a/messaging/src/ios/messaging.mm b/messaging/src/ios/messaging.mm index 83d02305f8..c6ab195e5c 100644 --- a/messaging/src/ios/messaging.mm +++ b/messaging/src/ios/messaging.mm @@ -32,6 +32,7 @@ #include "app/src/include/firebase/internal/mutex.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/no_destructor.h" #include "app/src/util_ios.h" #include "messaging/src/common.h" @@ -79,11 +80,14 @@ + (NSString *)normalizeTopic:(NSString *)topic; DEFINE_FIREBASE_VERSION_STRING(FirebaseMessaging); static FIRCppDelegate *g_delegate = nil; -static Mutex g_delegate_mutex; // Mutex for g_delegate's properties +static Mutex& DelegateMutex() { + static NoDestructor delegate_mutex; // Mutex for g_delegate's properties + return *delegate_mutex; +} static MessagingOptions g_messaging_options; static bool MessagingIsInitialized() { - MutexLock lock(g_delegate_mutex); + MutexLock lock(DelegateMutex()); return g_delegate.isListenerSet; } @@ -126,7 +130,10 @@ static bool MessagingIsInitialized() { static bool g_message_notification_opened = false; -static SafeFutureHandle g_permission_prompt_future_handle; +static SafeFutureHandle& PermissionPromptFutureHandle() { + static NoDestructor> handle; + return *handle; +} static void RetrieveRegistrationToken(); @@ -150,7 +157,7 @@ InitResult Initialize( if (!g_app) { #if !defined(NDEBUG) { - MutexLock lock(g_delegate_mutex); + MutexLock lock(DelegateMutex()); assert(!g_delegate.isListenerSet); } #endif // !defined(NDEBUG) @@ -171,7 +178,7 @@ void NotifyListenerSet(Listener* listener) { // If no listener is specified, or the API is already initialized, do nothing. if (!listener) return; { - MutexLock lock(g_delegate_mutex); + MutexLock lock(DelegateMutex()); if (g_delegate.isListenerSet) return; g_delegate.isListenerSet = YES; } @@ -190,7 +197,7 @@ void NotifyListenerSet(Listener* listener) { // Create the future. ReferenceCountedFutureImpl* api = FutureData::Get()->api(); - g_permission_prompt_future_handle = + PermissionPromptFutureHandle() = api->SafeAlloc(kMessagingFnRequestPermission); // Run on UI thread. dispatch_async(dispatch_get_main_queue(), ^{ @@ -250,7 +257,7 @@ void NotifyListenerSet(Listener* listener) { g_launch_notification = nil; } }); - return MakeFuture(api, g_permission_prompt_future_handle); + return MakeFuture(api, PermissionPromptFutureHandle()); } Future RequestPermissionLastResult() { @@ -280,7 +287,7 @@ void Terminate() { // Ensure g_delegate still exists (it cannot be unallocated in case a token is received via a // different Firebase library e.g. Invites while Messaging is not enabled). { - MutexLock lock(g_delegate_mutex); + MutexLock lock(DelegateMutex()); g_delegate.isListenerSet = NO; g_delegate.cachedMessaging = nil; g_delegate.cachedFCMToken = nil; @@ -344,9 +351,9 @@ static void CompletePermissionFuture(::firebase::messaging::Error error) { ::firebase::ReferenceCountedFutureImpl* api = ::firebase::messaging::FutureData::Get()->api(); // The ReferenceCountedFutureImpl object should be initialized in Initialize(). FIREBASE_ASSERT_RETURN_VOID(api != nullptr); - if (api->ValidFuture(::firebase::messaging::g_permission_prompt_future_handle) && + if (api->ValidFuture(::firebase::messaging::PermissionPromptFutureHandle()) && RequestPermissionLastResult().status() == kFutureStatusPending) { - api->Complete(::firebase::messaging::g_permission_prompt_future_handle, error); + api->Complete(::firebase::messaging::PermissionPromptFutureHandle(), error); } } else { LogError("Attempting to complete future before FCM initialized."); @@ -974,30 +981,30 @@ - (void)userNotificationCenter:(UNUserNotificationCenter *)notificationCenter @implementation FIRCppDelegate - (void)messaging:(FIRMessaging *)messaging didReceiveRegistrationToken:(NSString *)fcmToken { - ::firebase::messaging::g_delegate_mutex.Acquire(); + ::firebase::messaging::DelegateMutex().Acquire(); if (_isListenerSet) { - ::firebase::messaging::g_delegate_mutex.Release(); + ::firebase::messaging::DelegateMutex().Release(); ::firebase::LogInfo("FCM: new registration token received."); ::firebase::messaging::RetrieveRegistrationToken(); } else { _cachedMessaging = messaging; _cachedFCMToken = fcmToken; - ::firebase::messaging::g_delegate_mutex.Release(); + ::firebase::messaging::DelegateMutex().Release(); ::firebase::LogInfo( "FCM: registration token received, but no listener set yet - cached the token."); } } - (void)messaging:(FIRMessaging *)messaging didReceiveRegistration:(NSString *)installationId { - ::firebase::messaging::g_delegate_mutex.Acquire(); + ::firebase::messaging::DelegateMutex().Acquire(); if (_isListenerSet) { - ::firebase::messaging::g_delegate_mutex.Release(); + ::firebase::messaging::DelegateMutex().Release(); ::firebase::LogInfo("FCM: registration installation ID received."); ::firebase::messaging::NotifyListenerOnRegistrationReceived(installationId.UTF8String); } else { _cachedMessaging = messaging; _cachedRegistrationId = installationId; - ::firebase::messaging::g_delegate_mutex.Release(); + ::firebase::messaging::DelegateMutex().Release(); ::firebase::LogInfo( "FCM: registration installation ID received, but no listener set yet - cached the ID."); } @@ -1013,7 +1020,7 @@ - (void)processCachedRegistrationToken { NSString *token; NSString *regId; { - firebase::MutexLock lock(::firebase::messaging::g_delegate_mutex); + firebase::MutexLock lock(::firebase::messaging::DelegateMutex()); // TODO(butterfield): What if there's no cached message but there is a cached token? // The user may never get notified of the old token before a new one is registered. if (!_isListenerSet) { diff --git a/messaging/src/stub/messaging.cc b/messaging/src/stub/messaging.cc index c07b69a4f8..75b735bb3e 100644 --- a/messaging/src/stub/messaging.cc +++ b/messaging/src/stub/messaging.cc @@ -37,6 +37,7 @@ #include #include "app/src/include/firebase/internal/mutex.h" +#include "app/src/no_destructor.h" #endif // FIREBASE_MESSAGING_TEST_ENABLED namespace firebase { @@ -48,7 +49,10 @@ static bool g_initialized = false; #if FIREBASE_MESSAGING_TEST_ENABLED static pthread_t g_message_thread; -static Mutex g_message_thread_mutex; // NOLINT +static Mutex& MessageThreadMutex() { + static NoDestructor message_thread_mutex; + return *message_thread_mutex; +} static bool g_message_thread_run; static const int kMessageIntervalMilliseconds = 5000; #endif // FIREBASE_MESSAGING_TEST_ENABLED @@ -61,7 +65,7 @@ static void* MessageTestThread(void* arg) { int time_elapsed_milliseconds = 0; for (;;) { { - MutexLock lock(g_message_thread_mutex); + MutexLock lock(MessageThreadMutex()); if (!g_message_thread_run) break; } // Wait until kMessageIntervalMilliseconds has elapsed. @@ -103,7 +107,7 @@ InitResult Initialize(const ::firebase::App& /*app*/, Listener* listener, FutureData::Create(); #if FIREBASE_MESSAGING_TEST_ENABLED { - MutexLock lock(g_message_thread_mutex); + MutexLock lock(MessageThreadMutex()); if (g_message_thread_run) return kInitResultSuccess; g_message_thread_run = true; pthread_attr_t attr; @@ -129,7 +133,7 @@ void Terminate() { internal::UnregisterTerminateOnDefaultAppDestroy(); #if FIREBASE_MESSAGING_TEST_ENABLED { - MutexLock lock(g_message_thread_mutex); + MutexLock lock(MessageThreadMutex()); if (!g_message_thread_run) return; g_message_thread_run = false; } diff --git a/release_build_files/readme.md b/release_build_files/readme.md index f4eb8f5c39..ed1c60dfa8 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -615,9 +615,10 @@ code. ## Release Notes ### Upcoming - Changes - - General: Fixed a crash at process exit when an `Auth`, `RemoteConfig`, - `Database`, `Storage`, `Functions`, `Installations` or `AppCheck` - instance is owned by a static object (for example a static + - General: Fixed a crash at process exit when an `App` or Firebase product + instance (`Analytics`, `AppCheck`, `Auth`, `Database`, `Firestore`, + `Functions`, `Installations`, `Messaging`, `RemoteConfig`, `Storage`, or + `UMP`) is owned by a static object (for example a static `std::unique_ptr`) and destroyed after the SDK's own globals. ([#110](https://github.com/firebase/firebase-cpp-sdk/issues/110)) - Database, Functions, Storage (Android): Fixed a crash in `GetInstance()` diff --git a/remote_config/CMakeLists.txt b/remote_config/CMakeLists.txt index cf48e8d85e..582297427e 100644 --- a/remote_config/CMakeLists.txt +++ b/remote_config/CMakeLists.txt @@ -140,6 +140,12 @@ target_compile_definitions(firebase_remote_config -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_remote_config PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(remote_config) elseif(IOS) diff --git a/remote_config/src/android/remote_config_android.cc b/remote_config/src/android/remote_config_android.cc index 66cf18c821..55bf14ffbc 100644 --- a/remote_config/src/android/remote_config_android.cc +++ b/remote_config/src/android/remote_config_android.cc @@ -25,6 +25,7 @@ #include "app/src/embedded_file.h" #include "app/src/include/firebase/future.h" #include "app/src/include/firebase/version.h" +#include "app/src/no_destructor.h" #include "app/src/reference_counted_future_impl.h" #include "app/src/time.h" #include "app/src/util.h" @@ -254,7 +255,10 @@ static const ValueSource kFirebaseRemoteConfigSourceToValueSourceMap[] = { kValueSourceRemoteValue, // FirebaseRemoteConfig.VALUE_SOURCE_REMOTE (2) }; -ReferenceCount RemoteConfigInternal::initializer_; // NOLINT +static ReferenceCount& Initializer() { + static NoDestructor initializer; + return *initializer; +} static bool CacheJNIMethodIds( JNIEnv* env, jobject activity, @@ -672,8 +676,8 @@ struct ConfigUpdateListenerWrapper { }; RemoteConfigInternal::RemoteConfigInternal(const firebase::App& app) - : app_(app), future_impl_(kRemoteConfigFnCount) { - ReferenceCountLock lock(&initializer_); + : app_(app), future_impl_(kRemoteConfigFnCount), internal_obj_(nullptr) { + ReferenceCountLock lock(&Initializer()); LogDebug("Firebase RemoteConfig API Initializing"); static const char* kApiIdentifier = "Remote Config"; jni_task_id_ = CreateApiIdentifier(kApiIdentifier, this); @@ -731,7 +735,7 @@ bool RemoteConfigInternal::Initialized() const { } void RemoteConfigInternal::Cleanup() { - ReferenceCountLock lock(&initializer_); + ReferenceCountLock lock(&Initializer()); if (lock.RemoveReference() == 1) { JNIEnv* env = app_.GetJNIEnv(); ReleaseClasses(env); diff --git a/remote_config/src/android/remote_config_android.h b/remote_config/src/android/remote_config_android.h index bf4a0e9489..5b32355e25 100644 --- a/remote_config/src/android/remote_config_android.h +++ b/remote_config/src/android/remote_config_android.h @@ -113,7 +113,6 @@ class RemoteConfigInternal { } private: - static firebase::internal::ReferenceCount initializer_; // app const firebase::App& app_; diff --git a/remote_config/src/remote_config.cc b/remote_config/src/remote_config.cc index 0e9805905a..0494f11ea2 100644 --- a/remote_config/src/remote_config.cc +++ b/remote_config/src/remote_config.cc @@ -97,12 +97,11 @@ RemoteConfig* RemoteConfig::GetInstance(App* app) { rc->DeleteInternal(); }); - // Stick it in the global map so we remember it, and can delete it on - // shutdown. RemoteConfigs()[app] = rc; return rc; } + delete rc; return nullptr; } @@ -115,6 +114,15 @@ void RemoteConfig::DeleteInternal() { MutexLock lock(RemoteConfigsMutex()); if (!internal_) return; + // An instance whose initialization failed was never registered with the + // App's CleanupNotifier, added to RemoteConfigs(), or left holding JNI/iOS + // class references. + if (!internal_->Initialized()) { + delete internal_; + internal_ = nullptr; + return; + } + CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app()); assert(notifier); notifier->UnregisterObject(this); diff --git a/storage/CMakeLists.txt b/storage/CMakeLists.txt index 147bfdc18e..dcfb0efa91 100644 --- a/storage/CMakeLists.txt +++ b/storage/CMakeLists.txt @@ -111,6 +111,12 @@ target_compile_definitions(firebase_storage ${additional_DEFINES} ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_storage PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(storage) elseif(IOS) diff --git a/storage/src/android/storage_android.cc b/storage/src/android/storage_android.cc index 3619794a55..457d3b3e60 100644 --- a/storage/src/android/storage_android.cc +++ b/storage/src/android/storage_android.cc @@ -22,6 +22,7 @@ #include "app/src/embedded_file.h" #include "app/src/include/firebase/app.h" #include "app/src/include/firebase/internal/common.h" +#include "app/src/no_destructor.h" #include "app/src/util.h" #include "app/src/util_android.h" #include "storage/src/android/controller_android.h" @@ -136,7 +137,10 @@ METHOD_LOOKUP_DEFINITION( "com/google/firebase/storage/internal/cpp/CppByteUploader", CPP_BYTE_UPLOADER_METHODS) -Mutex StorageInternal::init_mutex_; // NOLINT +static Mutex& InitMutex() { + static NoDestructor init_mutex; + return *init_mutex; +} int StorageInternal::initialize_count_ = 0; std::map* StorageInternal::java_error_to_cpp_ = nullptr; @@ -206,7 +210,7 @@ static const struct { }; bool StorageInternal::Initialize(App* app) { - MutexLock init_lock(init_mutex_); + MutexLock init_lock(InitMutex()); if (initialize_count_ == 0) { JNIEnv* env = app->GetJNIEnv(); jobject activity = app->activity(); @@ -283,7 +287,7 @@ bool StorageInternal::InitializeEmbeddedClasses(App* app) { } void StorageInternal::Terminate(App* app) { - MutexLock init_lock(init_mutex_); + MutexLock init_lock(InitMutex()); FIREBASE_ASSERT_RETURN_VOID(initialize_count_ > 0); initialize_count_--; if (initialize_count_ == 0) { diff --git a/storage/src/android/storage_android.h b/storage/src/android/storage_android.h index b25fa42e03..d8dfdf1641 100644 --- a/storage/src/android/storage_android.h +++ b/storage/src/android/storage_android.h @@ -127,7 +127,6 @@ class StorageInternal { // Initialize classes loaded from embedded files. static bool InitializeEmbeddedClasses(App* app); - static Mutex init_mutex_; static int initialize_count_; static std::map* java_error_to_cpp_; diff --git a/storage/src/common/list_result.cc b/storage/src/common/list_result.cc index 1aafac0b47..04b3fdce24 100644 --- a/storage/src/common/list_result.cc +++ b/storage/src/common/list_result.cc @@ -14,6 +14,7 @@ #include "firebase/storage/list_result.h" +#include "app/src/no_destructor.h" #include "list_result_internal.h" namespace firebase { @@ -59,18 +60,18 @@ StorageListResult& StorageListResult::operator=(StorageListResult&& other) { #endif // defined(FIREBASE_USE_MOVE_OPERATORS) || defined(DOXYGEN) const std::vector& StorageListResult::prefixes() const { - static const std::vector empty_vector; - return internal_ ? internal_->prefixes_ : empty_vector; + static const NoDestructor> empty_vector; + return internal_ ? internal_->prefixes_ : *empty_vector; } const std::vector& StorageListResult::items() const { - static const std::vector empty_vector; - return internal_ ? internal_->items_ : empty_vector; + static const NoDestructor> empty_vector; + return internal_ ? internal_->items_ : *empty_vector; } const std::string& StorageListResult::next_page_token() const { - static const std::string empty_string; - return internal_ ? internal_->next_page_token_ : empty_string; + static const NoDestructor empty_string; + return internal_ ? internal_->next_page_token_ : *empty_string; } bool StorageListResult::is_valid() const { return internal_ != nullptr; } diff --git a/storage/src/common/storage.cc b/storage/src/common/storage.cc index 25b06ac3f5..fbd5917133 100644 --- a/storage/src/common/storage.cc +++ b/storage/src/common/storage.cc @@ -53,17 +53,23 @@ namespace storage { DEFINE_FIREBASE_VERSION_STRING(FirebaseStorage); -// A function-local static wrapped in NoDestructor, so it is constructed on -// first use and never destroyed. That keeps it out of static initialization -// and destruction entirely: it is ready no matter which translation unit's -// initializers run first, and a Storage owned by a static object can still -// lock it from its destructor after this file's globals would otherwise have -// been torn down by the C++ runtime. +using StorageMap = std::map, Storage*>; + +// The map and its mutex are function-local statics wrapped in NoDestructor, so +// they are constructed on first use and never destroyed. That keeps them out +// of static initialization and destruction entirely: they are ready no matter +// which translation unit's initializers run first, and a Storage owned by a +// static object can still lock the mutex and erase itself from its destructor +// during process exit. static Mutex& StoragesLock() { static NoDestructor mutex; return *mutex; } -static std::map, Storage*>* g_storages = nullptr; + +static StorageMap& Storages() { + static NoDestructor storages; + return *storages; +} Storage* Storage::GetInstance(::firebase::App* app, InitResult* init_result_out) { @@ -73,9 +79,6 @@ Storage* Storage::GetInstance(::firebase::App* app, Storage* Storage::GetInstance(::firebase::App* app, const char* url, InitResult* init_result_out) { MutexLock lock(StoragesLock()); - if (!g_storages) { - g_storages = new std::map, Storage*>(); - } // URL we use for our global index of Storage instances. // If a null URL is given, we look up the App's default storage bucket. std::string url_idx; @@ -106,9 +109,9 @@ Storage* Storage::GetInstance(::firebase::App* app, const char* url, return nullptr; } - std::map, Storage*>::iterator it = - g_storages->find(std::make_pair(app, url_idx)); - if (it != g_storages->end()) { + StorageMap& storages = Storages(); + StorageMap::iterator it = storages.find(std::make_pair(app, url_idx)); + if (it != storages.end()) { if (init_result_out != nullptr) *init_result_out = kInitResultSuccess; return it->second; } @@ -119,7 +122,7 @@ Storage* Storage::GetInstance(::firebase::App* app, const char* url, delete storage; return nullptr; } - g_storages->insert(std::make_pair(std::make_pair(app, url_idx), storage)); + storages.insert(std::make_pair(std::make_pair(app, url_idx), storage)); if (init_result_out) *init_result_out = kInitResultSuccess; return storage; } @@ -147,7 +150,7 @@ void Storage::DeleteInternal() { if (!internal_) return; // An instance whose initialization failed was never registered with the - // App's CleanupNotifier or added to g_storages, and app() may be null. + // App's CleanupNotifier or added to Storages(), and app() may be null. if (!internal_->initialized()) { delete internal_; internal_ = nullptr; @@ -165,14 +168,9 @@ void Storage::DeleteInternal() { ? std::string(internal::kCloudStorageScheme) + app()->options().storage_bucket() : url(); - g_storages->erase(std::make_pair(app(), url_idx)); + Storages().erase(std::make_pair(app(), url_idx)); delete internal_; internal_ = nullptr; - // If it's the last one, delete the map. - if (g_storages->empty()) { - delete g_storages; - g_storages = nullptr; - } } Storage::~Storage() { DeleteInternal(); } diff --git a/ump/CMakeLists.txt b/ump/CMakeLists.txt index 9442b1e069..aad295ee38 100644 --- a/ump/CMakeLists.txt +++ b/ump/CMakeLists.txt @@ -77,6 +77,12 @@ target_compile_definitions(firebase_ump -DINTERNAL_EXPERIMENTAL=1 ) +# Reject static/global C++ objects with non-trivial destructors so SDK state +# cannot be destroyed before user-owned static Firebase objects at process exit. +target_compile_options( + firebase_ump PRIVATE + $<$,$>:-Werror=exit-time-destructors>) + if(ANDROID) firebase_cpp_proguard_file(ump) elseif(IOS) diff --git a/ump/src/android/consent_info_internal_android.cc b/ump/src/android/consent_info_internal_android.cc index 9c179679e9..a82174c428 100644 --- a/ump/src/android/consent_info_internal_android.cc +++ b/ump/src/android/consent_info_internal_android.cc @@ -23,6 +23,7 @@ #include #include "app/src/assert.h" +#include "app/src/no_destructor.h" #include "app/src/thread.h" #include "app/src/util_android.h" #include "firebase/internal/common.h" @@ -33,9 +34,16 @@ namespace ump { namespace internal { ConsentInfoInternalAndroid* ConsentInfoInternalAndroid::s_instance = nullptr; -firebase::Mutex ConsentInfoInternalAndroid::s_instance_mutex; -::firebase::Mutex g_cached_ump_embedded_files_mutex; +static firebase::Mutex& InstanceMutex() { + static NoDestructor instance_mutex; + return *instance_mutex; +} + +::firebase::Mutex& CachedUmpEmbeddedFilesMutex() { + static NoDestructor<::firebase::Mutex> cached_ump_embedded_files_mutex; + return *cached_ump_embedded_files_mutex; +} std::vector<::firebase::internal::EmbeddedFile>* g_cached_ump_embedded_files = nullptr; @@ -161,7 +169,7 @@ ConsentInfoInternalAndroid::~ConsentInfoInternalAndroid() { env->CallVoidMethod(helper_, consent_info_helper::GetMethodId( consent_info_helper::kDisconnect)); - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); s_instance = nullptr; env->DeleteGlobalRef(helper_); @@ -230,7 +238,7 @@ void ConsentInfoInternalAndroid::CacheEnumValues(JNIEnv* env) { void ConsentInfoInternalAndroid::JNI_ConsentInfoHelper_completeFuture( JNIEnv* env, jclass clazz, jint future_fn, jlong consent_info_internal_ptr, jlong future_handle, jint error_code, jobject error_message_obj) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (consent_info_internal_ptr == 0 || s_instance == nullptr) { // Calling this with a null pointer, or if there is no active // instance, is a no-op, so just return. @@ -259,7 +267,7 @@ ConsentInfoInternalAndroid::ConsentInfoInternalAndroid(JNIEnv* env, activity_(nullptr), helper_(nullptr), has_requested_consent_info_update_(false) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); FIREBASE_ASSERT(s_instance == nullptr); s_instance = this; @@ -268,8 +276,7 @@ ConsentInfoInternalAndroid::ConsentInfoInternalAndroid(JNIEnv* env, // Ensure we only load these files once. { - MutexLock lock( - ::firebase::ump::internal::g_cached_ump_embedded_files_mutex); + MutexLock lock(::firebase::ump::internal::CachedUmpEmbeddedFilesMutex()); if (::firebase::ump::internal::g_cached_ump_embedded_files == nullptr) { ::firebase::ump::internal::g_cached_ump_embedded_files = new std::vector(); diff --git a/ump/src/android/consent_info_internal_android.h b/ump/src/android/consent_info_internal_android.h index f5ab3b8916..ee79652240 100644 --- a/ump/src/android/consent_info_internal_android.h +++ b/ump/src/android/consent_info_internal_android.h @@ -48,7 +48,7 @@ bool RegisterNatives(); void ReleaseClasses(JNIEnv* env); namespace internal { -extern ::firebase::Mutex g_cached_ump_embedded_files_mutex; +::firebase::Mutex& CachedUmpEmbeddedFilesMutex(); extern std::vector<::firebase::internal::EmbeddedFile>* g_cached_ump_embedded_files; @@ -136,7 +136,6 @@ class ConsentInfoInternalAndroid : public ConsentInfoInternal { const EnumCache& enums() { return enums_; } static ConsentInfoInternalAndroid* s_instance; - static firebase::Mutex s_instance_mutex; EnumCache enums_; diff --git a/ump/src/ios/consent_info_internal_ios.h b/ump/src/ios/consent_info_internal_ios.h index f1db540258..69c46a0bda 100644 --- a/ump/src/ios/consent_info_internal_ios.h +++ b/ump/src/ios/consent_info_internal_ios.h @@ -50,7 +50,6 @@ class ConsentInfoInternalIos : public ConsentInfoInternal { private: static ConsentInfoInternalIos* s_instance; - static firebase::Mutex s_instance_mutex; static unsigned int s_instance_tag; void SetLoadedForm(UMPConsentForm *form) { diff --git a/ump/src/ios/consent_info_internal_ios.mm b/ump/src/ios/consent_info_internal_ios.mm index 437cf72833..f2217ba570 100644 --- a/ump/src/ios/consent_info_internal_ios.mm +++ b/ump/src/ios/consent_info_internal_ios.mm @@ -18,6 +18,7 @@ #include "ump/src/ios/consent_info_internal_ios.h" #include "app/src/assert.h" +#include "app/src/no_destructor.h" #include "app/src/thread.h" #include "app/src/util_ios.h" @@ -26,15 +27,19 @@ namespace internal { ConsentInfoInternalIos* ConsentInfoInternalIos::s_instance = nullptr; -firebase::Mutex ConsentInfoInternalIos::s_instance_mutex; unsigned int ConsentInfoInternalIos::s_instance_tag = 0; +static firebase::Mutex& InstanceMutex() { + static NoDestructor instance_mutex; + return *instance_mutex; +} + // This explicitly implements the constructor for the outer class, // ConsentInfoInternal. ConsentInfoInternal* ConsentInfoInternal::CreateInstance() { return new ConsentInfoInternalIos(); } ConsentInfoInternalIos::ConsentInfoInternalIos() : loaded_form_(nil) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); FIREBASE_ASSERT(s_instance == nullptr); s_instance = this; // Increment this with each created instance, to ensure that any leftover @@ -43,7 +48,7 @@ } ConsentInfoInternalIos::~ConsentInfoInternalIos() { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); s_instance = nullptr; } @@ -83,7 +88,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { Future ConsentInfoInternalIos::RequestConsentInfoUpdate( const ConsentRequestParameters& params) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (RequestConsentInfoUpdateLastResult().status() == kFutureStatusPending) { // This operation is already in progress. // Return a future with an error - this will not override the Fn entry. @@ -125,7 +130,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { callback_instance_tag = s_instance_tag; util::DispatchAsyncSafeMainQueue(^{ - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (!s_instance || s_instance_tag != callback_instance_tag) { // Instance changed or was invalidated, don't call the iOS method any more. return; @@ -133,7 +138,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { [UMPConsentInformation.sharedInstance requestConsentInfoUpdateWithParameters:ios_parameters completionHandler:^(NSError* _Nullable error) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (s_instance && s_instance_tag == callback_instance_tag) { if (!error) { CompleteFuture(handle, kConsentRequestSuccess); @@ -182,7 +187,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { } Future ConsentInfoInternalIos::LoadConsentForm() { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (LoadConsentFormLastResult().status() == kFutureStatusPending) { // This operation is already in progress. // Return a future with an error - this will not override the Fn entry. @@ -198,14 +203,14 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { callback_instance_tag = s_instance_tag; util::DispatchAsyncSafeMainQueue(^{ - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (!s_instance || s_instance_tag != callback_instance_tag) { // Instance changed or was invalidated, don't call the iOS method any more. return; } [UMPConsentForm loadWithCompletionHandler:^(UMPConsentForm* _Nullable form, NSError* _Nullable error) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (s_instance && s_instance_tag == callback_instance_tag) { if (form) { SetLoadedForm(form); @@ -223,7 +228,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { } Future ConsentInfoInternalIos::ShowConsentForm(FormParent parent) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (ShowConsentFormLastResult().status() == kFutureStatusPending) { // This operation is already in progress. // Return a future with an error - this will not override the Fn entry. @@ -241,14 +246,14 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { callback_instance_tag = s_instance_tag; util::DispatchAsyncSafeMainQueue(^{ - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (!s_instance || s_instance_tag != callback_instance_tag) { // Instance changed or was invalidated, don't call the iOS method any more. return; } [loaded_form_ presentFromViewController:parent completionHandler:^(NSError* _Nullable error) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (s_instance && s_instance_tag == callback_instance_tag) { if (!error) { CompleteFuture(handle, kConsentRequestSuccess); @@ -264,7 +269,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { } Future ConsentInfoInternalIos::LoadAndShowConsentFormIfRequired(FormParent parent) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (LoadAndShowConsentFormIfRequiredLastResult().status() == kFutureStatusPending) { // This operation is already in progress. // Return a future with an error - this will not override the Fn entry. @@ -279,7 +284,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { callback_instance_tag = s_instance_tag; util::DispatchAsyncSafeMainQueue(^{ - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (!s_instance || s_instance_tag != callback_instance_tag) { // Instance changed or was invalidated, don't call the iOS method any more. return; @@ -287,7 +292,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { [UMPConsentForm loadAndPresentIfRequiredFromViewController:parent completionHandler:^(NSError* _Nullable error) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (s_instance && s_instance_tag == callback_instance_tag) { if (!error) { CompleteFuture(handle, kConsentRequestSuccess); @@ -320,7 +325,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { } Future ConsentInfoInternalIos::ShowPrivacyOptionsForm(FormParent parent) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (ShowPrivacyOptionsFormLastResult().status() == kFutureStatusPending) { // This operation is already in progress. // Return a future with an error - this will not override the Fn entry. @@ -334,7 +339,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { callback_instance_tag = s_instance_tag; util::DispatchAsyncSafeMainQueue(^{ - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (!s_instance || s_instance_tag != callback_instance_tag) { // Instance changed or was invalidated, don't call the iOS method any more. return; @@ -342,7 +347,7 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) { [UMPConsentForm presentPrivacyOptionsFormFromViewController:parent completionHandler:^(NSError* _Nullable error) { - MutexLock lock(s_instance_mutex); + MutexLock lock(InstanceMutex()); if (s_instance && s_instance_tag == callback_instance_tag) { if (!error) { CompleteFuture(handle, kConsentRequestSuccess);