Re: [8u] approval request for 8157160: JSON.stringify does not work on ScriptObjectMirror objects

[Jim Laskey (Oracle)]

+1

> On May 20, 2016, at 11:51 AM, Sundararajan Athijegannathan 
>  wrote:
> 
> Please approve.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8157160
> 
> 9 review thread:
> http://mail.openjdk.java.net/pipermail/nashorn-dev/2016-May/006179.html
> 
> 8u webrev: http://cr.openjdk.java.net/~sundar/8157160/8u/webrev.00/
> 
> The patch wouldn't apply "as is" because of slight difference in
> dynalink/Bootstrap code. I'm CC'ing this email to nashorn-dev as well
> for backport review.
> 
> Thanks,
> 
> -Sundar
> 
> 

Re: [8u] approval request for 8157160: JSON.stringify does not work on ScriptObjectMirror objects

[Seán Coffey]

Approved for jdk8u-dev but subject to peer review before pushing changes.

Regards,
Sean.

On 20/05/2016 15:51, Sundararajan Athijegannathan wrote:

Please approve.

Bug: https://bugs.openjdk.java.net/browse/JDK-8157160

9 review thread:
http://mail.openjdk.java.net/pipermail/nashorn-dev/2016-May/006179.html

8u webrev: http://cr.openjdk.java.net/~sundar/8157160/8u/webrev.00/

The patch wouldn't apply "as is" because of slight difference in
dynalink/Bootstrap code. I'm CC'ing this email to nashorn-dev as well
for backport review.

Thanks,

-Sundar

[8u] approval request for 8157160: JSON.stringify does not work on ScriptObjectMirror objects

[Sundararajan Athijegannathan]

Please approve.

Bug: https://bugs.openjdk.java.net/browse/JDK-8157160

9 review thread:
http://mail.openjdk.java.net/pipermail/nashorn-dev/2016-May/006179.html

8u webrev: http://cr.openjdk.java.net/~sundar/8157160/8u/webrev.00/

The patch wouldn't apply "as is" because of slight difference in
dynalink/Bootstrap code. I'm CC'ing this email to nashorn-dev as well
for backport review.

Thanks,

-Sundar

Re: RFR(XXS): 8157444: exclude jjs shebang handling test from runs

[Hannes Wallnoefer]

+1

Am 2016-05-20 um 15:47 schrieb Michael Haupt:

Dear all,

please review this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8157444
Webrev: http://cr.openjdk.java.net/~mhaupt/8157444/webrev.00/

Thanks,

Michael

Re: RFR(XXS): 8157444: exclude jjs shebang handling test from runs

[Jim Laskey (Oracle)]

+1

> On May 20, 2016, at 10:47 AM, Michael Haupt  wrote:
> 
> Dear all,
> 
> please review this fix.
> Bug: https://bugs.openjdk.java.net/browse/JDK-8157444
> Webrev: http://cr.openjdk.java.net/~mhaupt/8157444/webrev.00/
> 
> Thanks,
> 
> Michael
> 
> -- 
> 
> 
> Dr. Michael Haupt | Principal Member of Technical Staff
> Phone: +49 331 200 7277 | Fax: +49 331 200 7561
> Oracle Java Platform Group | LangTools Team | Nashorn
> Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, 
> Germany
> 
> ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
> München
> Registergericht: Amtsgericht München, HRA 95603
> 
> Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
> 3543 AS Utrecht, Niederlande
> Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
> Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
> Oracle is committed to developing 
> practices and products that help protect the environment
> 

Re: RFR(XXS): 8157444: exclude jjs shebang handling test from runs

[Sundararajan Athijegannathan]

+1


On 5/20/2016 7:17 PM, Michael Haupt wrote:
> Dear all,
>
> please review this fix.
> Bug: https://bugs.openjdk.java.net/browse/JDK-8157444
> Webrev: http://cr.openjdk.java.net/~mhaupt/8157444/webrev.00/
>
> Thanks,
>
> Michael
>

RFR(XXS): 8157444: exclude jjs shebang handling test from runs

[Michael Haupt]

Dear all,

please review this fix.
Bug: https://bugs.openjdk.java.net/browse/JDK-8157444
Webrev: http://cr.openjdk.java.net/~mhaupt/8157444/webrev.00/

Thanks,

Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: RFR: 8131017: jshell tool: pasting code with tabs invokes tab completion

[Sundararajan Athijegannathan]

Looks good.

-Sundar


On 5/20/2016 4:14 PM, Jan Lahoda wrote:
> Please review.
>
> Problem: when pasting code that contains tab characters into both
> jshell and jjs, the tab completion gets invoked on tabs instead of
> using the tab character itself.
>
> The proposed solution is to set setCopyPasteDetection(true) on jline's
> ConsoleReader, which should enable jline's copy-paste detection, and
> avoid invoking the tab completion on paste. A limitation of the
> detection is that it cannot detect trailing tab characters, only can
> detect tab character followed by at least one more character. I hope
> that shouldn't be a big problem.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8131017
>
> Webrev - langtools:
> http://cr.openjdk.java.net/~jlahoda/8131017/langtools/webrev.00/
>
> Webrev - nashorn:
> http://cr.openjdk.java.net/~jlahoda/8131017/nashorn/webrev.00/
>
> Thanks,
>  Jan

RFR: 8131017: jshell tool: pasting code with tabs invokes tab completion

[Jan Lahoda]

Please review.

Problem: when pasting code that contains tab characters into both jshell 
and jjs, the tab completion gets invoked on tabs instead of using the 
tab character itself.


The proposed solution is to set setCopyPasteDetection(true) on jline's 
ConsoleReader, which should enable jline's copy-paste detection, and 
avoid invoking the tab completion on paste. A limitation of the 
detection is that it cannot detect trailing tab characters, only can 
detect tab character followed by at least one more character. I hope 
that shouldn't be a big problem.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8131017

Webrev - langtools:
http://cr.openjdk.java.net/~jlahoda/8131017/langtools/webrev.00/

Webrev - nashorn:
http://cr.openjdk.java.net/~jlahoda/8131017/nashorn/webrev.00/

Thanks,
 Jan

Re: RFR 8157310: jdk.dynalink.linker.support.Lookup should have more checks before adding module read link

[Attila Szegedi]

Seconded, this is indeed better. +1

> On 20 May 2016, at 10:38, Hannes Wallnoefer  
> wrote:
> 
> Hi Sundar,
> 
> I agree methods feel more in their place now.
> 
> Hannes
> 
> Am 2016-05-20 um 09:12 schrieb Sundararajan Athijegannathan:
>> Hi,
>> 
>> Thanks for the review. But, I adjusted the change slightly -- moved
>> addModuleRead and caller-sensitive fallback attempt to
>> CallerSensitiveDynamicMethod itself - leaving Lookup as general
>> exception translating version of j.l.invoke.MethodHandle.Lookup.
>> 
>> I think this is better caller sensitivity handling moved to specific
>> place and Lookup remains a generic API.
>> 
>> http://cr.openjdk.java.net/~sundar/8157310/webrev.01/
>> 
>> Thanks,
>> 
>> -Sundar
>> 
>> 
>> On 5/20/2016 12:22 PM, Attila Szegedi wrote:
>>> +1. Nice work!
>>> 
 On 19 May 2016, at 15:28, Sundararajan Athijegannathan 
  wrote:
 
 Please review http://cr.openjdk.java.net/~sundar/8157310/ for
 https://bugs.openjdk.java.net/browse/JDK-8157310
 
 What is this fix?
 
 * When unreflecting caller sensitive methods, dynalink uses specific
 Lookup of actual caller (instead of publicLookup) - in nashorn's case
 it's be the Lookup of script class.
 
 * Script class may not have read link to the module of the class (of
 caller sensitive member). If there is any IllegalAccessError, dynalink
 adds the read link and tries to
 
 unreflect again. We don't want unnecessary module link read in such
 cases. Check has been added whether the package is exported from
 declaring module. Note that there is no security issue here.  Even
 before the fix, module read edge does not give any capability. unreflect
 call after adding unnecessary read line would still fail for
 non-exported package case. There were unnecessary module read links
 created - which is being avoided now.
 
 * Additional "API" in Lookup.java slipped from jigsaw code into
 jdk9-dev. Those unreflectCallerSensitive and
 unreflectConstructorCallerSensitive were meant to be 'interim' somehow
 slipped.  Now, only unreflect and unreflectConstructor in Lookup. Caller
 sensitiveness is hidden in the implementation.  These methods were never
 APIs when dynalink API was created.
 
 Thanks,
 
 -Sundar
 
 
> 

Re: RFR 8157310: jdk.dynalink.linker.support.Lookup should have more checks before adding module read link

[Hannes Wallnoefer]

Hi Sundar,

I agree methods feel more in their place now.

Hannes

Am 2016-05-20 um 09:12 schrieb Sundararajan Athijegannathan:

Hi,

Thanks for the review. But, I adjusted the change slightly -- moved
addModuleRead and caller-sensitive fallback attempt to
CallerSensitiveDynamicMethod itself - leaving Lookup as general
exception translating version of j.l.invoke.MethodHandle.Lookup.

I think this is better caller sensitivity handling moved to specific
place and Lookup remains a generic API.

http://cr.openjdk.java.net/~sundar/8157310/webrev.01/

Thanks,

-Sundar


On 5/20/2016 12:22 PM, Attila Szegedi wrote:

+1. Nice work!


On 19 May 2016, at 15:28, Sundararajan Athijegannathan 
 wrote:

Please review http://cr.openjdk.java.net/~sundar/8157310/ for
https://bugs.openjdk.java.net/browse/JDK-8157310

What is this fix?

* When unreflecting caller sensitive methods, dynalink uses specific
Lookup of actual caller (instead of publicLookup) - in nashorn's case
it's be the Lookup of script class.

* Script class may not have read link to the module of the class (of
caller sensitive member). If there is any IllegalAccessError, dynalink
adds the read link and tries to

unreflect again. We don't want unnecessary module link read in such
cases. Check has been added whether the package is exported from
declaring module. Note that there is no security issue here.  Even
before the fix, module read edge does not give any capability. unreflect
call after adding unnecessary read line would still fail for
non-exported package case. There were unnecessary module read links
created - which is being avoided now.

* Additional "API" in Lookup.java slipped from jigsaw code into
jdk9-dev. Those unreflectCallerSensitive and
unreflectConstructorCallerSensitive were meant to be 'interim' somehow
slipped.  Now, only unreflect and unreflectConstructor in Lookup. Caller
sensitiveness is hidden in the implementation.  These methods were never
APIs when dynalink API was created.

Thanks,

-Sundar

Re: RFR 8157310: jdk.dynalink.linker.support.Lookup should have more checks before adding module read link

[Sundararajan Athijegannathan]

Hi,

Thanks for the review. But, I adjusted the change slightly -- moved
addModuleRead and caller-sensitive fallback attempt to
CallerSensitiveDynamicMethod itself - leaving Lookup as general
exception translating version of j.l.invoke.MethodHandle.Lookup.

I think this is better caller sensitivity handling moved to specific
place and Lookup remains a generic API.

http://cr.openjdk.java.net/~sundar/8157310/webrev.01/

Thanks,

-Sundar


On 5/20/2016 12:22 PM, Attila Szegedi wrote:
> +1. Nice work!
>
>> On 19 May 2016, at 15:28, Sundararajan Athijegannathan 
>>  wrote:
>>
>> Please review http://cr.openjdk.java.net/~sundar/8157310/ for
>> https://bugs.openjdk.java.net/browse/JDK-8157310
>>
>> What is this fix?
>>
>> * When unreflecting caller sensitive methods, dynalink uses specific
>> Lookup of actual caller (instead of publicLookup) - in nashorn's case
>> it's be the Lookup of script class.
>>
>> * Script class may not have read link to the module of the class (of
>> caller sensitive member). If there is any IllegalAccessError, dynalink
>> adds the read link and tries to
>>
>> unreflect again. We don't want unnecessary module link read in such
>> cases. Check has been added whether the package is exported from
>> declaring module. Note that there is no security issue here.  Even
>> before the fix, module read edge does not give any capability. unreflect
>> call after adding unnecessary read line would still fail for
>> non-exported package case. There were unnecessary module read links
>> created - which is being avoided now.
>>
>> * Additional "API" in Lookup.java slipped from jigsaw code into
>> jdk9-dev. Those unreflectCallerSensitive and
>> unreflectConstructorCallerSensitive were meant to be 'interim' somehow
>> slipped.  Now, only unreflect and unreflectConstructor in Lookup. Caller
>> sensitiveness is hidden in the implementation.  These methods were never
>> APIs when dynalink API was created.
>>
>> Thanks,
>>
>> -Sundar
>>
>>