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

pass ScriptRuntime.emptyArgs in getApplyArguments() #1551

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

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Aug 2, 2024

Sorry folks, but i have no test case for this - but i found this in a complex script scenario. Maybe someone has an idea how to test this.

What is this about: the parameter handling for the apply function replaces many args with emptyArgs. But this replacements does not check for emptyArgs itself - in this case the call throws an error. Therefore i added the check and the js code now runs fine.

@gbrail
Copy link
Collaborator

gbrail commented Aug 8, 2024

Sure, I get it, and we can add this check, but I feel like something very strange must be going wrong in your test case. It looks like that function is supposed to be called with the first argument -- if that new check is necessary it implies to me that someone is passing in an arguments array with a first argument that's itself an empty array, and that sounds wrong. So far you sure that there isn't a bug somewhere else first?

@tonygermano
Copy link
Contributor

I've been trying, and I can't figure out how to reproduce this. Even if I call apply with org.mozilla.javascript.ScriptRuntime.emptyArgs as the second arg, it's still fine because that ends up being wrapped in a NativeJavaArray, which is an instance of Scriptable and is array-like.

What is the error you are seeing when it happens?

@rbri
Copy link
Collaborator Author

rbri commented Aug 10, 2024

@tonygermano sorry but i also spend a lot of time trying to nail down this thing. Found this while trying to get a huge web page with all this f**** javascript that tracks you running in HtmlUnit. But finally i gave up and made this 'fix'. Sorry i do not have the web page and my local setup at hand. There are soo many reports i have to work on.

Maybe we can leave this and see if some day a similar scenario pops up....

@anonyein
Copy link

anonyein commented Aug 10, 2024

@tonygermano sorry but i also spend a lot of time trying to nail down this thing. Found this while trying to get a huge web page with all this f**** javascript that tracks you running in HtmlUnit. But finally i gave up and made this 'fix'. Sorry i do not have the web page and my local setup at hand. There are soo many reports i have to work on.

Maybe we can leave this and see if some day a similar scenario pops up....

htmlunit-rhino-fork
I use your fork and run smoothly, only with some missing of parameters such as "https://github.com/HtmlUnit/htmlunit-rhino-fork/blob/master/rhino/src/main/java/org/mozilla/javascript/ImporterTopLevel.java#L143" (and many other places)

@p-bakker
Copy link
Collaborator

@rbri do you want this PR hanging around or can we close it? Can always reopen it when the cause it's found

@rbri
Copy link
Collaborator Author

rbri commented Aug 19, 2024

@rbri do you want this PR hanging around or can we close it? Can always reopen it when the cause it's found

Do whatever you like - as always i have done this for core-js, my problem is gone without any negative effects for my cases (users). This was only to get rhino informed about my finding.

@p-bakker p-bakker added the JSR 223 Script Engine Issues related to the JSR 223 Script Engine wrapper label Aug 24, 2024
@p-bakker p-bakker marked this pull request as draft September 8, 2024 06:45
@p-bakker
Copy link
Collaborator

p-bakker commented Sep 8, 2024

Converted to a draft, as based on the discussion a testcase is needed for this PR to be considered and as of this moment providing one has proved to be a challenge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSR 223 Script Engine Issues related to the JSR 223 Script Engine wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants