uhop / node-re2

node.js bindings for RE2: fast, safe alternative to backtracking regular expression engines.
Other
495 stars 53 forks source link

Core Dump when using worker_threads #65

Closed BannerBomb closed 4 years ago

BannerBomb commented 4 years ago

Hey, I see you have fixed the issue with the worker_threads. However now when using the package without initializing the constructor will cause a core dump.

So first off my node.js version is: v13.10.1 and RE2 version is 1.11.0

#
# Fatal error in , line 0
# Check failed: map->instance_type() == JS_REG_EXP_TYPE || map->instance_type() == JS_OBJECT_TYPE || map->instance_type() == JS_ERROR_TYPE || map->instance_type() == JS_ARRAY_TYPE || map->instance_type() == JS_API_OBJECT_TYPE || map->instance_type() == WASM_GLOBAL_OBJECT_TYPE || map->instance_type() == WASM_INSTANCE_OBJECT_TYPE || map->instance_type() == WASM_MEMORY_OBJECT_TYPE || map->instance_type() == WASM_MODULE_OBJECT_TYPE || map->instance_type() == WASM_TABLE_OBJECT_TYPE || map->instance_type() == JS_SPECIAL_API_OBJECT_TYPE.
#
#
#
#FailureMessage Object: 0x7fffd87c9bf0
 1: 0xa77221  [node]
 2: 0x19ed1d5 V8_Fatal(char const*, ...) [node]
 3: 0xd18ad8 v8::internal::Factory::CopyJSObjectWithAllocationSite(v8::internal::Handle<v8::internal::JSObject>, v8::internal::Handle<v8::internal::AllocationSite>) [node]
 4: 0xd1908b v8::internal::Factory::CopyJSObject(v8::internal::Handle<v8::internal::JSObject>) [node]
 5: 0xb7dc84 v8::internal::ApiNatives::InstantiateObject(v8::internal::Isolate*, v8::internal::Handle<v8::internal::ObjectTemplateInfo>, v8::internal::Handle<v8::internal::JSReceiver>) [node]
 6: 0xbf01f0  [node]
 7: 0xbf126d v8::internal::Builtins::InvokeApiFunction(v8::internal::Isolate*, bool, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Handle<v8::internal::HeapObject>) [node]
 8: 0xcc6ee1  [node]
 9: 0xcc74fd v8::internal::Execution::New(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [node]
10: 0xb93720 v8::Function::NewInstanceWithSideEffectType(v8::Local<v8::Context>, int, v8::Local<v8::Value>*, v8::SideEffectType) const [node]
11: 0xb939dc v8::Function::NewInstance(v8::Local<v8::Context>, int, v8::Local<v8::Value>*) const [node]
12: 0x7f3685b9f3be WrappedRE2::New(Nan::FunctionCallbackInfo<v8::Value> const&) [/mnt/c/Users/Admin/Desktop/HighlightMajorUpdate/node_modules/re2/build/Release/re2.node]
13: 0x7f3685b9b65f  [/mnt/c/Users/Admin/Desktop/HighlightMajorUpdate/node_modules/re2/build/Release/re2.node]
14: 0xbef09c  [node]
15: 0xbf0ea7 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
16: 0x13da699  [node]
Illegal instruction (core dumped)

A reproducible code is

const RE2 = require('re2');
RE2('test')
const RE2 = require('re2');
RE2(/test/);

the 2 above are examples that will cause a core dump. But it will cause the core dump anytime you try to use it without initializing the constructor, and an example of the code that won't cause a core dump is

const RE2 = require('re2');
new RE2('test');
const RE2 = require('re2');
new RE2(/test/);
BannerBomb commented 4 years ago

I have played around with it some more. And it also causes a core dump when doing something like

const RE2 = require('re2');
new RE2(/test/).constructor()

And I have found an issue with the properties.

In the images below the first bot with the green avatar(Highlight Testing) is using worker_threads, while the bot with the pink avatar(Highlight) isn't using worker_threads.

result of the source property. image

result of the flags property. image

result of the global property. image

The methods like toString, exec test etc seems fine 👍 image

Looking at these all of the methods seem to be fine but the RegExp properties are all undefined.

BannerBomb commented 4 years ago

also in reply to this https://github.com/uhop/node-re2/issues/57#issuecomment-581194375 comment, not sure if this helps or if you've seen it but there's some small documentations on implementing worker thread support to a C++ addon. https://nodejs.org/api/addons.html#addons_object_factory starting from here and scrolling down from that page is about factory objects, wrapped functions, and factory wrapped objects https://nodejs.org/api/addons.html#addons_factory_of_wrapped_objects etc

uhop commented 4 years ago

Like almost everybody else I use nan.h, which supposed to hide API differences between versions yet supporting all features. I'll try to refresh my nan code first before doing anything drastic.

Thank you for identifying specific problems. It gives me something to start with.

uhop commented 4 years ago

New version: https://github.com/uhop/node-re2/tree/1.12.0

It WFM in workers, yet my worker tests are rather primitive. Please try to verify that it works in your project as well.

uhop commented 4 years ago

Since the previous comment, I found a problem with redefining the constructor in different environments. The constructor was stored in a global. I got rid of it.

New version: https://github.com/uhop/node-re2/tree/1.13.0

Please let me know the results of your testing.

BannerBomb commented 4 years ago

@uhop I've tested all the issues I've posted previously along with a few other things and it seems to be working good now. Thanks. I'll let you know if I come across anything else. Keep up the good work 😄