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 new file mode 100644 index 0000000000..aedde395b8 --- /dev/null +++ b/app/src/no_destructor.h @@ -0,0 +1,98 @@ +/* + * 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 +#include + +namespace firebase { +namespace internal { + +// Wraps an instance of T in inline aligned storage whose destructor is never +// called. +// +// 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; +// } +template +class NoDestructor { + public: + // Constructs the wrapped T in place, forwarding any arguments. + // 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 (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; + + // Trivially destructible: intentionally never destroys the wrapped T. + ~NoDestructor() = default; + + T& operator*() { return *get(); } + T* operator->() { return get(); } + 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 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 65d5949c03..88f98fba4e 100644 --- a/app/tests/CMakeLists.txt +++ b/app/tests/CMakeLists.txt @@ -161,6 +161,21 @@ 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_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 new file mode 100644 index 0000000000..15e3b794a2 --- /dev/null +++ b/app/tests/no_destructor_test.cc @@ -0,0 +1,136 @@ +/* + * 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 +#include +#include + +#include "app/src/include/firebase/variant.h" +#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; + +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"); + 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, 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; + { + 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, 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"); + EXPECT_EQ(str->size(), 5u); + EXPECT_EQ(str.get(), &*str); +} + +} // namespace +} // namespace firebase 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 4e3c065488..d57c958f98 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,8 +62,23 @@ namespace app_check { DEFINE_FIREBASE_VERSION_STRING(FirebaseAppCheck); -Mutex g_app_check_lock; // NOLINT -static std::map<::firebase::App*, AppCheck*>* g_app_check_map = nullptr; +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 AppCheckMap& AppChecks() { + static NoDestructor app_checks; + return *app_checks; +} // Define the destructors for the virtual listener/provider/factory classes. AppCheckListener::~AppCheckListener() {} @@ -86,12 +102,11 @@ namespace internal { AppCheck* GetExistingAppCheckInstance(::firebase::App* app) { if (!app) return nullptr; - 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()) { - return it->second; - } + MutexLock lock(AppCheckLock()); + AppCheckMap& app_checks = AppChecks(); + auto it = app_checks.find(app); + if (it != app_checks.end()) { + return it->second; } return nullptr; } @@ -101,18 +116,15 @@ AppCheck* GetExistingAppCheckInstance(::firebase::App* app) { AppCheck* AppCheck::GetInstance(::firebase::App* app) { if (!app) return nullptr; - MutexLock lock(g_app_check_lock); - 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()) { + MutexLock lock(AppCheckLock()); + 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; } @@ -127,19 +139,14 @@ AppCheck::AppCheck(::firebase::App* app) { AppCheck::~AppCheck() { DeleteInternal(); } void AppCheck::DeleteInternal() { - MutexLock lock(g_app_check_lock); + MutexLock lock(AppCheckLock()); 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 b1417f63f1..e6b8d759e5 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" @@ -59,14 +60,41 @@ 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; -Mutex* g_auths_mutex = new Mutex(); +// +// 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 NoDestructor auths; + return *auths; +} + +static Mutex& AuthsMutex() { + static NoDestructor mutex; + 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(*g_auths_mutex); + 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. @@ -85,7 +113,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. - g_auths[app] = auth; + Auths()[app] = auth; if (init_result_out) *init_result_out = kInitResultSuccess; return auth; @@ -93,13 +121,8 @@ Auth* Auth::GetAuth(App* app, InitResult* init_result_out) { // static Auth* Auth::FindAuth(App* app) { - MutexLock lock(*g_auths_mutex); - // Return the Auth if it already exists. - std::map::iterator it = g_auths.find(app); - if (it != g_auths.end()) { - return it->second; - } - return nullptr; + MutexLock lock(AuthsMutex()); + return FindAuthLocked(app); } // Auth uses the pimpl mechanism to hide internal data types from the interface. @@ -129,7 +152,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; @@ -149,17 +172,15 @@ void Auth::DeleteInternal() { int num_auths_remaining = 0; { - // 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) { - if (it->second == this) { - LogDebug("Deleting Auth %p for App %p", this, it->first); - g_auths.erase(it); - break; - } + // Remove `this` from the Auths map by its owning App key. + AuthMap& auths = Auths(); + 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 = g_auths.size(); + num_auths_remaining = auths.size(); } auth_data_->ClearListeners(); 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 ac17b9382c..9677ec3967 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,8 +57,29 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseDatabase); using internal::DatabaseInternal; using DatabaseMap = std::map, Database*>; -Mutex g_databases_lock; // NOLINT -static DatabaseMap* g_databases = nullptr; + +// 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& 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) { @@ -71,14 +93,12 @@ Database* Database::GetInstance(App* app, const char* url, LogError("Database::GetInstance(): The app must not be null."); return nullptr; } - MutexLock lock(g_databases_lock); - if (!g_databases) { - g_databases = new DatabaseMap(); - } + MutexLock lock(DatabasesLock()); + 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; } @@ -90,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; } @@ -118,12 +138,12 @@ Database::Database(::firebase::App* app, internal::DatabaseInternal* internal) } void Database::DeleteInternal() { - MutexLock lock(g_databases_lock); + MutexLock lock(DatabasesLock()); 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; @@ -138,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 df262eeac5..da5aee6660 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,9 +55,23 @@ DEFINE_FIREBASE_VERSION_STRING(FirebaseFunctions); constexpr const char* kDefaultRegion = "us-central1"; -Mutex g_functions_lock; // NOLINT -static std::map, Functions*>* g_functions = - nullptr; +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 FunctionsMap& FunctionsByKey() { + static NoDestructor functions; + return *functions; +} Functions* Functions::GetInstance(::firebase::App* app, InitResult* init_result_out) { @@ -65,10 +80,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); - if (!g_functions) { - g_functions = new std::map, Functions*>(); - } + MutexLock lock(FunctionsLock()); // Region we use for our global index of Functions instances. // If a null region is given, we use the default region. @@ -79,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; } @@ -91,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; @@ -118,12 +131,12 @@ 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; // 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; @@ -138,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 792e7a846f..881c407687 100644 --- a/installations/src/installations.cc +++ b/installations/src/installations.cc @@ -18,21 +18,47 @@ #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 -Mutex g_installations_mutex; // NOLINT +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; +} using internal::InstallationsInternal; +// Must be called with InstallationsMutex() held. +static Installations* FindInstallationsLocked(App* app) { + InstallationsMap& installations = InstallationsByApp(); + InstallationsMap::iterator it = installations.find(app); + if (it != installations.end()) { + return it->second; + } + return nullptr; +} + 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); + 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. @@ -60,54 +86,28 @@ 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; } Installations* Installations::FindInstallations(App* app) { - MutexLock lock(g_installations_mutex); - 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; + MutexLock lock(InstallationsMutex()); + return FindInstallationsLocked(app); } +// Private constructor called only from GetInstance() while InstallationsMutex() +// is already held. Installations::Installations(App* app) : app_(app) { - MutexLock lock(g_installations_mutex); + FIREBASE_ASSERT(app != nullptr); installations_internal_ = new InstallationsInternal(*app); } Installations::~Installations() { - MutexLock lock(g_installations_mutex); - - CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app_); - if (notifier) { - notifier->UnregisterObject(this); - } - DeleteInternal(); - - if (g_installations) { - // Remove installations from the g_installations map. - g_installations->erase(app_); - if (g_installations->empty()) { - delete g_installations; - g_installations = nullptr; - } - } app_ = nullptr; } @@ -116,36 +116,62 @@ bool Installations::InitInternal() { } void Installations::DeleteInternal() { - MutexLock lock(g_installations_mutex); + 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 dceff0c6aa..6b6f39a182 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -615,6 +615,12 @@ code. ## Release Notes ### Upcoming - Changes + - 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)) - General (Linux Desktop): Fixed a potential 1-byte buffer underflow and file descriptor leak when reading `/etc/timezone` in `GetTimezone()`. - 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 0c6b0a3c27..0494f11ea2 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" @@ -39,13 +40,41 @@ namespace firebase { namespace remote_config { -std::map g_rcs; -Mutex g_rc_mutex; // NOLINT +// The global map of Apps to RemoteConfigs. +// +// 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 NoDestructor remote_configs; + return *remote_configs; +} + +static Mutex& RemoteConfigsMutex() { + static NoDestructor mutex; + 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(g_rc_mutex); + 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; } @@ -68,29 +97,32 @@ RemoteConfig* RemoteConfig::GetInstance(App* app) { rc->DeleteInternal(); }); - // Stick it in the global map so we remember it, and can delete it on - // shutdown. - g_rcs[app] = rc; + RemoteConfigs()[app] = rc; return rc; } + delete rc; return nullptr; } RemoteConfig* RemoteConfig::FindRemoteConfig(App* app) { - MutexLock lock(g_rc_mutex); - // Return the RemoteConfig if it already exists. - std::map::iterator it = g_rcs.find(app); - if (it != g_rcs.end()) { - return it->second; - } - return nullptr; + MutexLock lock(RemoteConfigsMutex()); + return FindRemoteConfigLocked(app); } void RemoteConfig::DeleteInternal() { - MutexLock lock(g_rc_mutex); + 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); @@ -99,8 +131,8 @@ void RemoteConfig::DeleteInternal() { delete internal_; internal_ = nullptr; - // Remove rc from the g_rcs map. - g_rcs.erase(app()); + // Remove rc from the RemoteConfigs map. + RemoteConfigs().erase(app()); } RemoteConfig::RemoteConfig(App* app) { 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 3c995042b1..fbd5917133 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,8 +53,23 @@ namespace storage { DEFINE_FIREBASE_VERSION_STRING(FirebaseStorage); -Mutex g_storages_lock; // NOLINT -static std::map, Storage*>* g_storages = nullptr; +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 StorageMap& Storages() { + static NoDestructor storages; + return *storages; +} Storage* Storage::GetInstance(::firebase::App* app, InitResult* init_result_out) { @@ -62,10 +78,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); - if (!g_storages) { - g_storages = new std::map, Storage*>(); - } + MutexLock lock(StoragesLock()); // 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; @@ -96,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; } @@ -109,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; } @@ -133,11 +146,11 @@ Storage::Storage(::firebase::App* app, const char* url) { } void Storage::DeleteInternal() { - MutexLock lock(g_storages_lock); + MutexLock lock(StoragesLock()); 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; @@ -155,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);