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

Changes the implementation of Error.captureStackTrace so that it does not need to create new Error() #1436

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnspackman
Copy link

This fixes a problem where a stack overflow will occur when the global Error class has been replaced, such as in the recent implementation of core-js polyfill (see https://github.com/zloirock/core-js).

Only recent revisions of core-js have triggered the stack overflow, and the polyfill is essential - the Qooxdoo.org project supports Rhino as a target and incorporating core-js is mandatory.

This change is 100% backwards compatible.

…es not need to create `new Error()` in order to provide the stack trace; this fixes a problem where a stack overflow can occur when the global `Error` class has been replaced, such as in the recent implementation of `core-js` polyfill (see https://github.com/zloirock/core-js)
@gbrail
Copy link
Collaborator

gbrail commented Jan 15, 2024

I'm in favor of doing this.

To get the tests to pass, you'll need to ensure that "./gradlew check" passes. It looks like the formatting isn't what the "spotless" tool expects, and you can fix that by first running "./gradlew spotlessApply".

Thanks!

@gbrail
Copy link
Collaborator

gbrail commented Jan 15, 2024

Two other things:

  1. It'd be great to see a way to test to verify that this is really fixed. First choice would be a test case that formerly produced a StackOverflow error.
  2. I'm not sure how I feel about throwing an IllegalStateException in the case of recursion, because I'm not sure how well users of Rhino are going to be able to handle having Rhino throw raw Java exceptions instead of some sort of JavaScript error, like by throwing the exception produced by ScriptRuntime.constructError.

@johnspackman
Copy link
Author

  1. It'd be great to see a way to test to verify that this is really fixed. First choice would be a test case that formerly produced a StackOverflow error.

I came across it only when a minor-version upgrade to the core-js package was installed; and the polyfill is quite convoluted and spread over a number of lines. I'm not confident it'll be at all easy to reproduce without pulling in the core-js code.

OTOH the original Rhino code says that it was just trying to reuse the code from the constructor of Error - I don't know if something's changed, but the code seems more straightforward this way.

Would you be comfortable if there was unit tests to show that it still works this way, i.e. rather than test for the stack overflow, test instead that the captureStackTrace still works as expected?

  1. I'm not sure how I feel about throwing an IllegalStateException in the case of recursion

No problem, also the unit tests and formatting

@johnspackman
Copy link
Author

  1. I'm not sure how I feel about throwing an IllegalStateException in the case of recursion

No problem, also the unit tests and formatting

Ah - hang on, the problem would be that throwing a JavaScript error would cause the stack overflow exception; that throw should (in theory at least) never be thrown any more, but I had to add it so that I could set a breakpoint and catch the problem. Probably best if I just delete it?

@johnspackman
Copy link
Author

How do you debug the javascript code as it is being run by rhino? I've got "a" debugger but only one that works inside of my framework.

@gbrail
Copy link
Collaborator

gbrail commented Jan 24, 2024

Thanks for continuing to look at this. I have been able to use "normal" debuggers like the one built-in to VS Code with Rhino, but that's primarily to debug the Java code. For debugging the JavaScript directly it's a very different thing, although the debugger built in to Rhino (in the "toolsrc" directory) has given some people help in some circumstancesl.

@p-bakker
Copy link
Collaborator

#366 seems to be concerned with the same part of the code that creates a new object inside captureStackTrace

If this PR gets merged, the issue for which #366 got created likely is resolved as well (assuming it not already is somehow)

@p-bakker
Copy link
Collaborator

@johnspackman any update on this? Is there anything you need from us to move this forward?

@johnspackman
Copy link
Author

@johnspackman any update on this? Is there anything you need from us to move this forward?

No update unfortunately; the issue I have is that in order to add a unit test, I'm struggling to reproduce it - what was failing for me is too big to include in a unit test, and a lack of debugger for the javascript made life hard. Although there is a debugger in toolsrc, I havn't had a chance to look at it, and AFAICR I also had issues with just running the existing unit tests. I think I ended up giving up because the simple task of "add a unit test" blew up into a large amount of work just to be able to get to the starting block, so to speak and I ran out of time

@p-bakker p-bakker marked this pull request as draft September 8, 2024 06:51
@p-bakker
Copy link
Collaborator

p-bakker commented Sep 8, 2024

Converted to a draft as it's missing a testcase

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

Successfully merging this pull request may close these issues.

3 participants