feat: add destroy method to clear registry state on module invalidation #858
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new mechanism for cleaning up native resources was introduced across Android, iOS, and C++ layers. This includes adding an Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. All modifications directly relate to the introduction of a native cleanup/invalidation mechanism as required by the linked issue. ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
android/src/main/cxx/NativeUnistylesModule.h (1)
31-33: Suppress unused-parameter warning forjThis
jThisisn’t used insideinvalidateNative, which triggers-Wunused-parameterwith strict builds. Either drop the param or mark it unused:-static void invalidateNative(jni::alias_ref<jhybridobject> jThis) { +static void invalidateNative(jni::alias_ref<jhybridobject> /* jThis */) {Tiny, but keeps CI warnings clean.
android/src/main/java/com/unistyles/UnistylesModule.kt (1)
51-52: Missing@Keep/ visibility modifier on native methodProguard/R8 can still strip private externals even with
@DoNotStripin some configs. Mark asinternal externalor annotate with@Keepto be safe.android/src/main/cxx/NativeUnistylesModule.cpp (1)
28-30: Use fully-qualified member pointer for consistencyBoth existing registrations use the
UnistylesModule::qualifier; dropping it here is harmless but breaks the established convention and makes grepping harder.- makeNativeMethod("invalidateNative", invalidateNative), + makeNativeMethod("invalidateNative", UnistylesModule::invalidateNative),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
android/src/main/cxx/NativeUnistylesModule.cpp(1 hunks)android/src/main/cxx/NativeUnistylesModule.h(2 hunks)android/src/main/java/com/unistyles/UnistylesModule.kt(2 hunks)cxx/core/UnistylesRegistry.cpp(1 hunks)cxx/core/UnistylesRegistry.h(2 hunks)ios/UnistylesModuleOnLoad.mm(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
android/src/main/cxx/NativeUnistylesModule.cpp (1)
android/src/main/java/com/unistyles/UnistylesModule.kt (2)
initHybrid(45-49)invalidateNative(51-52)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (1)
android/src/main/cxx/NativeUnistylesModule.cpp (1)
25-31: Confirm registry teardown is safe under concurrent access
invalidateNativetriggerscore::UnistylesRegistry::destroy(). If any JS / native thread may still touch the registry, you risk use-after-free.
Please ensure:
destroy()is idempotent and internally synchronized.- Calls into the registry are quiesced (or guarded by a mutex/atomic flag) before this native method can run.
Consider adding explicit locking or an “isDestroyed” guard inside
UnistylesRegistryif it isn’t already present.
| void setScopedTheme(std::optional<std::string> themeName); | ||
| core::Unistyle::Shared getUnistyleById(jsi::Runtime& rt, std::string unistyleID); | ||
| void destroy(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
destroy() needs explicit thread-safety guarantees
destroy() mutates _states, _styleSheetRegistry, _shadowRegistry, and _scopedTheme without any locking whilst most public APIs manipulate the same structures under trafficController.withLock or other threads. Exposing it publicly without the same protection opens the door to racy teardown.
Consider:
- void destroy();
+ // Thread-safe wipe of the global registry.
+ // Must be called when _no other thread_ might read / write the maps.
+ void destroy();…and inside the implementation use a mutex / reuse trafficController lock.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void setScopedTheme(std::optional<std::string> themeName); | |
| core::Unistyle::Shared getUnistyleById(jsi::Runtime& rt, std::string unistyleID); | |
| void destroy(); | |
| void setScopedTheme(std::optional<std::string> themeName); | |
| core::Unistyle::Shared getUnistyleById(jsi::Runtime& rt, std::string unistyleID); | |
| // Thread-safe wipe of the global registry. | |
| // Must be called when _no other thread_ might read / write the maps. | |
| void destroy(); |
🤖 Prompt for AI Agents
In cxx/core/UnistylesRegistry.h around lines 50 to 53, the public destroy()
method modifies shared state variables without any thread-safety mechanisms,
unlike other public APIs that use trafficController.withLock for
synchronization. To fix this, update the destroy() implementation to acquire the
same mutex or lock used by trafficController before mutating _states,
_styleSheetRegistry, _shadowRegistry, and _scopedTheme, ensuring consistent
thread-safe access during teardown.
Summary
Fixes #830
Summary by CodeRabbit