Hi Alan, Thank you for the review. My comments are inline and Webrev is updated here: http://cr.openjdk.java.net/~rpatil/8167063/webrev.01/
Change Summary: - Removed SecurityException handling - Updated the error message in launcher.properties - Removed loadModuleMainClass0 method and moved the code back into original loadModuleMainClass - NoClassDefFoundError is replaced by its parent class LinkageError in method loadMainClass Regards, Ramanand. -----Original Message----- From: Alan Bateman Sent: Friday, January 20, 2017 8:03 PM To: Ramanand Patil <ramanand.pa...@oracle.com>; core-libs-dev@openjdk.java.net Subject: Re: RFR: 8167063: spurious message "A JNI error has occurred" if start-class cannot be initialized On 20/01/2017 13:21, Ramanand Patil wrote: > Hi all, > Please review the following bug fix: > Bug: https://bugs.openjdk.java.net/browse/JDK-8167063 > Webrev: http://cr.openjdk.java.net/~rpatil/8167063/webrev.00/ > > Handled the SecurityException and LinkageError which can be thrown from > Class.forName(...) method used in LauncherHelper.java and added corresponding > error messages to launcher.properties. > Though the reported bug is because of the LinkageError, Security exception is > also handled to be safe from calling Class.forName method. > I see this changes loadMainClass to abort with resources that are keyed on java.launcher.cls.error6 and java.launcher.cls.error7 but they don't appear in the launcher.properties file. Does this work? I would assume MissingResourceException is thrown. Thanks for pointing this. I missed to add it to launcher.properties. But now I have changed error6 to point to java.launcher.cls.error1 and removed security exception handling. For java.launcher.module.error3 and java.launcher.module.error4 then "link" is likely to confuse people. Sure, there may be linkage errors but there are many linkage errors and I think would be a lot clearer if the message was "Unable to load main class {0} from module {1}\n\{2}". Updated the message as suggested. It's not clear to me that having a different message for security exceptions make sense, esp. when the exception is printed. So I think I would drop that. Yes, I agree to drop out the Security exceptions handling. Also loadModuleMainClass0 is unusual method name, we've mostly (not always) used the 0 suffix on native methods. In any case, it doesn't look like it is needed, the code was okay in loadModuleMainClass as it was. The method was added just to look the exception handling easier. I have removed the extra method and placed the code back into loadModuleMainClass. One final point is that is the nesting catching of LinkageError and SecurityException in loadMainClass, I assume you don't need the inner catch. I think both the catch blocks were needed, because the first(inner) catch was already inside catch.(This was same as NoClassDefFoundError catching twice in the same method). Now since NoClassDefFoundError is subclass of LinkageError, I have replaced it with LinkageError. And I think still the same abort message holds good. Please let me know if this is ok. -Alan
