fix: Fix dangling pointer#1010
Conversation
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-02-16T20:43:19.339ZApplied to files:
📚 Learning: 2026-02-18T15:43:07.515ZApplied to files:
🧬 Code graph analysis (1)src/main.cpp (1)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughIn onGoToReader, the code now copies the incoming initialEpubPath into a local std::string before calling exitActivity(), then passes that local copy to the ReaderActivity constructor to avoid potential reference invalidation when exitActivity() destroys the current activity. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Here’s the serial log that shows the bug in action: |
b68bb79 to
fde13b7
Compare
There was a problem hiding this comment.
This is OK for a quick fix, but I think we must eventually have an ActivityManager to avoid having to pass the function pointer to each activity. We should implement the same idea as Intent on android
The current code is a dangerous delete this because the activity is deleting itself while calling its member (which is onSelectBook in this case)
Call stack:
instance.onSelectBook()callsonGoToReader()onGoToReader()callsexitActivity()exitActivity()invokesdelete currentActivitycurrentActivityis the same asinstanceon step 1 --> literallydelete this;
## Summary * **What is the goal of this PR?** Small fix for bug I found. ## Additional Context 1. `RecentBooksActivity::loop()` calls `onSelectBook(recentBooks[selectorIndex].path)` - passing a **reference** to the path 2. `onSelectBook` is `onGoToReader` which first calls `exitActivity()` 3. `exitActivity()` triggers `RecentBooksActivity::onExit()` which call `recentBooks.clear()` 4. The string reference `initialEpubPath` is now a **dangling reference** - the underlying string has been destroyed 5. When the reference is then used in `new ReaderActivity(...)`, it reads garbage memory 6. The same issue occurs in `HomeActivity` at line 200 with the same pattern The fix is to make a copy of the string in `onGoToReader` before calling `exitActivity()`, so the path data persists even after the activity clears its data structures. --- ### AI Usage Did you use AI tools to help write this code? _**< YES >**_ Claude found the bug, after I shared with it a serial log.
## Summary * **What is the goal of this PR?** Small fix for bug I found. ## Additional Context 1. `RecentBooksActivity::loop()` calls `onSelectBook(recentBooks[selectorIndex].path)` - passing a **reference** to the path 2. `onSelectBook` is `onGoToReader` which first calls `exitActivity()` 3. `exitActivity()` triggers `RecentBooksActivity::onExit()` which call `recentBooks.clear()` 4. The string reference `initialEpubPath` is now a **dangling reference** - the underlying string has been destroyed 5. When the reference is then used in `new ReaderActivity(...)`, it reads garbage memory 6. The same issue occurs in `HomeActivity` at line 200 with the same pattern The fix is to make a copy of the string in `onGoToReader` before calling `exitActivity()`, so the path data persists even after the activity clears its data structures. --- ### AI Usage Did you use AI tools to help write this code? _**< YES >**_ Claude found the bug, after I shared with it a serial log.
## Summary Ref comment: #1010 (review) This PR introduces `ActivityManager`, which mirrors the same concept of Activity in Android, where an activity represents a single screen of the UI. The manager is responsible for launching activities, and ensuring that only one activity is active at a time. Main differences from Android's ActivityManager: - No concept of Bundle or Intent extras - No onPause/onResume, since we don't have a concept of background activities - onActivityResult is implemented via a callback instead of a separate method, for simplicity ## Key changes - Single `renderTask` shared across all activities - No more sub-activity, we manage them using a stack; Results can be passed via `startActivityForResult` and `setResult` - Activity can call `finish()` to destroy themself, but the actual deletion will be handled by `ActivityManager` to avoid `delete this` pattern As a bonus: the manager will automatically call `requestUpdate()` when returning from another activity ## Example usage **BEFORE**: ```cpp // caller enterNewActivity(new WifiSelectionActivity(renderer, mappedInput, [this](const bool connected) { onWifiSelectionComplete(connected); })); // subactivity onComplete(true); // will eventually call exitActivity(), which deletes the caller instance (dangerous behavior) ``` **AFTER**: (mirrors the `startActivityForResult` and `setResult` from android) ```cpp // caller startActivityForResult(new NetworkModeSelectionActivity(renderer, mappedInput), [this](const ActivityResult& result) { onNetworkModeSelected(result.selectedNetworkMode); }); // subactivity ActivityResult result; result.isCancelled = false; result.selectedNetworkMode = mode; setResult(result); finish(); // signals to ActivityManager to go back to last activity AFTER this function returns ``` TODO: - [x] Reconsider if the `Intent` is really necessary or it should be removed (note: it's inspired by [Intent](https://developer.android.com/guide/components/intents-common) from Android API) ==> I decided to keep this pattern fr clarity - [x] Verify if behavior is still correct (i.e. back from sub-activity) - [x] Refactor the `ActivityWithSubactivity` to just simple `Activity` --> We are using a stack for keeping track of sub-activity now - [x] Use single task for rendering --> avoid allocating 8KB stack per activity - [x] Implement the idea of [Activity result](https://developer.android.com/training/basics/intents/result) --> Allow sub-activity like Wifi to report back the status (connected, failed, etc) --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? **PARTIALLY**, some repetitive migrations are done by Claude, but I'm the one how ultimately approve it --------- Co-authored-by: Zach Nelson <[email protected]>
Summary
Additional Context
RecentBooksActivity::loop()callsonSelectBook(recentBooks[selectorIndex].path)- passing a reference to the pathonSelectBookisonGoToReaderwhich first callsexitActivity()exitActivity()triggersRecentBooksActivity::onExit()which callrecentBooks.clear()initialEpubPathis now a dangling reference - the underlying string has been destroyednew ReaderActivity(...), it reads garbage memoryHomeActivityat line 200 with the same patternThe fix is to make a copy of the string in
onGoToReaderbefore callingexitActivity(), so the path data persists even after the activity clears its data structures.AI Usage
Did you use AI tools to help write this code? < YES > Claude found the bug, after I shared with it a serial log.