Review request for JDK-8133299: Nashorn Java adapters should not early bind to functions

[Sundararajan Athijegannathan]

Looks good to me.

Yes, I did apply the patch and checked the generated code as well and 
also tested few things (manually) with jjs.

* If we want to backport this to jdk8u (?), we may want to avoid lambdas 
or have to prepare adjusted patch for jdk8u.

* could there be a simple JS test like?

var obj = {
   run: function() {
     print("hello")
   }
}

var r = new java.lang.Runnable(obj);
// "hello" printed
r.run()
obj.run = function() { print("world") }
// "world" printed
r.run()

-Sundar

On 1/15/2016 9:08 PM, Attila Szegedi wrote:
> Please review the second take on JDK-8133299 "Nashorn Java adapters should not early bind to functions" at <http://cr.openjdk.java.net/~attila/8133299/webrev.jdk9.01> for <https://bugs.openjdk.java.net/browse/JDK-8133299>
>
> I made some further changes:
>
> - while the code would now be able to invoke just about any Nashorn callable object as an implementation for an adapter method, I restricted it to ScriptFunction only following a discussion with Sundar. The old adapter only supported ScriptFunction, so there's no backwards compatibility issue.
> - I have also made the bytecode of the adapter methods much smaller, by extracting some oft-repeated bytecode sequences into static methods on JavaAdapterServices (wrapThrowable(), unsupported()).
> - I reworked the way setGlobal works, again drastically reducing the bytecode size and moving all of the logic into JavaAdapterServices. JavaAdapterServices.setGlobal() now sets the necessary global and returns a Runnable that when run() will restore the previous global. The bytecode thus no longer has any logic for dealing with globals; it just invokes JAS.setGlobal(), stores the returned Runnable, and invokes run() on it in finally block.
> - The generator also detects when the INVOKEDYNAMIC dyn:call would need more than 255 slots for its parameter list, and introduces a Nashorn-compliant (Object callee, Object this, Object[] args) variable arity invocation in its place.
> - Added a fairly comprehensive test suite.
>
> With these changes, this work should be complete.
>
> It’s probably hard to review the changes in JavaAdapterBytecodeGenerator just by looking at the diff, as the changes are quite significant, amounting to a partial rewrite. It’s probably easier to apply the diff and review the resulting new code of the class. A fairly good way to review the effect of these changes is to add “-pc” to line 102 of JavaAdapterTest.java and run it with and without the rest of the changes applied to see what kind of code is being generated.
>
> Attila.
>
>> On Jan 4, 2016, at 2:48 PM, Attila Szegedi <szegedia at gmail.com> wrote:
>>
>>
>> On Mon, Jan 4, 2016 at 5:13 AM, Sundararajan Athijegannathan <sundararajan.athijegannathan at oracle.com> wrote:
>> I'm yet to look at the code.  Is it possible to use any callable for methods?
>>
>> For members of a delegate ScriptObject, yes. The adapter constructors still only accept either a ScriptObject or a ScriptFunction, but if you pass in an object, its members implementing methods can now be arbitrary callables.
>>   
>> If so, do we have tests to cover those cases (like JSObject "functions",  DynamicMethods etc.)? In particular, sandbox tests to make sure can't get any more privilege by implementing an interface (for eg. binding sensitive Java method as function implementing interface method and making sure it gets SecurityException when interface method is called).
>>
>> We generate separate adapter classes for every ProtectionDomain, so I'm quite confident everything is secure, but it's better to not be overconfident when it comes to security… surely we can add such tests. Do you have some go-to sensitive functionality for testing in this fashion?
>>
>> FWIW, I actually started writing some tests for other functionality; what I have so far is tests for correct conversion of primitive return values as well as tests for try/finally for restoring old Global being executed when UnsupportedOperationException is thrown. (All tests I wrote pass, so the code as it is now is correct for these.) I wanted to write few more tests, specifically for binding to vararg functions.
>>
>> Actually, I realized that there's one deficiency in the current code that wasn't there before my changes, namely since the CALL operation takes explicit callee and this arguments, the adapter code will actually fail to generate a delegate for a method with more than 253 parameters. I might need to introduce a special case for this, to have them packed into an Object array in this case and do a Nashorn vararg invocation…
>>
>> Speaking of which, anyone knows why is LinkerCallSite.ARGLIMIT set to 250 and not 253?
>>
>> Attila.
>>   
>>
>> Thanks,
>> -Sundar
>>
>>
>> On 1/2/2016 8:31 PM, Attila Szegedi wrote:
>> Please review JDK-8133299 "Nashorn Java adapters should not early bind to functions" at <http://cr.openjdk.java.net/~attila/8133299/webrev.jdk9> for <https://bugs.openjdk.java.net/browse/JDK-8133299>
>>
>> See implementation notes in <https://bugs.openjdk.java.net/browse/JDK-8133299?focusedCommentId=13883269&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13883269>
>>
>> Also note that this changeset is based on current tip (rev 1584, [da397aea8ada]) and is as such independent of the change sets for JDK-8144917 and JDK-8144919 that are still pending review.
>>
>> Thanks,
>>     Attila.
>>
>>