Fix memory leak around update hooks

Author: simolus3

crates/core/src/pre_close_vtab.rs

@@ -8,6 +8,7 @@ use powersync_sqlite_nostd as sqlite;
 use sqlite::{Connection, ResultCode};
 
 use crate::state::DatabaseState;
+use crate::update_hooks::uninstall_update_hooks;
 use crate::vtab_util::*;
 
 /// A virtual table hack to implement a "pre-close hook" for databases.
@@ -20,6 +21,7 @@ use crate::vtab_util::*;
 struct VirtualTable {
     base: sqlite::vtab,
     state: Rc<DatabaseState>,
+    db: *mut sqlite::sqlite3,
 }
 
 extern "C" fn connect(
@@ -42,6 +44,7 @@ extern "C" fn connect(
                 zErrMsg: core::ptr::null_mut(),
             },
             state: DatabaseState::clone_from(aux),
+            db,
         }));
         *vtab = tab.cast::<sqlite::vtab>();
         let _ = sqlite::vtab_config(db, 0);
@@ -57,6 +60,7 @@ extern "C" fn disconnect(vtab: *mut sqlite::vtab) -> c_int {
     // So we can use this as a "pre-close" hook and ensure we clear prepared statements the core
     // extension might hold.
     vtab.state.release_resources();
+    uninstall_update_hooks(vtab.db, &vtab.state);
 
     ResultCode::OK as c_int
 }

crates/core/src/state.rs

@@ -25,6 +25,8 @@ use crate::{
 #[derive(Default)]
 pub struct DatabaseState {
     pub is_in_sync_local: Cell<bool>,
+    /// Whether the core extension has installed update, commit and rollback hooks.
+    pub core_extension_has_update_hooks: Cell<bool>,
     schema: RefCell<Option<Schema>>,
     pending_updates: RefCell<BTreeSet<String>>,
     commited_updates: RefCell<BTreeSet<String>>,

crates/core/src/update_hooks.rs

@@ -1,5 +1,4 @@
 use core::{
-    cell::Cell,
     ffi::{CStr, c_char, c_int, c_void},
     ptr::null_mut,
 };
@@ -21,11 +20,7 @@ use crate::{constants::SUBTYPE_JSON, error::PowerSyncError, state::DatabaseState
 /// The update hooks don't have to be uninstalled manually, that happens when the connection is
 /// closed and the function is unregistered.
 pub fn register(db: *mut sqlite::sqlite3, state: Rc<DatabaseState>) -> Result<(), ResultCode> {
-    let state = Box::new(HookState {
-        has_registered_hooks: Cell::new(false),
-        db,
-        state,
-    });
+    let state = Box::new(HookState { db, state });
 
     db.create_function_v2(
         "powersync_update_hooks",
@@ -41,31 +36,13 @@ pub fn register(db: *mut sqlite::sqlite3, state: Rc<DatabaseState>) -> Result<()
 }
 
 struct HookState {
-    has_registered_hooks: Cell<bool>,
     db: *mut sqlite::sqlite3,
     state: Rc<DatabaseState>,
 }
 
 extern "C" fn destroy_function(ctx: *mut c_void) {
     let state = unsafe { Box::from_raw(ctx as *mut HookState) };
-
-    if state.has_registered_hooks.get() {
-        check_previous(
-            "update",
-            &state.state,
-            state.db.update_hook(None, null_mut()),
-        );
-        check_previous(
-            "commit",
-            &state.state,
-            state.db.commit_hook(None, null_mut()),
-        );
-        check_previous(
-            "rollback",
-            &state.state,
-            state.db.rollback_hook(None, null_mut()),
-        );
-    }
+    uninstall_update_hooks(state.db, &state.state);
 }
 
 extern "C" fn powersync_update_hooks(
@@ -107,7 +84,7 @@ extern "C" fn powersync_update_hooks(
                     Rc::into_raw(db_state.clone()) as *mut c_void,
                 ),
             );
-            state.has_registered_hooks.set(true);
+            db_state.core_extension_has_update_hooks.set(true);
         }
         "get" => {
             let state = unsafe { user_data.as_ref().unwrap_unchecked() };
@@ -155,6 +132,14 @@ unsafe extern "C" fn rollback_hook_impl(ctx: *mut c_void) {
     state.track_rollback();
 }
 
+pub fn uninstall_update_hooks(db: *mut sqlite::sqlite3, state: &Rc<DatabaseState>) {
+    if state.core_extension_has_update_hooks.take() {
+        check_previous("update", state, db.update_hook(None, null_mut()));
+        check_previous("commit", state, db.commit_hook(None, null_mut()));
+        check_previous("rollback", state, db.rollback_hook(None, null_mut()));
+    }
+}
+
 fn check_previous(desc: &'static str, expected: &Rc<DatabaseState>, previous: *const c_void) {
     let expected = Rc::as_ptr(expected);
 

dart/pubspec.lock

@@ -237,10 +237,10 @@ packages:
     dependency: transitive
     description:
       name: native_toolchain_c
-      sha256: "89e83885ba09da5fdf2cdacc8002a712ca238c28b7f717910b34bcd27b0d03ac"
+      sha256: "8aaead321425bd3f03bd5894aa27c8ea6993eab95531da7e59f5d39c6e5708ec"
       url: "https://pub.dev"
     source: hosted
-    version: "0.17.4"
+    version: "0.18.0"
   node_preamble:
     dependency: transitive
     description:
@@ -364,11 +364,12 @@ packages:
   sqlite3:
     dependency: "direct main"
     description:
-      name: sqlite3
-      sha256: c6cfe9b1cc159c9eb8ba174b533a60b5126f9db8c6e34efb127d2bc04bc45034
-      url: "https://pub.dev"
-    source: hosted
-    version: "3.1.4"
+      path: sqlite3
+      ref: main
+      resolved-ref: "97268a5b32cf6313dd670e6fa57ae88d38a6564b"
+      url: "https://github.com/simolus3/sqlite3.dart.git"
+    source: git
+    version: "3.3.2"
   sqlite3_test:
     dependency: "direct dev"
     description:
@@ -514,4 +515,4 @@ packages:
     source: hosted
     version: "3.1.3"
 sdks:
-  dart: ">=3.9.999 <4.0.0"
+  dart: ">=3.10.0 <4.0.0"

dart/pubspec.yaml

@@ -19,6 +19,15 @@ dev_dependencies:
   meta: ^1.16.0
   path: ^1.9.1
 
+dependency_overrides:
+  # TODO: Remove after https://github.com/simolus3/sqlite3.dart/commit/97268a5b32cf6313dd670e6fa57ae88d38a6564b
+  # gets released.
+  sqlite3:
+    git:
+      url: https://github.com/simolus3/sqlite3.dart.git
+      path: sqlite3
+      ref: main
+
 # See https://pub.dev/documentation/sqlite3/latest/topics/hook-topic.html
 hooks:
   user_defines: