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.

Sign up for GitHub

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

Jump to bottom

Fix vscode/electron crash #144

Closed
wants to merge 6 commits into from
Closed

Fix vscode/electron crash #144

wants to merge 6 commits into from

Conversation

selfint
Copy link

@selfint selfint commented Jun 18, 2023 •
edited

This PR makes node-tree-sitter run in Electron v22 (NODE_MODULE_VERSION 110).

The current patch for this is incomplete (#134). This PR finishes it (hopefully).

I've marked this as a draft, since I had trouble with this #if:

#if /*_MSC_VER && NODE_RUNTIME_ELECTRON && */ NODE_MODULE_VERSION >= 89

Both _MSC_VER and NODE_RUNTIME_ELECTRON were not set, and I couldn't figure out how to set them.

Otherwise this worked for me when running from this VSCode on an M1 mac:

Version: 1.79.2
Commit: 695af097c7bd098fbf017ce3ac85e09bbc5dda06
Date: 2023-06-14T08:58:33.551Z (4 days ago)
Electron: 22.5.7
Chromium: 108.0.5359.215
Node.js: 16.17.1
V8: 10.8.168.25-electron.0
OS: Darwin arm64 22.5.0

Testing repo: https://github.com/selfint/vscode-tree-sitter

@selfint
it works
c811ecb
@selfint
preserve old code
eeed6c3
@selfint
fix bad copy paste
5604230
@selfint
remove unneeded cast
52dfdfe
@selfint
fix vscode crash
cf9473e 
when using Buffer instead of ArrayBuffer (in V8), the exported
object is of type (in JS) Uint8Array.

But the transfer buffers are uint32 arrays, so we need to load them
as Uint32Array s manually.
@selfint selfint marked this pull request as ready for review June 18, 2023 15:14
@selfint selfint changed the title Fix electron crash Fix vscode/electron crash Jun 18, 2023
@verhovsky
Copy link
Collaborator

verhovsky commented Jun 22, 2023 •
edited

I investigated this a bit in #141. Through github actions, on Windows Server 2019 I ran (in this order)

npx prebuild -t 10.0.0 -t 12.0.0 -t 14.0.0 -t 16.0.0 -t 18.0.0 -t 20.0.0
npx prebuild -r electron -t 3.0.0 -t 4.0.0 -t 5.0.0 -t 6.0.0 -t 7.0.0 -t 8.0.0 -t 9.0.0 -t 10.0.0 -t 11.0.0 -t 12.0.0 -t 13.0.0 -t 14.0.0 -t 15.0.0 -t 16.0.0 -t 17.0.0 -t 18.0.0 -t 19.0.0 -t 20.0.0 -t 21.0.0 -t 22.0.0 -t 23.0.0 -t 24.0.0 -t 25.0.0

under Node 10, 12, 14, 16, 18 and 20 on the old code (not the changes in this repo).

Node 10, 12 and 14 fail with this uninteresting error when prebuilding for Node 18:

gyp: name 'llvm_version' is not defined while evaluating condition 'llvm_version=="0.0"' in binding.gyp while trying to load binding.gyp

Node 16, 18 and 20 can prebuild for all Node versions and Electron 3-12 but fail when prebuilding for Electron 13+ with these two errors:

  lib.c
D:\a\node-tree-sitter\node-tree-sitter\vendor\tree-sitter\lib\src\query.c(3135,51): warning C4018: '>': signed/unsigned mismatch [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter.vcxproj]
  win_delay_load_hook.cc
  tree_sitter.vcxproj -> D:\a\node-tree-sitter\node-tree-sitter\build\Release\\tree_sitter.lib
  binding.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  conversions.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  language.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  logger.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  node.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  parser.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  query.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  tree.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  tree_cursor.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(8652,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  util.cc
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h(1670,55): warning C4996: 'v8::Module::ResolveCallback': Use ResolveModuleCallback [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\prebuild\electron\13.0.0\include\node\v8.h([865](https://github.com/tree-sitter/node-tree-sitter/actions/runs/5343552156/jobs/9688022523#step:10:866)2,7): warning C4996: 'v8::HostImportModuleDynamicallyCallback': Use HostImportModuleDynamicallyWithImportAssertionsCallback instead [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
D:\a\node-tree-sitter\node-tree-sitter\src\util.cc(18,20): warning C4996: 'v8::Object::CreationContext': Use MaybeLocal<Context> GetCreationContext() [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
  win_delay_load_hook.cc
     Creating library D:\a\node-tree-sitter\node-tree-sitter\build\Release\tree_sitter_runtime_binding.lib and object D:\a\node-tree-sitter\node-tree-sitter\build\Release\tree_sitter_runtime_binding.exp
conversions.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: static class v8::Local<class v8::ArrayBuffer> __cdecl v8::ArrayBuffer::New(class v8::Isolate *,class std::shared_ptr<class v8::BackingStore>)" (__imp_?New@ArrayBuffer@v8@@SA?AV?$Local@VArrayBuffer@v8@@@2@PEAVIsolate@2@V?$shared_ptr@VBackingStore@v8@@@std@@@Z) referenced in function "void __cdecl node_tree_sitter::InitConversions(class v8::Local<class v8::Object>)" (?InitConversions@node_tree_sitter@@YAXV?$Local@VObject@v8@@@v8@@@Z) [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
node.obj : error LNK2001: unresolved external symbol "__declspec(dllimport) public: static class v8::Local<class v8::ArrayBuffer> __cdecl v8::ArrayBuffer::New(class v8::Isolate *,class std::shared_ptr<class v8::BackingStore>)" (__imp_?New@ArrayBuffer@v8@@SA?AV?$Local@VArrayBuffer@v8@@@2@PEAVIsolate@2@V?$shared_ptr@VBackingStore@v8@@@std@@@Z) [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
    Hint on symbols that are defined and could potentially match:
      "__declspec(dllimport) public: static class v8::Local<class v8::Array> __cdecl v8::Array::New(class v8::Isolate *,int)" (__imp_?New@Array@v8@@SA?AV?$Local@VArray@v8@@@2@PEAVIsolate@2@H@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::External> __cdecl v8::External::New(class v8::Isolate *,void *)" (__imp_?New@External@v8@@SA?AV?$Local@VExternal@v8@@@2@PEAVIsolate@2@PEAX@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::FunctionTemplate> __cdecl v8::FunctionTemplate::New(class v8::Isolate *,void (__cdecl*)(class v8::FunctionCallbackInfo<class v8::Value> const &),class v8::Local<class v8::Value>,class v8::Local<class v8::Signature>,int,enum v8::ConstructorBehavior,enum v8::SideEffectType,class v8::CFunction const *)" (__imp_?New@FunctionTemplate@v8@@SA?AV?$Local@VFunctionTemplate@v8@@@2@PEAVIsolate@2@P6AXAEBV?$FunctionCallbackInfo@VValue@v8@@@2@@ZV?$Local@VValue@v8@@@2@V?$Local@VSignature@v8@@@2@HW4ConstructorBehavior@2@W4SideEffectType@2@PEBVCFunction@2@@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::Integer> __cdecl v8::Integer::New(class v8::Isolate *,int)" (__imp_?New@Integer@v8@@SA?AV?$Local@VInteger@v8@@@2@PEAVIsolate@2@H@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::Number> __cdecl v8::Number::New(class v8::Isolate *,double)" (__imp_?New@Number@v8@@SA?AV?$Local@VNumber@v8@@@2@PEAVIsolate@2@N@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::Object> __cdecl v8::Object::New(class v8::Isolate *)" (__imp_?New@Object@v8@@SA?AV?$Local@VObject@v8@@@2@PEAVIsolate@2@@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::ObjectTemplate> __cdecl v8::ObjectTemplate::New(class v8::Isolate *,class v8::Local<class v8::FunctionTemplate>)" (__imp_?New@ObjectTemplate@v8@@SA?AV?$Local@VObjectTemplate@v8@@@2@PEAVIsolate@2@V?$Local@VFunctionTemplate@v8@@@2@@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::Signature> __cdecl v8::Signature::New(class v8::Isolate *,class v8::Local<class v8::FunctionTemplate>)" (__imp_?New@Signature@v8@@SA?AV?$Local@VSignature@v8@@@2@PEAVIsolate@2@V?$Local@VFunctionTemplate@v8@@@2@@Z)
      "__declspec(dllimport) public: static class v8::Local<class v8::Uint32Array> __cdecl v8::Uint32Array::New(class v8::Local<class v8::ArrayBuffer>,unsigned __int64,unsigned __int64)" (__imp_?New@Uint32Array@v8@@SA?AV?$Local@VUint32Array@v8@@@2@V?$Local@VArrayBuffer@v8@@@2@_K1@Z)
conversions.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: static class std::unique_ptr<class v8::BackingStore,struct std::default_delete<class v8::BackingStore> > __cdecl v8::ArrayBuffer::NewBackingStore(void *,unsigned __int64,void (__cdecl*)(void *,unsigned __int64,void *),void *)" (__imp_?NewBackingStore@ArrayBuffer@v8@@SA?AV?$unique_ptr@VBackingStore@v8@@U?$default_delete@VBackingStore@v8@@@std@@@std@@PEAX_KP6AX010@Z0@Z) referenced in function "void __cdecl node_tree_sitter::InitConversions(class v8::Local<class v8::Object>)" (?InitConversions@node_tree_sitter@@YAXV?$Local@VObject@v8@@@v8@@@Z) [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
node.obj : error LNK2001: unresolved external symbol "__declspec(dllimport) public: static class std::unique_ptr<class v8::BackingStore,struct std::default_delete<class v8::BackingStore> > __cdecl v8::ArrayBuffer::NewBackingStore(void *,unsigned __int64,void (__cdecl*)(void *,unsigned __int64,void *),void *)" (__imp_?NewBackingStore@ArrayBuffer@v8@@SA?AV?$unique_ptr@VBackingStore@v8@@U?$default_delete@VBackingStore@v8@@@std@@@std@@PEAX_KP6AX010@Z0@Z) [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]
D:\a\node-tree-sitter\node-tree-sitter\build\Release\tree_sitter_runtime_binding.node : fatal error LNK1120: 2 unresolved externals [D:\a\node-tree-sitter\node-tree-sitter\build\tree_sitter_runtime_binding.vcxproj]

which is saying the functions v8::ArrayBuffer::New and v8::ArrayBuffer::NewBackingStore don't exist. Electron 13+ uses Node 14.16.0, NODE_MODULE_VERSION 89. Node 14.21.3 uses V8 8.4.371.23. NewBackingStore was added in V8 8.0.22 v8/v8@bba5f1f and v8::ArrayBuffer::New obviously way before then v8/v8@44f2d53 . Also, that line is also using BackingStore::EmptyDeleter which was added in V8 v8.3.9 v8/v8@5cf02f0 but there's no error about that. So are these two functions missing from Electron's Node on purpose? greping through Electron 13's V8 patches, none of them mention ArrayBuffer.

_MSC_VER is a Windows thing that should be set when compiling with Microsoft Visual C++ (MSVC) and NODE_RUNTIME_ELECTRON is set in binding.gyp:

node-tree-sitter/binding.gyp

Line 59 in 1328145

['runtime=="electron"', { 'defines': ['NODE_RUNTIME_ELECTRON=1'] }],

The errors suggest it's compiling this line https://github.com/tree-sitter/node-tree-sitter/blob/master/src/conversions.cc#L38-L39 so it must not be hitting the #if _MSC_VER && NODE_RUNTIME_ELECTRON && NODE_MODULE_VERSION >= 89 line in GitHub Actions either, even though it should since it's compiling on windows, for electron, for NODE_MODULE_VERSION == 89.

The current code in node-tree-sitter is a modification of the code suggested here electron/electron#29893 (comment) (added in #95) which I modified a bit because as I mentioned BackingStore::EmptyDeleter was added in V8 v8.3.9 so it seems more correct to check for V8 v8.4 but that shouldn't be what's causing this.

So I think the first thing is to figure out which of _MSC_VER and/or NODE_RUNTIME_ELECTRON it's not setting and why.

@verhovsky
Copy link
Collaborator

This PR seems to produce some memory errors in the CI on Node 16. On Ubuntu:

malloc_consolidate(): unaligned fastbin chunk detected
Aborted (core dumped)

on macos:

/Users/runner/work/_temp/500e515e-e80c-4eb6-8c99-4e5581a50aa6.sh: line 1: 11581 Abort trap: 6

so we obviously can't merge it.

@verhovsky verhovsky mentioned this pull request Jun 24, 2023
Merged
@verhovsky verhovsky closed this Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@selfint @verhovsky