-
Notifications
You must be signed in to change notification settings - Fork 858
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
When InterfaceAdapter is used, the wrong thisObj is used #1453
base: master
Are you sure you want to change the base?
When InterfaceAdapter is used, the wrong thisObj is used #1453
Conversation
@@ -152,9 +152,8 @@ Object invokeImpl( | |||
} | |||
} | |||
} | |||
Scriptable thisObj = wf.wrapAsJavaObject(cx, topScope, thisObject, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thisObject
is an instance of JavaAdapterInvokeTest.AdapterClass
and we want to call a method on target
This seems logical to me but it's a pretty fundamental thing to the Java embedding support and I'd like to see someone who works with this stuff regularly take a look before merging it. |
testsrc/org/mozilla/javascript/tests/JavaAdapterInvokeTest.java
Outdated
Show resolved
Hide resolved
testsrc/org/mozilla/javascript/tests/JavaAdapterInvokeTest.java
Outdated
Show resolved
Hide resolved
Not a regular user of code that uses code constructs like the testcases, calling a java method passing as argument a JavaScript objects that implements a java interface, but the change looks ok to me |
I'd love to see a few of the questions answered on the PR above and then see if it makes sense to merge this. |
+ "function Obj() { this.myObj = {one: 1} }\n" | ||
+ "Obj.prototype.seven = 7\n" | ||
+ "Obj.prototype.m1 = function(i) { return i + this.myObj.one }\n" | ||
+ "Obj.prototype.m2 = function() { return this.seven }\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test shows the problem. this.myObj
and this.seven
were undefined, when the js object is passed in an InterfaceAdapter
The other tests are all passing without the fix and are for completeness, not to break anything else
@gbrail I resolved the merge conflicts and updated the test with some documentation |
Hmm. Seems that there might be a flaky test
Will investigate later |
Converted this to a draft as it currently requires more work as the pipeline fails Please remove the draft status once the pipeline passed and the PR can be considered for merging,. This way it allows the committers to focus on ready-to-merge PRs |
I've added a more realistic testcase where i use |
I had an issue when I pass a JS object to Java class and an InterfaceAdapter is constructed:
Java interface
Corresponding impl. in javascript:
When I pass this object in my AdapterClass, I get the error:
this.myObj is undefined
This PR provides a test and a possible fix