JSI fails to pass arguments js->c++ when on v8 #133
Comments
|
hi there! i am happy to help, but it's hard for me to investigate a problem i cannot reproduce. could you help to provide a minimal reproducible example for me to further investigate? |
|
Thank you! |
|
So my plan didn't work out, but then I tried the reductive approach. Here's a sample as minimal as I could get it: https://github.com/OzymandiasTheGreat/v8-error I started removing bits and testing 'till I could keep the error but have no extras. |
|
sorry i have been late to response this issue. the reproducible example is great. as far as i can tell for issue, because the return statement here, any calls to so these patch to your js code should make it work: diff --git a/App.tsx b/App.tsx
index 32eafd7..cc81011 100644
--- a/App.tsx
+++ b/App.tsx
@@ -4,8 +4,8 @@ import { getRAFBindings } from './binding/native'
const App = () => {
useEffect(() => {
const raf = getRAFBindings()
- // console.log((raf as any).raf_stat('/data/data/com.explibuv/cache'))
- console.log((raf as any).raf_stat(13))
+ console.log((raf as any).raf_stat('/data/data/com.explibuv/cache'))
+ // console.log((raf as any).raf_stat(13))
})
return null
}
diff --git a/binding/native.ts b/binding/native.ts
index 5e782db..c700fa3 100644
--- a/binding/native.ts
+++ b/binding/native.ts
@@ -6,7 +6,7 @@ export const RAF_BASE: string = HolepunchBindings.RAF_BASE
export const RAF_TMP_BASE: string = HolepunchBindings.RAF_TMP_BASE
__hlp_uv.setup()
-__hlp_raf.setup()
+__hlp_raf.setup("SHOULD PASS A STRING")
__hlp_thrower = (err) => {
throw err
}i am not sure whether this is exactly your original issue. please share more insight if i misunderstand your problem. thank you! |
|
Oh boy, this is bigger than I thought. See, your workaround suppresses the error I'm getting, but This works correctly on hermes, and I think jsc too, but it's been a while since I tried that. It also works correctly if we don't start uv loop in separate thread, but then uv methods don't work obviously. |
|
could you share some insight how to test your example? as i mentioned previous, because the return statement, you just return the first argument as string. that's why i also tested on hermes for your original example and it crashed. |
|
Sorry for the delay, been swamped with other things. So we updated react-native version and I figured out that when we use macros in c++ debugger gives incorrect or at least incomplete stack trace. So I remade the minimal repro, made sure it runs on hermes, and tried adding v8 again. So basically what happens, is when we want to throw error JS side, we call On hermes this works as expected, but on v8 the app crashes with SIGABRT because assertion So yeah, I reported the error wrong, because the macros with incomplete stack traces had me confused. Could you please look into this again? If you want to try hermes to see how it works just go a couple of commits back in repo. This time I made sure the repro is actually representative of the error. |
|
that's a great repro, thanks @OzymandiasTheGreat! i get to find a problem in my implementation. before publishing a fix, i would like to have your verification and make sure everything works as expected. could you apply the following patch and try again locally? you could use patch-package to apply to patch or simply edit the file. patches/react-native-v8+1.5.2.patch diff --git a/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp b/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp
index 7d4f4eb..7b85e75 100644
--- a/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp
+++ b/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp
@@ -215,8 +215,10 @@ jsi::Value V8Runtime::ExecuteScript(
void V8Runtime::ReportException(v8::Isolate *isolate, v8::TryCatch *tryCatch)
const {
v8::HandleScope scopedHandle(isolate);
+ v8::Local<v8::Context> context(isolate->GetCurrentContext());
+ v8::Local<v8::Value> exceptionValue = tryCatch->Exception()->ToString(context).ToLocalChecked();
std::string exception =
- JSIV8ValueConverter::ToSTLString(isolate, tryCatch->Exception());
+ JSIV8ValueConverter::ToSTLString(isolate, exceptionValue);
v8::Local<v8::Message> message = tryCatch->Message();
if (message.IsEmpty()) {
// V8 didn't provide any extra information about this error; just
@@ -225,7 +227,6 @@ void V8Runtime::ReportException(v8::Isolate *isolate, v8::TryCatch *tryCatch)
return;
} else {
std::ostringstream ss;
- v8::Local<v8::Context> context(isolate->GetCurrentContext());
// Print (filename):(line number): (message).
ss << JSIV8ValueConverter::ToSTLString(
|
|
I'm sorry, I can't test your patch. I spent all day today trying to do an android build, and it crashes on startup with |
@OzymandiasTheGreat to fix that error run |
|
@Kudo I've applied your patch, and the crash now doesn't seem to be coming from V8 anymore, but the app does still crash I am also using a different test case, where I basically have a JS issue
hope this helps 👍 |
We have a large project using a lot of JSI C++ code. We've previously used Hermes and it all worked fine. To make interoperability between our node code and react native better we'd like to switch to v8.
So I followed the readme to a T and set up v8 (no INTL, no JIT). Everything seemed fine at first,
but when calling native code our app crashed with SIGABORT. After some digging I traced this to
failing
assert(string->IsString()). Some more digging and the string in question is undefined.After short-circuiting every possible path and leaving just one function, I found that no arguments
are passed from javascript to C++. Like, no matter what or how many args I pass from js, on c++ argument count is always 0 and values are undefined.
Now my first guess is that there's something wrong with our project configuration or that v8 is
improperly linked. However, I have no clue when it comes to gradle and our gradle person is on vacation.
I know this is off-topic, but could you help me out? Any idea what could cause JSI to fail like this?
The text was updated successfully, but these errors were encountered: