RFR: 8150219: ReferenceError in 1.8.0_72

[Hannes Wallnöfer]

I’ve uploaded a new webrev that reintroduces the Global.initscontext field instead of reusing the current context thread local so it will behave the same from all threads. Please review.

http://cr.openjdk.java.net/~hannesw/8150219/webrev.01/

Thanks,
Hannes


> Am 14.06.2016 um 10:22 schrieb Hannes Wallnöfer <hannes.wallnoefer at oracle.com>:
> 
> Thanks for the comments, Sundar.
> 
> I agree that using the existing ScriptContext thread local is a bit of a hack. I chose this path because it allowed me to keep the changes minimal and fix the problem for the most predominant and reasonable use case.
> 
> What I absolutely do not agree with is to have a method to evaluate a script with a non-default ScriptContext and then ignore that ScriptContext on invocation of a function obtained from that evaluation. This breaks about every assumption a somewhat experienced JavaScript developer would have, and it also is a regression against JDK up to u71. 
> 
> Note that my change does not go back to the behavior of pre-u72, so engine’s default ScriptContext is still used for all globals created the „normal“ way (e.g. using NashornScriptEngine.createBindings method, or the default global created in NashornScriptEngine constructor). The only case we set the engine this way is for evals with a non-default ScriptEngine which does not already have a Nashorn global. This is of course also reflected by the tests you added for JDK-8138616 still passing.
> 
> I hope we can find some common ground here.
> 
> Hannes
> 
> 
>> Am 14.06.2016 um 04:32 schrieb Sundararajan Athijegannathan <sundararajan.athijegannathan at oracle.com>:
>> 
>> Sorry, I'm not sure I'm comfortable with this fix :(
>> 
>> Global.setScriptContext sets the ScriptContext for the *current thread*
>> [because that Global class methods set thread local]. This means that
>> this ReferenceError goes away only for the thread in a new Global is
>> created and associated with that ScriptContext. For other threads,
>> you'll continue to get ReferenceError.
>> 
>> The root of the issue is the Invocable's methods are expected to use the
>> default ScriptContext. That means that if you eval code or put globals
>> in  another ScriptContext, you can't do Invocable calls on it! That is
>> the limitation of javax.script. While eval's have ScriptContext,
>> Bindings accepting variants, there are no similar methods exists for
>> Invocable and Compilable. These two use script engine's default
>> ScriptContext!
>> 
>> BTW, there is clear Nashorn alternative - which is to use
>> jdk.scripting.api.scripting.ScriptObjectMirror's call or eval method.
>> These take care of using the right global in which the function was
>> defined - lexical scoping is respected as expected!
>> 
>> Besides the current "fix" works only the particular thread which feels
>> wrong as well.
>> 
>> -Sundar
>> 
>> On 6/14/2016 1:04 AM, Attila Szegedi wrote:
>>> +1
>>> 
>>>> On 13 Jun 2016, at 18:56, Hannes Wallnöfer <hannes.wallnoefer at oracle.com> wrote:
>>>> 
>>>> Please review 8150219: ReferenceError in 1.8.0_72:
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150219
>>>> Webrev: http://cr.openjdk.java.net/~hannesw/8150219/webrev.00/
>>>> 
>>>> This is a bit of a compromise that partially restores the behavior we had before JDK-8138616 by associating a ScriptContext with a Nashorn Global, but only in the very specific case where a script is evaluated with a non-default ScriptContext that does not have a Nashorn Global yet, and the Global is created for that specific ScriptContext. Also, we use the existing setScriptContext method and ThreadLocal field in Global instead of introducing an extra field as we had it before. 
>>>> 
>>>> The purpose is to make functions obtained from such globals use the ScriptContext they were evaluated with. 
>>>> 
>>>> Thanks,
>>>> Hannes
>>>> 
>> 
>