sqlite: fix segfault SQLTagStore when db handle is garbage collected

[louwers]

Fixes #60448 by changing from a BaseObjectWeakPtr to a BaseObjectPtr.

A SQLTagStore should keep the database alive.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[cjihrig]

Left a couple comments, but LGTM.

[addaleax]

If the DatabaseSync object somehow refers back to the SQLTagStore object, this creates a memory leak.

Would it make sense to keep storing a BaseObjectWeakPtr but to keep it alive by also storing the associated JS object in an internal field of SQLTagStore?

[geeksilva97]

If the DatabaseSync object somehow refers back to the SQLTagStore object, this creates a memory leak.

We're good. DatabaseSync holds no reference to SQLTagStore.

[addaleax]

@geeksilva97 Why do we know that that's true?

DatabaseSync has an associated JS object and can refers to other JS objects (e.g. an authorizer callback) which in turn can refer to the SQLTagStore – what am I missing?

[geeksilva97]

Just to make sure I got it.

Are you referring to a circular dependency? If so, that's the part I'm unable to visualize. You mean like in JS-land, if one has an authorizer callback holding an sqltagstore in a closure or something?

[louwers]

I was not able to reproduce this scenario.

I don't know a lot about V8's garbage collector, but it seems to be able to do its job here.

[addaleax]

Are you referring to a circular dependency?

Yes, but this dependency here (the one represented by the BaseObjectPtr) isn't visible to GC.

You mean like in JS-land, if one has an authorizer callback holding an sqltagstore in a closure or something?

Yup!

I was not able to reproduce this scenario.

Just going to quote myself here:

DatabaseSync has an associated JS object and can refers to other JS objects (e.g. an authorizer callback) which in turn can refer to the SQLTagStore – what am I missing?

Turning this into runnable code is pretty straightforward:

while (true) {
  const sql = new DatabaseSync(':memory:').createTagStore();

  sql.db.exec('CREATE TABLE test (data INTEGER)');
  sql.db.setAuthorizer(() => void sql.db);
  console.log(process.memoryUsage())
}

before this PR this crashes, now this leaks.

[louwers]

Oof. Good catch!

[geeksilva97]

Thanks for the clarification, @addaleax

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.58%. Comparing base (77d8197) to head (b8df359).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60462      +/-   ##
==========================================
- Coverage   88.58%   88.58%   -0.01%     
==========================================
  Files         704      704              
  Lines      207826   207826              
  Branches    40049    40043       -6     
==========================================
- Hits       184112   184107       -5     
- Misses      15757    15764       +7     
+ Partials     7957     7955       -2     

Files with missing lines	Coverage Δ
src/node_sqlite.cc	79.97% <100.00%> (+0.04%)	⬆️
src/node_sqlite.h	80.39% <ø> (ø)

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.