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

"Uncaught JavaError in function callback" needs more details #446

Open
lukehutch opened this issue Jan 20, 2025 · 24 comments
Open

"Uncaught JavaError in function callback" needs more details #446

lukehutch opened this issue Jan 20, 2025 · 24 comments

Comments

@lukehutch
Copy link

lukehutch commented Jan 20, 2025

When I call into a Java method from V8, if the Java code triggers an ExceptionInInitializerError, then the JS code throws Uncaught JavaError in function callback. However, no other information is provided back to V8, which means you have to single-step through the Java code to figure out what went wrong (and for ExceptionInInitializerError, it's not even easy to find the problem by single-stepping). There needs to be more info passed back to JS.

Can you please include the message and Exception/Error class name from any Java Throwable in the JS exception's message?

Even better, can you please return the Java stacktrace as part of the stacktrace that is included when the JS exception is thrown? (I assume that it's possible to manually add things to a JS stacktrace, but I have no idea...)

@caoccao
Copy link
Owner

caoccao commented Jan 20, 2025

Please leave a repo with the issue you mentioned and specify the expected behavior you want.

@lukehutch
Copy link
Author

Here you go

https://github.com/lukehutch/javet-issue

@caoccao
Copy link
Owner

caoccao commented Jan 21, 2025

That feature hasn't been implemented yet. It seems there are a few Javet users asking for this feature. I'll consider putting it in the future releases.

@lukehutch
Copy link
Author

lukehutch commented Jan 21, 2025

Thanks. The augmented stacktrace should also be passed back to the outermost Java frame too (here main), if the JS frame does not catch the exception.

This is an important feature, because debugging is very difficult without it.

This should work for uncaught Throwable too, not just uncaught Exception (I think the current system does that, but I just wanted to mention it).

@caoccao
Copy link
Owner

caoccao commented Jan 21, 2025

It might not be the one you wished. The feature I was thinking of is to carry over the Java stack trace as a JS string. Anything beyond that has already been resolved by other approaches. For instance:

  • Apply global try-catch to all callback functions.
  • Save and re-throw the exceptions.
  • Get the saved exceptions in Java or JavaScript via your own API.

A lot of Javet users are using this approach.

Why doesn't Javet build it in? Because there are many things to be customized here. Javet leaves the freedom to the Javet users.

  • App may want to preserve inner exceptions, exception hierarchy, etc.
  • App may want to stop the exception propagation.

Please let me know if you have any questions.

@lukehutch
Copy link
Author

A user should not have to catch all exceptions, anywhere they could be thrown, just to be able to see what went wrong when something unexpected goes wrong.

In Java, most exceptions are checked exceptions, so try-catch is used wherever exceptions are expected, and unchecked exceptions are only used for things that really are never supposed to happen, and when they do happen, it's serious. In Javascript, there are no checked exceptions whatsoever, so the chance of an exception being thrown that you didn't expect is very high. For this very reason, it is important to preserve all stack traces in code that runs in Javascript AND code that is run by Javascript. The user simply cannot be expected to surround every operation with try/catch.

And even if they do use try/catch in Javascript, this is not "resolved by other approaches" to use your word, because the JS stack trace will not show the Java stack trace, if an exception is thrown in Java. Saving and re-throwing the exception doesn't preserve information that is not there.

The caller of V8 needs to catch Javet exceptions. But those exceptions won't have the information the caller needs, if an exception is thrown in Java code run by V8.

Why doesn't Javet build it in? Because there are many things to be customized here. Javet leaves the freedom to the Javet users.

  • App may want to preserve inner exceptions, exception hierarchy, etc.
  • App may want to stop the exception propagation.

Nothing I have proposed precludes a user from still doing those things. I am not talking about changing how or where exceptions are caught, or how they are handled. I am ONLY saying that exceptions need to include all information about ALL stack frames, in both JS and Java, regardless of where the exception is caught, or how it is handled.

This is a major usability issue for Javet, in my opinion, so I don't know why you don't want to make this work end-to-end. I don't know the internals of Javet, but I don't see why this would be difficult or impossible to implement.

BTW as far as I understand it, you can do better than just adding the Java stacktrace as part of the JS error message, by using Error.prepareStackTrace, and passing in structured information about each stack frame. In Java, you can override Exception.fillInStackTrace or Exception.getStackTrace to add custom stack frames, or you can call Exception.setStackTrace.

@caoccao
Copy link
Owner

caoccao commented Jan 21, 2025

Things are not as simple as you thought. V8 value cannot hold Java objects in V8, otherwise memory leak will happen. All API that imply multiple steps of inter-operation between JS and Java objects will cause potential memory leak, because step 2, 3, ...etc. may not happen. The simple solution is to convert the Java objects to native V8 values.

@lukehutch
Copy link
Author

You can just read the stack frames in Java, turn them into a stringified JSON object, and send a single string back to V8. And actually you could use V8's own stack frame JSON schema for this, so you can just plug the returned JSON straight into the V8 structured stack frame array.

@lukehutch
Copy link
Author

Actually, correction: you would need to wrap these structured JSON stack frames with a CallSite instance that simply returns the appropriate info from a specific stack frame, then insert the CallSite instance for each stack frame into the stacktrace in V8.

@caoccao
Copy link
Owner

caoccao commented Jan 22, 2025

That won't be feasible for the following reasons.

  1. It won't be accurate. Json cannot represent datetime, long, bigint, etc.
  2. It implies a huge performance overhead because getting the whole stack trace is very expensive. That could even crash the runtime in edge cases.
  3. It implies security vulnerabilities. The top 1 reason that Javet doesn't expose the stack trace is nothing should be exploited by the guest JS scripts so that the guest scripts couldn't tell if it's being excused in JVM.

You can always embed the stack trace in the error message by your custom exception implementation. The message can be in Json, XML, whatever format you want.

@lukehutch
Copy link
Author

  1. It won't be accurate. Json cannot represent datetime, long, bigint, etc.

The only types that need to be passed from a Java function that threw an exception back to JS are strings (filename, function name, type name of this) and ints (line number, column number), to be able to return these values from CallSite. There are no datetime, long, bigint etc. fields either in a Java StackFrame (as long as you don't populate an Value or LocalVariable values), or anything that expects them in CallSite.

2. It implies a huge performance overhead because getting the whole stack trace is very expensive. That could even crash the runtime in edge cases.

Literally every single time an Exception is constructed, the stack trace is obtained. The only way to avoid this (to make exceptions very very fast) is to override fillInStackTrace with an empty method body, which almost no Exception subclass actually does.

If you have caught a Java exception, the entire stack trace has already been obtained and stored (in fact it was created even before throw was called, because it happens at time of construction of the Exception). You can't do anything about the fact that this happened before you had any control over it. Using the stacktrace takes close to zero time after that.

3. It implies security vulnerabilities. The top 1 reason that Javet doesn't expose the stack trace is nothing should be exploited by the guest JS scripts so that the guest scripts couldn't tell if it's being excused in JVM.

What about the (I would claim very common) usecase where the Javet user wrote both the JS code, and the Java code it is calling? If you're calling custom Java functions from JS, then probably you wrote the JS code. Therefore, there are no security concerns in this case, and having a hobbled Javet, with no ability to get Javet to "do the right thing" and show all stack frames on exceptions, is a major usability issue.

Now I am definitely a fan of security by default. So if you want to make Javet not create hybrid stacktraces by default, then that's fine -- but please add a switch to enable hybrid stacktraces in V8Options and NodeOptions, and please add to the stacktrace at least Java stack frames elided -- enable full stacktrace using V8Options.enableHybridStackFrames(true) or similar, so that it is easy to see how to get the info when something goes wrong.

@caoccao
Copy link
Owner

caoccao commented Jan 22, 2025

Thank you for sharing your thoughts.

  1. People sometimes extend Java exceptions to carry more data that may consist of datetime, long, BigInt, BigDecimal, etc. They want those customized member properties to be carried over to the JS Error as member properties as well. Json cannot be used at all.
  2. The performance overhead is at recursively converting the Java stack trace to the JS counterparts. Most of people don't want to pay the overhead they don't consume at all.
  3. As it's a highly customizable feature, one flag would be far away from the need. Someone else would raise concerns on that flag. That's also why Javet doesn't step into this kind of customizations. There are some ways one of which was presented by me in the previous reply.

@lukehutch
Copy link
Author

  1. People sometimes extend Java exceptions to carry more data that may consist of datetime, long, BigInt, BigDecimal, etc. They want those customized member properties to be carried over to the JS Error as member properties as well. Json cannot be used at all.

You don't need to pass the entire Exception instance back to JS, just to enable complete stack frame information.

It would be highly unusual for someone to extend Exception with some extra metadata field (already very unusual), and to expect that extra field to be readable by calling JS code (even more unusual).

You would meet 99.999% of the need (specifically the very common need of debugging) by providing only stacktrace info, not the full exception object.

2. The performance overhead is at recursively converting the Java stack trace to the JS counterparts. Most of people don't want to pay the overhead they don't consume at all.

Java programmers should know that exception production is expensive, and if they dig a little, they will discover they can make exceptions really fast by overriding fillInStackTrace.

But again, we are talking about providing debug info in the case of unexpected exceptions specifically, which (1) should be a rare event, and/or (2) should only happen during code development (therefore speed is not an issue, because the programmer cares far more about correctness and completeness than speed, at least initially), and/or (3) if an unexpected exception is thrown, it should be a showstopper since the exception is not caught anywhere (so who cares about overhead? It's a showstopper).

3. As it's a highly customizable feature, one flag would be far away from the need. Someone else would raise concerns on that flag. That's also why Javet doesn't step into this kind of customizations. There are some ways one of which was presented by me in the previous reply.

Well then I suggest making the generation of full stack trace the only behavior, and then telling Java users who call from their code into untrusted JS code, and then back into their own code, that if they care about security and don't want to expose any internals of their nested Java code to JS, they need to ensure they never throw an exception back to JS. This is a totally fair and reasonable thing for extremely security-conscious people to have to do anyway, EVEN if they never leave the Java world (e.g. their Java code calls an untrusted library, providing a callback to that library, which calls one of the user's own functions).

In other words, your security concerns have nothing whatsoever to do with the fact that JS is inserted between Java and Java; it has to do with trusted code calling untrusted code, which calls user-supplied trusted code again that has to not expose its inner workings to the untrusted code.

@caoccao
Copy link
Owner

caoccao commented Jan 22, 2025

I have been always talking about general needs from various product requirements' perspective. I'm under the impression that you are focused on your need. Thus, I wonder the discussion wouldn't be fruitful if we are constantly in the separate channels.

@lukehutch
Copy link
Author

No, literally every argument I have made has been along the lines of "here is what the most likely usecase is" or "here is what programmers are most likely to expect".

@caoccao
Copy link
Owner

caoccao commented Jan 23, 2025

Edge cases are also critical to a framework / SDK developers.

Most of use cases don't need the stack trace, and don't have to take the performance overhead. If you are not aware of those use cases, please browse other issues to get some ideas. They just need a way to stop the execution as quickly as possible.

You custom needs can be achieved by yourself, but not suitable to be built in to the SDK.

@lukehutch
Copy link
Author

I don't understand -- you are saying it is important for a framework like Javet to handle edge cases, and then you are asking me to handle my own edge cases?

And yet I don't believe that the needs I am describing are edge cases at all. I am talking about basic debugging functionality that EVERY developer will need at some point, until they have a fully running system, where, sure, maybe they won't want to take a performance hit for exceptions. During debugging, you NEED the information about what went wrong when something went wrong.

It is fine if you don't enable this by default, but why do you not want to add an option to pass back at least source files and line numbers on exception?

And no, I can't implement this functionality myself, because when an exception is thrown, I need to pass back the stack trace of the exception from the Java function that threw the exception, and if the return type is say int, there is no way at all to pass back even a String containing the stack information. No, I'm not going to write to a global static variable on exception to record the exception information, or some other ugly and dangerous hack like that.

There is literally no way at all to handle this, other than Javet adding support for developers to see the source of exceptions when they happen.

@caoccao
Copy link
Owner

caoccao commented Jan 23, 2025

You can pass anything via error message in arbitrary format.

  1. Encode whatever you want to pass on, e.g. stack trace, user id, transaction id, etc. and set the encoded string to exception message.
  2. Javet sends the exception message from Java to JS error message.
  3. Decode the JS error message in JavaScript to get whatever you want to get.

You can do that.

Most of use cases don't need to debug at all. They just want to terminate the script execution as soon as possible.

@lukehutch
Copy link
Author

The whole reason I filed this issue in the first place is that Javet shows no information whatsoever about Java exceptions in the stacktrace, only Error: Uncaught JavaError in function callback -- it does not even show the exception error message.

@caoccao
Copy link
Owner

caoccao commented Jan 23, 2025

That's because the message of that Java exception is null. As I emphasized, you need to create your own exception with arbitrary message format to satisfy your requirements.

@lukehutch
Copy link
Author

As the test project I created shows, I threw a custom exception with a non-null message. It is wrapped in an Error by the JVM, and your library is ignoring "Caused by" information of wrapped exceptions.

@caoccao
Copy link
Owner

caoccao commented Jan 23, 2025

I wonder what makes it so hard to write a PoC with a custom exception.

@lukehutch
Copy link
Author

No need to be so patronizing. Obviously throwing a custom exception is not hard. But that is not the solution I am asking for, because having to proactively wrap ALL code just in case an unchecked exception is thrown is not a solution. Honestly that is a ridiculous thing to require of your users.

I don't know why you are resisting so hard helping your users to actually see what went wrong when unexpected things go wrong. This is NOT just about my specific use case. It's about a fundamental flaw in the functioning of Javet.

The amount of code required for this will not be large. Please just put it behind a config switch, if you are so concerned about overhead.

@caoccao
Copy link
Owner

caoccao commented Jan 23, 2025

Please post your PoC and you will find there are numerous ways of customizations.

  • Filter by exception type
  • Filter by message via regexp
  • If outer exception message is null, recursively look up the inner exception till the message is not null (your case)

Those customizations cannot be built into Javet. Please do it by yourself. Other Javet users don't have to pay the performance penalty. Even a flag / enum in the config will slow down the script execution because many other users use exceptions as a cheap way of communication between JVM and V8.

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

No branches or pull requests

2 participants