[10] RFR: 8189272 & 8189291
Hello, Please review the changes for the following two issues: 8189272: CLDR and JRE LocaleProviderAdapters silently swallow exceptions [1] 8189291: Test policy should extend the default system policy [2] The proposed fixes for the above issues are located below, respectively: http://cr.openjdk.java.net/~naoto/8189272/webrev.00/ http://cr.openjdk.java.net/~naoto/8189291/webrev.00/ The fix to 8189272 will throw exceptions that were swallowed in those providers, which should not happen in proper environment. That fix makes test cases with security manager fail. The fix to 8189291 addresses it. Naoto [1] https://bugs.openjdk.java.net/browse/JDK-8189272 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
On 10/20/17 2:20 PM, Naoto Sato wrote:
Hello,
Please review the changes for the following two issues:
8189272: CLDR and JRE LocaleProviderAdapters silently swallow exceptions [1] 8189291: Test policy should extend the default system policy [2]
The proposed fixes for the above issues are located below, respectively:
Looks fine. The installed providers are our implementation and silently ignore any exception would make it really hard to diagnose problems. This change will help the diagnosability.
If the default policy is not cached early before any test policy is constructed, the test might get the wrong Policy object set by the test previously (for example, if it runs in agentvm mode). It would be more reliable if this is cached as the static field in the test class (i.e. the enclosing class of the policy subclass) - like CallerSensitiveTest. test/jdk/sun/util/locale/provider/Bug8038436.java - I think the test fix should be part of JDK-8189272, right? Mandy
The fix to 8189272 will throw exceptions that were swallowed in those providers, which should not happen in proper environment. That fix makes test cases with security manager fail. The fix to 8189291 addresses it.
Naoto
[1] https://bugs.openjdk.java.net/browse/JDK-8189272 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Hi Mandy, Thanks for the review. Webrevs updated as suggested: http://cr.openjdk.java.net/~naoto/8189272/webrev.01/ http://cr.openjdk.java.net/~naoto/8189291/webrev.01/ Naoto On 10/20/17 2:45 PM, mandy chung wrote:
On 10/20/17 2:20 PM, Naoto Sato wrote:
Hello,
Please review the changes for the following two issues:
8189272: CLDR and JRE LocaleProviderAdapters silently swallow exceptions [1] 8189291: Test policy should extend the default system policy [2]
The proposed fixes for the above issues are located below, respectively:
Looks fine. The installed providers are our implementation and silently ignore any exception would make it really hard to diagnose problems. This change will help the diagnosability.
If the default policy is not cached early before any test policy is constructed, the test might get the wrong Policy object set by the test previously (for example, if it runs in agentvm mode). It would be more reliable if this is cached as the static field in the test class (i.e. the enclosing class of the policy subclass) - like CallerSensitiveTest.
test/jdk/sun/util/locale/provider/Bug8038436.java - I think the test fix should be part of JDK-8189272, right?
Mandy
The fix to 8189272 will throw exceptions that were swallowed in those providers, which should not happen in proper environment. That fix makes test cases with security manager fail. The fix to 8189291 addresses it.
Naoto
[1] https://bugs.openjdk.java.net/browse/JDK-8189272 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
On 10/20/17 5:05 PM, Naoto Sato wrote:
Hi Mandy,
Thanks for the review. Webrevs updated as suggested:
http://cr.openjdk.java.net/~naoto/8189272/webrev.01/ http://cr.openjdk.java.net/~naoto/8189291/webrev.01/
DEFAULT_POLICY in test/jdk/java/util/logging/* tests can be moved to the enclosing class as suggested. Other than that, the change looks good. Mandy
Naoto
On 10/20/17 2:45 PM, mandy chung wrote:
On 10/20/17 2:20 PM, Naoto Sato wrote:
Hello,
Please review the changes for the following two issues:
8189272: CLDR and JRE LocaleProviderAdapters silently swallow exceptions [1] 8189291: Test policy should extend the default system policy [2]
The proposed fixes for the above issues are located below, respectively:
Looks fine. The installed providers are our implementation and silently ignore any exception would make it really hard to diagnose problems. This change will help the diagnosability.
If the default policy is not cached early before any test policy is constructed, the test might get the wrong Policy object set by the test previously (for example, if it runs in agentvm mode). It would be more reliable if this is cached as the static field in the test class (i.e. the enclosing class of the policy subclass) - like CallerSensitiveTest.
test/jdk/sun/util/locale/provider/Bug8038436.java - I think the test fix should be part of JDK-8189272, right?
Mandy
The fix to 8189272 will throw exceptions that were swallowed in those providers, which should not happen in proper environment. That fix makes test cases with security manager fail. The fix to 8189291 addresses it.
Naoto
[1] https://bugs.openjdk.java.net/browse/JDK-8189272 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
Hi Mandy, Sorry, I seem to have generated/uploaded an incorrect webrev for 8189291. Here is the correct one: http://cr.openjdk.java.net/~naoto/8189291/webrev.02/ Naoto On 10/23/17 5:48 PM, mandy chung wrote:
On 10/20/17 5:05 PM, Naoto Sato wrote:
Hi Mandy,
Thanks for the review. Webrevs updated as suggested:
http://cr.openjdk.java.net/~naoto/8189272/webrev.01/ http://cr.openjdk.java.net/~naoto/8189291/webrev.01/
DEFAULT_POLICY in test/jdk/java/util/logging/* tests can be moved to the enclosing class as suggested.
Other than that, the change looks good.
Mandy
Naoto
On 10/20/17 2:45 PM, mandy chung wrote:
On 10/20/17 2:20 PM, Naoto Sato wrote:
Hello,
Please review the changes for the following two issues:
8189272: CLDR and JRE LocaleProviderAdapters silently swallow exceptions [1] 8189291: Test policy should extend the default system policy [2]
The proposed fixes for the above issues are located below, respectively:
Looks fine. The installed providers are our implementation and silently ignore any exception would make it really hard to diagnose problems. This change will help the diagnosability.
If the default policy is not cached early before any test policy is constructed, the test might get the wrong Policy object set by the test previously (for example, if it runs in agentvm mode). It would be more reliable if this is cached as the static field in the test class (i.e. the enclosing class of the policy subclass) - like CallerSensitiveTest.
test/jdk/sun/util/locale/provider/Bug8038436.java - I think the test fix should be part of JDK-8189272, right?
Mandy
The fix to 8189272 will throw exceptions that were swallowed in those providers, which should not happen in proper environment. That fix makes test cases with security manager fail. The fix to 8189291 addresses it.
Naoto
[1] https://bugs.openjdk.java.net/browse/JDK-8189272 [2] https://bugs.openjdk.java.net/browse/JDK-8189291
On 10/24/17 8:00 AM, Naoto Sato wrote:
Hi Mandy,
Sorry, I seem to have generated/uploaded an incorrect webrev for 8189291. Here is the correct one:
+1 Mandy
participants (2)
-
mandy chung
-
Naoto Sato