Weird and inexplicable crashes when using TypedArrayContents #922
Comments
|
Based on |
|
Hmm, but that doesn't explain why a single call |
If this is the case, my guess would be that this is a |
|
AMD64 and Linux... Yeah, an ABI mismatch is a possibility, but I don't know how I could prove or disprove this... |
|
@robinchrist Can you maybe share the exact compiler version and flags used when building the addon? |
I missed that doing it twice causes a problem. However, |
Well that'd give a compile time error, so I'd say your "do not permit assigning" macro doesn't work as intended.
|
|
Strange, I did get a compile time error with Apple clang version 12.0.0 (clang-1200.0.32.29) |
|
Ah, problem is C++17. |
|
Trying to do a std::move() fails as expected. Perhaps C++17 allows this assignment expression because it is guaranteed to be identical to directly using the constructor as |
|
I've got a stacktrace with a debug version of Electron (9.0.0, but same for all other versions) The assertion / check is raised in |
|
Compile Flags for Electron 9 (that happens to be the version I have a debug build of...) |
|
Based on what @addaleax mentioned, the |
|
Yeah it definitely sticks out... But I'm not sure where the ABI mismatch could be. I'm definitely building against the right node version (otherwise the addon wouldn't be able to be loaded by node because the node ABI version is embedded in the path where the actual shared library is located) |
|
Are there threads involved somehow? |
|
Yes, in the original version. But this definitely happens inside the main loop (in an async worker). I have prepared another minimal example (don't know whether I'll be able to push it today, it's 3AM), without any threads or whatever involved. The issue depends on the size of the buffer! Renderer crashes for If you remove the Yes, I have actually bisected the issue |
|
|
I tried this and did not see a crash. Could this problem be specific to Electron or is it exposing a deeper flaw? #include <nan.h>
#include <cstdint>
NAN_METHOD(CalculateSync) {
Nan::HandleScope scope;
int32_t len = Nan::To<int32_t>(info[0]).FromJust();
v8::Local<v8::ArrayBuffer> fieldDataBuffer = v8::ArrayBuffer::New(v8::Isolate::GetCurrent(), len * sizeof(float));
v8::Local<v8::Float32Array> fieldDataArr = v8::Float32Array::New(fieldDataBuffer, 0, len);
float* ptr = (float*)(fieldDataBuffer->GetBackingStore()->Data());
ptr[0] = 1.0;
info.GetReturnValue().Set(fieldDataArr);
}
NAN_MODULE_INIT(InitAll) {
Nan::Set(target, Nan::New<v8::String>("calculateSync").ToLocalChecked(),
Nan::GetFunction(Nan::New<v8::FunctionTemplate>(CalculateSync)).ToLocalChecked());
}
NODE_MODULE(addon, InitAll)var addon = require('./build/Release/addon');
for (var size = 245760; size != 528975; ++size)
addon.calculateSync(size); |
|
Update: |
|
I think this is related to what happens here : #892 |
|
Oh yeah, I forgot to follow up. TL;DR Electron and Node are built against libc++ whereas the default on Linux is still libstdc++ Everything was fine until "now" (meaning the point when I upgraded electron and thus node). The native node API was very C-ish, so the ABI issue which was there all the time didn't matter. But at some point, the node guys decided that it would be a clever thing to change the API and now pass a non-trivial object, aka While I understand the reason (more intuitive and idiomatic code within node), it is a huge issue for everyone using the native node API. So for me, the only viable solution was to port the entire addon (more than 10k lines) to N-API / node-addon-api. I am still a bit mad about that decision from the node devs, but I have successfully ported the addon to node-addon-api / N-API and everything is working fine. My personal recommendation: If you want more bloody details, here's a story from a guy on medium which almost applys 1:1 |
Not to deflect, but unfortunately we weren’t given a choice here – this came from V8/Chromium. They don’t prioritize situations like the Node.js one, where the V8 “embedder” doesn’t consist of a monolithic codebase and instead potentially includes sub-embedders like Electron and users who write addons against the V8 API.
👍 👍 👍 |
|
Maybe this should be added to the "Known issues" section? And it would probably be good to place a warning at the very top like Opinions on this? |
Well, in that case I'm mad at the Chromium / V8 devs now 😁 |
|
This is for sure a previously unknown issue and should be noted among the known issues. NAN, being a header-only library, has never promised a stable ABI. node-addon-api was created in order to provide a stable ABI.
…On November 23, 2021 12:55:09 PM GMT+02:00, Robin Christ ***@***.***> wrote:
Maybe this should be added to the "Known issues" section? And it would probably be good to place a warning at the very top
like
```
Please be aware that nan, as the name says, uses the native Node.js API
Due to the way Node.js (and Chromium) work, using nan or the native Node.js API may lead to a variety of issues, most importantly ABI issues which may crash the application when certain features are used.
This is especially important if you're planning to write an addon for Electron
Unless you're sure you want to go this way and know what you're doing, please consider using node-addon-api (based on the ABI stable Node-API / N-API) instead
```
Opinions on this?
|
Right – to be clear, there’s also nothing wrong with NAN here. It’s Node.js that does more or less promise to expose a stable ABI through its module version number, and fails to do so here. |
I'm getting really weird crashes with v8 array buffers / typed arrays
Base:
works:
segfaults:
...but it doesn't segfault immediately, but rather a couple of seconds later, probably when some GC happens.
I don't even need to do anything with the pointer. It is enough to construct a (temporary)
Nan::TypedArrayContentsI have digged through the
nan_typedarray_contents.hheaderonce I comment out
there is no crash.
I tried to find out what this is, but now it's getting really weird:
works.
Where as
(just the first call repeated)
segfaults immediately... I suppose this is a node bug?
I'm compiling against electron 8.0.0, node version should be 12.13.0
I'll try with a later node / electron version now
The text was updated successfully, but these errors were encountered: