Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix db::serialize() crashing with Electron #1036

Merged

Conversation

DamienEspitallier
Copy link
Contributor

This is a fix for #981

It use a #ifdef encapsulation to only use the copy method on electron build.

This is the first time I use lzz and I did not achieve to output #ifndef/else/endif clause from the database.lzz file, so I use the macro.lzz instead. I hope this is acceptable :).

@JoshuaWise JoshuaWise merged commit 0092d43 into WiseLibs:master Sep 2, 2023
@JoshuaWise
Copy link
Member

thanks for this fix!

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Jul 14, 2024

@DamienEspitallier @JoshuaWise @mceachen isn't the if/else backwards? Reversing it fixes #1223 and the original #981

For repro I still use the minimal:

const Database = require('better-sqlite3');
const db = new Database(':memory:');

db.exec('CREATE TABLE foo (id)');

console.log(db.serialize());

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Jul 14, 2024

Or maybe it is the wrong flag now? Turns out it actually works with 8.6.0 but for 11.1.2 it does not. I'm confused.

Edit: Can confirm that better-sqlite3@11.1.2 works with electron@29.4.3 but breaks with electron@30.0.0-alpha.1. But compiling it with the following changes makes it work:

image

however, this does not work (basically the original code):

image

but this works:

image

Someone smarter and more patient than me pls explain. Did they invert the flag on us?

Looking through https://grep.app/search?q=V8_COMPRESS_POINTERS_IN_SHARED_CAGE this is also super rarely used so maybe nobody noticed?

NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED is probably the appropriate flag and we were just lucky that it previously aligned with V8_COMPRESS_POINTERS_IN_SHARED_CAGE

@TheOneTheOnlyJJ
Copy link

TheOneTheOnlyJJ commented Jul 15, 2024

Can a fix for this be expected in the near future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants