Re: RFR(JDK12/JAXP/java.xml) 8204329: Java API doc for XMLStreamReader.next() needs to be clarified for the exception thrown when hasNext() method returns false

[Joe Wang]

Thanks Roger, Lance!  Pushed.

Best,
Joe

On 7/2/18, 12:21 PM, Roger Riggs wrote:

+1

On 7/2/2018 3:07 PM, Lance Andersen wrote:

+1


On Jul 2, 2018, at 1:44 PM, Joe Wang  wrote:

Hi,

Please review a patch that clarifies that XMLStreamReader.next() 
throws NoSuchElementException when hasNext() is false by removing 
the confusing note.


JBS: https://bugs.openjdk.java.net/browse/JDK-8204329
CSR: https://bugs.openjdk.java.net/browse/JDK-8206084
webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8204329/webrev/

Thanks,
Joe


 

Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR(JDK12/JAXP/java.xml) 8204329: Java API doc for XMLStreamReader.next() needs to be clarified for the exception thrown when hasNext() method returns false

[Roger Riggs]

+1

On 7/2/2018 3:07 PM, Lance Andersen wrote:

+1


On Jul 2, 2018, at 1:44 PM, Joe Wang  wrote:

Hi,

Please review a patch that clarifies that XMLStreamReader.next() throws 
NoSuchElementException when hasNext() is false by removing the confusing note.

JBS: https://bugs.openjdk.java.net/browse/JDK-8204329
CSR: https://bugs.openjdk.java.net/browse/JDK-8206084
webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8204329/webrev/

Thanks,
Joe

  
   

  Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR(JDK12/JAXP/java.xml) 8204329: Java API doc for XMLStreamReader.next() needs to be clarified for the exception thrown when hasNext() method returns false

[Lance Andersen]

+1

> On Jul 2, 2018, at 1:44 PM, Joe Wang  wrote:
> 
> Hi,
> 
> Please review a patch that clarifies that XMLStreamReader.next() throws 
> NoSuchElementException when hasNext() is false by removing the confusing note.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8204329
> CSR: https://bugs.openjdk.java.net/browse/JDK-8206084
> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8204329/webrev/
> 
> Thanks,
> Joe

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

[12] RFR: 8206120: Add test cases for lenient Japanese era parsing

[Naoto Sato]

Hello,

Please review the changes to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8206120

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8206120/webrev.00/

This is a test case only fix to ensure the new Japanese era 
implementation introduced with JDK-8202088 accepts lenient era/year, 
such as "Heisei 32."


Naoto

RFR(JDK12/JAXP/java.xml) 8204329: Java API doc for XMLStreamReader.next() needs to be clarified for the exception thrown when hasNext() method returns false

[Joe Wang]

Hi,

Please review a patch that clarifies that XMLStreamReader.next() throws 
NoSuchElementException when hasNext() is false by removing the confusing 
note.


JBS: https://bugs.openjdk.java.net/browse/JDK-8204329
CSR: https://bugs.openjdk.java.net/browse/JDK-8206084
webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8204329/webrev/

Thanks,
Joe

Re: [11] Review Request: 8205537 Drop of sun.applet package

[Sergey Bylokhov]

I can argue that this is not p2 as related JDK-8201446, but it is 
definitely not p4.


On 02/07/2018 09:30, Phil Race wrote:

It is not necessary and I reject that it is a P3. Let's leave it for 12.

-phil.

On 07/02/2018 09:24 AM, Sergey Bylokhov wrote:
I think it is a good thing to drop this api completely in the same 
release as JDK-8201446. This is not a p4 and so it can be be fixed and 
integrated in jdk11 if the fix is ready.


On 02/07/2018 09:10, Phil Race wrote:

Sergey,

I think at this point, this is not a P3 for 11.
So I won't approve it for 11, but will consider it for 12.

-phil.

On 7/2/2018 6:36 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8205537
Webrev: http://cr.openjdk.java.net/~serb/8205537/webrev.00/

sun.applet is an internal package contained some code related to 
implementation of applets and appletviewer. Some of its classes were 
dropped already: JDK-8204454, JDK-8203308. Now there are only 3 
classes, all related to the applet security and we can drop them as 
well.


In the fix this package was removed, and the tests were updated to 
not use it. I am not sure how "ClassnameCharTest.java" is useful 
after applets removal, but since this test used subclass of the 
AppletClassLoader, I have copied some code from the removed class to 
the test.













--
Best regards, Sergey.

Re: [11] Review Request: 8205537 Drop of sun.applet package

[Phil Race]

It is not necessary and I reject that it is a P3. Let's leave it for 12.

-phil.

On 07/02/2018 09:24 AM, Sergey Bylokhov wrote:
I think it is a good thing to drop this api completely in the same 
release as JDK-8201446. This is not a p4 and so it can be be fixed and 
integrated in jdk11 if the fix is ready.


On 02/07/2018 09:10, Phil Race wrote:

Sergey,

I think at this point, this is not a P3 for 11.
So I won't approve it for 11, but will consider it for 12.

-phil.

On 7/2/2018 6:36 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8205537
Webrev: http://cr.openjdk.java.net/~serb/8205537/webrev.00/

sun.applet is an internal package contained some code related to 
implementation of applets and appletviewer. Some of its classes were 
dropped already: JDK-8204454, JDK-8203308. Now there are only 3 
classes, all related to the applet security and we can drop them as 
well.


In the fix this package was removed, and the tests were updated to 
not use it. I am not sure how "ClassnameCharTest.java" is useful 
after applets removal, but since this test used subclass of the 
AppletClassLoader, I have copied some code from the removed class to 
the test.

Re: [11] Review Request: 8205537 Drop of sun.applet package

[Sergey Bylokhov]

I think it is a good thing to drop this api completely in the same 
release as JDK-8201446. This is not a p4 and so it can be be fixed and 
integrated in jdk11 if the fix is ready.


On 02/07/2018 09:10, Phil Race wrote:

Sergey,

I think at this point, this is not a P3 for 11.
So I won't approve it for 11, but will consider it for 12.

-phil.

On 7/2/2018 6:36 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8205537
Webrev: http://cr.openjdk.java.net/~serb/8205537/webrev.00/

sun.applet is an internal package contained some code related to 
implementation of applets and appletviewer. Some of its classes were 
dropped already: JDK-8204454, JDK-8203308. Now there are only 3 
classes, all related to the applet security and we can drop them as well.


In the fix this package was removed, and the tests were updated to not 
use it. I am not sure how "ClassnameCharTest.java" is useful after 
applets removal, but since this test used subclass of the 
AppletClassLoader, I have copied some code from the removed class to 
the test.








--
Best regards, Sergey.

Re: 8143850: retrofit ArrayDeque to implement List

[Alex Foster]

Another question is whether or not it should allow null elements. ArrayDeque 
currently doesn't but I think it would be useful to support them, which makes 
having an asList() view or sharing code harder.

Re: [11] Review Request: 8205537 Drop of sun.applet package

[Phil Race]

Sergey,

I think at this point, this is not a P3 for 11.
So I won't approve it for 11, but will consider it for 12.

-phil.

On 7/2/2018 6:36 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8205537
Webrev: http://cr.openjdk.java.net/~serb/8205537/webrev.00/

sun.applet is an internal package contained some code related to 
implementation of applets and appletviewer. Some of its classes were 
dropped already: JDK-8204454, JDK-8203308. Now there are only 3 
classes, all related to the applet security and we can drop them as well.


In the fix this package was removed, and the tests were updated to not 
use it. I am not sure how "ClassnameCharTest.java" is useful after 
applets removal, but since this test used subclass of the 
AppletClassLoader, I have copied some code from the removed class to 
the test.

[11] Review Request: 8205537 Drop of sun.applet package

[Sergey Bylokhov]

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8205537
Webrev: http://cr.openjdk.java.net/~serb/8205537/webrev.00/

sun.applet is an internal package contained some code related to 
implementation of applets and appletviewer. Some of its classes were 
dropped already: JDK-8204454, JDK-8203308. Now there are only 3 classes, 
all related to the applet security and we can drop them as well.


In the fix this package was removed, and the tests were updated to not 
use it. I am not sure how "ClassnameCharTest.java" is useful after 
applets removal, but since this test used subclass of the 
AppletClassLoader, I have copied some code from the removed class to the 
test.



--
Best regards, Sergey.

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

> On 29 Jun 2018, at 17:34, Seán Coffey  wrote:
> 
> I've introduced a new test helper class in the jdk/test/lib/jfr directory to 
> help with the dual test nature of the new tests. It's helped alot with test 
> code duplication.
> 

My thinking was to put things like the certificates in a separate file, i.e 
TestCertificates, and then have a logging test and a JFR test reuse it.

One rationale for adding logging was to use it if JFR is not present. By 
putting the tests together, it becomes impossible to compile and test logging 
without having JFR.

> Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits. 
> The output is displayed in units like "KiB" - not the normal when examining 
> key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2 
> KiB" - I'd prefer to keep the 2048 display version.

We should not let the JMC GUI decide how units are specified. There will be 
other GUIs and this is the first event that uses bits, so I don’t think it is 
formatted that way because it was considered superior.

Erik

> 
> new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/
> 
> Regards,
> Sean.
> 
> On 28/06/18 17:59, Seán Coffey wrote:
>> Comments inline.
>> 
>> 
>> On 28/06/2018 17:20, Erik Gahlin wrote:
>>> It's sufficient if an event object escapes to another method (regardless if 
>>> JFR is enabled or not).
>>> 
>>> Some more feedback:
>>> 
>>> Rename event jdk.CertChain to jdk.CertificateChain
>>> Rename event jdk.X509Cert to jdk.X509Certificate
>>> Rename field certChain to certificateChain.
>>> Rename field serialNum to serialNumber
>> all above done.
>>> Rename field algId to AlgorithmicId or AlgorithmicID
>> maybe "algorithm" works here also ?
>>> Rename @Label("Ciphersuite") to @Label("Cipher Suite")
>>> Rename @Label("Cert Chain") to @Label("Certificate Chain")
>>> Rename @Label("Property Name") to "Name" or "Key" if that makes sense in 
>>> the context?
>>> Rename @Label("Property Value") to "Value".
>>> Put events in a subcategory, i.e  @Category({"Java Development Kit", 
>>> "Security"})
>> done.
>>> Make classes final.
>> done. I had thought that the JFR mechanism required non-final classes.
>>> What is the unit of the key in X509Certificate event? Bits? If that is the 
>>> case, use @DataAmount(DataAmount.BITS)
>> Yes - it's essentially the bit length of the keys used. Let me look into 
>> that annotation usage.
>>> @Label("Serial numbers forming chain of trust") should not be a sentence. 
>>> How about "Serial Numbers"?
>>> 
>>> I think the tests are hard to read when two things are tested at the same 
>>> time. It is also likely that if a test fail due to logging issues, it will 
>>> be assigned to JFR because of the test name, even thought the issues is not 
>>> JFR related.
>> I think we're always going to have some ownership issues when tests serve a 
>> dual purpose. I still think it makes sense to keep the test logic in one 
>> place. Any failures in these tests will most likely be owned by security 
>> team. (moving the tests to security directory is also an option)
>>> 
>>> If you want to reuse code between tests, I would put it in testlibrary.
>> Let me check if there's any common patterns that could be added to the 
>> testlibary.
>> 
>> Thanks,
>> Sean.
>>> 
>>> Erik
>>> 
 Thanks for the update Erik. By default I'm proposing that the new JFR 
 Events and Logger be disabled. As a result the event class shouldn't 
 escape. If performance metrics highlight an issue, we should revisit.
 
 regards,
 Sean.
 
 
 On 27/06/2018 20:57, Erik Gahlin wrote:
> On 2018-06-27 21:14, Seán Coffey wrote:
>> 
>> 
>> On 27/06/2018 19:57, Xuelei Fan wrote:
>>> Hi Sean,
>>> 
>>> I may reply in several replies.
>>> 
>>> PKIXMasterCertPathValidator.java
>>> 
>>> +  CertChainEvent cce = new CertChainEvent();
>>> +  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
>>> +  String c = reversedCertList.stream()
>>> +  .map(x -> x.getSerialNumber().toString(16))
>>> +.collect(Collectors.joining(", "));
>>> + EventHelper.commitCertChainEvent(cce, c);
>>> +   }
>>> 
>>> No matter the event or the JFR mechanism is enabled or not, the above 
>>> code will create a new instance.  Is the return value of 
>>> cce.isEnabled() dynamically changed or static?
>> This is a requirement from the JFR framework. All their event.isEnabled 
>> calls are instance methods and follow a similar pattern. The JFR team 
>> assure me that the JIT can optimize away such calls.
> 
> The JIT will most likely not be able to optimize if the event class 
> escapes.
> 
> From a JFR perspective, this would be the preferred layout:
> 
> EventA eventA= new EventA();
> eventA.value = this.value;
> eventA.commit();
> 
> and t