From d1b821579ba0856ee11e9753bea3b6d846ee2cc3 Mon Sep 17 00:00:00 2001 From: Qutrek <235843705+qutrek@users.noreply.github.com> Date: Fri, 2 Oct 2026 06:23:56 +0300 Subject: [PATCH] fix: Make live instances unusable after `deleteMMKV()` instead of crashing `MMKV::removeStorage()` closes and deletes the file's native MMKV instance, but every `HybridMMKV` of that file kept its raw pointer to it. Any later call used freed memory, and `createMMKV()`'s AppState listener makes one (`checkContentChanged()`) the next time the app comes to the foreground: a crash or a hang. `HybridMMKV` now keeps a registry of live instances. `deleteMMKV()` unlinks every instance of the file before deleting it: using one then throws, while `checkContentChanged()` and `trim()` (called by the listeners) do nothing, and `id` stays readable. Fixes #1082 --- README.md | 2 + example/__tests__/MMKV.harness.ts | 34 ++++++++ packages/react-native-mmkv/cpp/HybridMMKV.cpp | 86 ++++++++++++++++++- packages/react-native-mmkv/cpp/HybridMMKV.hpp | 25 +++++- .../cpp/HybridMMKVFactory.cpp | 3 + 5 files changed, 145 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 58deee06..8e912469 100644 --- a/README.md +++ b/README.md @@ -253,6 +253,8 @@ import { deleteMMKV } from 'react-native-mmkv' const wasDeleted = deleteMMKV('my-instance') ``` +Instances of a deleted storage that are still alive become unusable: using them throws. Create a new one with `createMMKV(...)`. + ### Log Level By default, MMKV logs at `Debug` level in debug builds and `Warning` level in release builds. You can override this at build time to control the verbosity of MMKV's native logs. diff --git a/example/__tests__/MMKV.harness.ts b/example/__tests__/MMKV.harness.ts index 88a6c101..e924ac45 100644 --- a/example/__tests__/MMKV.harness.ts +++ b/example/__tests__/MMKV.harness.ts @@ -485,6 +485,40 @@ describe('MMKV Configuration & Multiple Instances', () => { expect(storage2.getBoolean('key3')).toStrictEqual(true); }); + it('should make live instances unusable after deleteMMKV()', (context) => { + context.skip( + Platform.OS === 'web', + 'deleteMMKV does not invalidate instances on web', + ); + const storage = createMMKV({ id: 'deleted-live-instance' }); + const sameFile = createMMKV({ id: 'deleted-live-instance' }); + const other = createMMKV({ id: 'deleted-live-instance-other' }); + storage.set('key', 'value'); + other.set('key', 'value'); + + expect(deleteMMKV('deleted-live-instance')).toStrictEqual(true); + + // Every instance of the deleted file throws instead of using freed memory + expect(() => storage.getString('key')).toThrow(); + expect(() => sameFile.set('key', 'value')).toThrow(); + expect(() => other.importAllFrom(storage)).toThrow(); + // The AppState and memory warning listeners still call these + expect(() => storage.checkContentChanged()).not.toThrow(); + expect(() => storage.trim()).not.toThrow(); + expect(storage.id).toStrictEqual('deleted-live-instance'); + // Other files are not affected + expect(other.getString('key')).toStrictEqual('value'); + + // The id can be used again + const recreated = createMMKV({ id: 'deleted-live-instance' }); + expect(recreated.getString('key')).toBeUndefined(); + recreated.set('key', 'new'); + expect(recreated.getString('key')).toStrictEqual('new'); + + recreated.clearAll(); + other.clearAll(); + }); + it('should handle instance properties correctly', () => { const storage = createMMKV({ id: 'properties-test' }); diff --git a/packages/react-native-mmkv/cpp/HybridMMKV.cpp b/packages/react-native-mmkv/cpp/HybridMMKV.cpp index b6bcfed5..66fedcf4 100644 --- a/packages/react-native-mmkv/cpp/HybridMMKV.cpp +++ b/packages/react-native-mmkv/cpp/HybridMMKV.cpp @@ -13,7 +13,10 @@ namespace margelo::nitro::mmkv { -HybridMMKV::HybridMMKV(const Configuration& config) : HybridObject(TAG) { +std::mutex HybridMMKV::_liveInstancesMutex; +std::unordered_set HybridMMKV::_liveInstances; + +HybridMMKV::HybridMMKV(const Configuration& config) : HybridObject(TAG), _id(config.id), _rootPath(config.path.value_or("")) { MMKVMode mmkvMode = getMMKVMode(config); if (config.readOnly.value_or(false)) { mmkvMode = mmkvMode | MMKVMode::MMKV_READ_ONLY; @@ -36,7 +39,7 @@ HybridMMKV::HybridMMKV(const Configuration& config) : HybridObject(TAG) { Logger::log(LogLevel::Info, TAG, "Creating MMKV instance \"%s\"... (Path: %s, Encrypted: %s)", config.id.c_str(), rootPath.c_str(), hasEncryptionKey ? "true" : "false"); - instance = MMKV::mmkvWithID(config.id, mmkvConfig); + MMKV* instance = MMKV::mmkvWithID(config.id, mmkvConfig); if (instance == nullptr) [[unlikely]] { // Check if instanceId is invalid @@ -65,13 +68,61 @@ HybridMMKV::HybridMMKV(const Configuration& config) : HybridObject(TAG) { throw std::runtime_error("Failed to create MMKV instance!"); } + + _instance = instance; + std::lock_guard lock(_liveInstancesMutex); + _liveInstances.insert(this); +} + +HybridMMKV::~HybridMMKV() { + std::lock_guard lock(_liveInstancesMutex); + _liveInstances.erase(this); +} + +void HybridMMKV::invalidateInstances(const std::string& id, const std::string& rootPath) { + auto resolvedRoot = [](const std::string& path) -> const std::string& { return path.empty() ? MMKV::getRootDir() : path; }; + const std::string& root = resolvedRoot(rootPath); + + std::lock_guard lock(_liveInstancesMutex); + // 1. Find the native instance of that file. MMKV caches one per file, so every + // HybridMMKV of the file shares it (even one created with an equivalent path). + MMKV* deleted = nullptr; + for (HybridMMKV* hybrid : _liveInstances) { + if (hybrid->_id == id && resolvedRoot(hybrid->_rootPath) == root) { + deleted = hybrid->_instance.load(); + if (deleted != nullptr) { + break; + } + } + } + if (deleted == nullptr) { + // No live instance of this file. + return; + } + // 2. Unlink every HybridMMKV that uses it. + for (HybridMMKV* hybrid : _liveInstances) { + MMKV* expected = deleted; + hybrid->_instance.compare_exchange_strong(expected, nullptr); + } +} + +MMKV* HybridMMKV::getInstance() const { + MMKV* instance = _instance.load(); + if (instance == nullptr) [[unlikely]] { + throw std::runtime_error("The MMKV instance \"" + _id + + "\" has been deleted with `deleteMMKV(...)`! Create a new one with `createMMKV(...)`."); + } + return instance; } std::string HybridMMKV::getId() { - return instance->mmapID(); + MMKV* instance = _instance.load(); + // Still readable after `deleteMMKV(...)`, e.g. for logging. + return instance != nullptr ? instance->mmapID() : _id; } double HybridMMKV::getLength() { + MMKV* instance = getInstance(); return instance->count(); } @@ -80,18 +131,22 @@ double HybridMMKV::getSize() { } double HybridMMKV::getByteSize() { + MMKV* instance = getInstance(); return instance->actualSize(); } size_t HybridMMKV::getExternalMemorySize() noexcept { + MMKV* instance = _instance.load(); return instance != nullptr ? instance->actualSize() : 0; } bool HybridMMKV::getIsReadOnly() { + MMKV* instance = getInstance(); return instance->isReadOnly(); } bool HybridMMKV::getIsEncrypted() { + MMKV* instance = getInstance(); return instance->isEncryptionEnabled(); } @@ -104,6 +159,7 @@ template overloaded(Ts...) -> overloaded; void HybridMMKV::set(const std::string& key, const std::variant, std::string, double>& value) { + MMKV* instance = getInstance(); if (key.empty()) [[unlikely]] { throw std::runtime_error("Cannot set a value for an empty key!"); } @@ -136,6 +192,7 @@ void HybridMMKV::set(const std::string& key, const std::variant HybridMMKV::getBoolean(const std::string& key) { + MMKV* instance = getInstance(); bool hasValue; bool result = instance->getBool(key, /* defaultValue */ false, &hasValue); if (hasValue) { @@ -146,6 +203,7 @@ std::optional HybridMMKV::getBoolean(const std::string& key) { } std::optional HybridMMKV::getString(const std::string& key) { + MMKV* instance = getInstance(); std::string result; bool hasValue = instance->getString(key, result, /* inplaceModification */ true); if (hasValue) { @@ -156,6 +214,7 @@ std::optional HybridMMKV::getString(const std::string& key) { } std::optional HybridMMKV::getNumber(const std::string& key) { + MMKV* instance = getInstance(); bool hasValue; double result = instance->getDouble(key, /* defaultValue */ 0.0, &hasValue); if (hasValue) { @@ -166,6 +225,7 @@ std::optional HybridMMKV::getNumber(const std::string& key) { } std::optional> HybridMMKV::getBuffer(const std::string& key) { + MMKV* instance = getInstance(); MMBuffer result; bool hasValue = instance->getBytes(key, result); if (hasValue) { @@ -176,10 +236,12 @@ std::optional> HybridMMKV::getBuffer(const std::str } bool HybridMMKV::contains(const std::string& key) { + MMKV* instance = getInstance(); return instance->containsKey(key); } bool HybridMMKV::remove(const std::string& key) { + MMKV* instance = getInstance(); bool wasRemoved = instance->removeValueForKey(key); if (wasRemoved) { // Notify on changed @@ -189,10 +251,12 @@ bool HybridMMKV::remove(const std::string& key) { } std::vector HybridMMKV::getAllKeys() { + MMKV* instance = getInstance(); return instance->allKeys(); } void HybridMMKV::clearAll() { + MMKV* instance = getInstance(); auto keysBefore = getAllKeys(); instance->clearAll(); for (const auto& key : keysBefore) { @@ -210,6 +274,7 @@ void HybridMMKV::recrypt(const std::optional& key) { } void HybridMMKV::encrypt(const std::string& key, std::optional encryptionType) { + MMKV* instance = getInstance(); bool isAes256Encryption = encryptionType == EncryptionType::AES_256; bool successful = instance->reKey(key, isAes256Encryption); if (!successful) { @@ -218,6 +283,7 @@ void HybridMMKV::encrypt(const std::string& key, std::optional e } void HybridMMKV::decrypt() { + MMKV* instance = getInstance(); bool successful = instance->reKey(""); if (!successful) [[unlikely]] { throw std::runtime_error("Failed to decrypt MMKV instance!"); @@ -225,15 +291,26 @@ void HybridMMKV::decrypt() { } void HybridMMKV::trim() { + MMKV* instance = _instance.load(); + if (instance == nullptr) { + // Deleted with `deleteMMKV(...)`: nothing to trim. The memory warning listener calls this. + return; + } instance->trim(); instance->clearMemoryCache(); } void HybridMMKV::checkContentChanged() { + MMKV* instance = _instance.load(); + if (instance == nullptr) { + // Deleted with `deleteMMKV(...)`: nothing to check. The AppState listener calls this. + return; + } instance->checkContentChanged(); } Listener HybridMMKV::addOnValueChangedListener(const std::function& onValueChanged) { + MMKV* instance = getInstance(); // Add listener auto mmkvID = instance->mmapID(); auto listenerID = MMKVValueChangedListenerRegistry::addListener(mmkvID, onValueChanged); @@ -272,12 +349,13 @@ std::optional HybridMMKV::getRecoveryStrategy(const Config } double HybridMMKV::importAllFrom(const std::shared_ptr& other) { + MMKV* instance = getInstance(); auto hybridMMKV = std::dynamic_pointer_cast(other); if (hybridMMKV == nullptr) [[unlikely]] { throw std::runtime_error("The given `MMKV` instance is not of type `HybridMMKV`!"); } - size_t importedCount = instance->importFrom(hybridMMKV->instance); + size_t importedCount = instance->importFrom(hybridMMKV->getInstance()); return static_cast(importedCount); } diff --git a/packages/react-native-mmkv/cpp/HybridMMKV.hpp b/packages/react-native-mmkv/cpp/HybridMMKV.hpp index 1ddad2ce..fb27a876 100644 --- a/packages/react-native-mmkv/cpp/HybridMMKV.hpp +++ b/packages/react-native-mmkv/cpp/HybridMMKV.hpp @@ -10,12 +10,24 @@ #include "Configuration.hpp" #include "HybridMMKVSpec.hpp" #include "MMKVTypes.hpp" +#include +#include +#include namespace margelo::nitro::mmkv { class HybridMMKV final : public HybridMMKVSpec { public: explicit HybridMMKV(const Configuration& configuration); + ~HybridMMKV() override; + +public: + /** + * Makes every live instance of the MMKV file `id` in `rootPath` (MMKV's root + * directory if empty) unusable. Call it before deleting the file: + * `MMKV::removeStorage(...)` destroys the native instance they all point to. + */ + static void invalidateInstances(const std::string& id, const std::string& rootPath = ""); public: // Properties @@ -53,7 +65,18 @@ class HybridMMKV final : public HybridMMKVSpec { static std::optional getRecoveryStrategy(const Configuration& config); private: - MMKV* instance; + /** + * The native instance, or throws if it was deleted with `deleteMMKV(...)`. + */ + MMKV* getInstance() const; + +private: + std::atomic _instance; + std::string _id; + std::string _rootPath; + + static std::mutex _liveInstancesMutex; + static std::unordered_set _liveInstances; }; } // namespace margelo::nitro::mmkv diff --git a/packages/react-native-mmkv/cpp/HybridMMKVFactory.cpp b/packages/react-native-mmkv/cpp/HybridMMKVFactory.cpp index e93be0b7..312c20a1 100644 --- a/packages/react-native-mmkv/cpp/HybridMMKVFactory.cpp +++ b/packages/react-native-mmkv/cpp/HybridMMKVFactory.cpp @@ -27,6 +27,9 @@ std::shared_ptr HybridMMKVFactory::createMMKV(const Configuratio } bool HybridMMKVFactory::deleteMMKV(const std::string& id) { + // MMKV destroys the file's native instance: instances still alive in JS + // must not keep pointing to it. + HybridMMKV::invalidateInstances(id); return MMKV::removeStorage(id); }