Skip to content

Commit 5d1edcb

Browse files
committed
docs: update P02 with correct fix explanation for dangling pointers
1 parent e1a3fcb commit 5d1edcb

File tree

1 file changed

+9
-15
lines changed

1 file changed

+9
-15
lines changed

doc/todo/P02-investigate-flaky-native-crashes.md

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -101,17 +101,15 @@ Affected tests: `extension-loading.test.ts`, `session-lifecycle.test.ts`, `sessi
101101
4. Y's destructor calls `Delete()``sqlite3session_delete(Y->session_)` (Y's session_ is still valid!)
102102
5. Loop continues to Y → calls `sqlite3session_delete(Y)` again → **double-free → SIGABRT**
103103

104-
**Fix**: Split cleanup into two passes:
105-
1. Pass 1: Delete all SQLite sessions and clear all `session_` pointers
106-
2. Pass 2: Release database references (can trigger GC, but Delete() is now a no-op for all sessions)
104+
**Fix**: Don't call `database_ref_.Reset()` in DeleteAllSessions() at all. The iteration loop only uses pure C/C++ operations (no Napi calls that could trigger GC), so it's safe. When Sessions are later GC'd, their `Delete()` returns early (session_ is nullptr) and Napi::ObjectReference destructor handles cleanup automatically.
107105

108-
**Commit**: `3a6aaff`
106+
**Commit**: `e1a3fcb`
109107

110108
---
111109

112110
## Fixes Implemented
113111

114-
### Session Fixes (Commits: a151cb6, fb283df, dadbb86, 3a6aaff)
112+
### Session Fixes (Commits: a151cb6, fb283df, dadbb86, e1a3fcb)
115113

116114
**[src/sqlite_impl.h](../../src/sqlite_impl.h)**:
117115

@@ -136,7 +134,7 @@ Napi::ObjectReference database_ref_;
136134
}
137135
```
138136
139-
3. `DeleteAllSessions()` - Two-pass cleanup to prevent double-free:
137+
3. `DeleteAllSessions()` - Only clean up SQLite sessions, don't call Reset():
140138
141139
```cpp
142140
std::set<Session *> sessions_copy;
@@ -145,19 +143,15 @@ Napi::ObjectReference database_ref_;
145143
sessions_copy = sessions_;
146144
sessions_.clear(); // RemoveSession() becomes no-op
147145
}
148-
// Pass 1: Delete SQLite sessions and clear pointers
146+
// Only pure C/C++ operations - no Napi calls that could trigger GC
149147
for (auto *session : sessions_copy) {
150148
if (session->GetSession()) {
151149
sqlite3session_delete(session->GetSession());
152-
session->session_ = nullptr; // Makes Delete() a no-op
153-
}
154-
}
155-
// Pass 2: Release references (can trigger GC safely now)
156-
for (auto *session : sessions_copy) {
157-
if (!session->database_ref_.IsEmpty()) {
158-
session->database_ref_.Reset();
150+
session->session_ = nullptr; // Makes later Delete() a no-op
159151
}
160152
}
153+
// DON'T call database_ref_.Reset() - it can trigger GC and create
154+
// dangling pointers. Let Sessions clean up when naturally GC'd.
161155
```
162156

163157
### Verification Commands
@@ -321,5 +315,5 @@ docker run --rm -v "$(pwd)":/host:ro node:22-alpine sh -c '\
321315
| `a151cb6` | Add `database_ref_` to Session |
322316
| `fb283df` | Release `database_ref_` in DeleteAllSessions |
323317
| `dadbb86` | Release mutex before GC-triggering operations |
324-
| `3a6aaff` | Two-pass cleanup to prevent double-free during GC |
318+
| `e1a3fcb` | Remove Reset() from DeleteAllSessions (prevents dangling ptrs) |
325319
| `5d86172` | Fix multi-process test expected values (unrelated) |

0 commit comments

Comments
 (0)