Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]
On Wed, 17 Mar 2021 00:57:24 GMT, Henry Jen wrote: >> This patch ensure launcher won't crash JVM for the new static Methods from >> local/anonymous class on MacOS. >> >> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug >> when the launcher trying to grab class name to be displayed as the >> Application name on the menu. >> >> The fix is to not setting name, test shows that GUI java application shows >> 'bin' as the application name. It's possible for us to set the name to >> something more friendly, for example, "Java", but I am not sure that should >> be launcher's responsibility to choose such a default name. It seems to me >> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be >> responsible to pick such name in case the environment variable is not set. > > Henry Jen has updated the pull request incrementally with one additional > commit since the last revision: > > Add copyright and another test case Looks fine from the client point of view. - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]
On Wed, 17 Mar 2021 08:55:54 GMT, Alan Bateman wrote: > > > Using an anonymous class for the main class looks strange and hard to > > > believe anyone is relying on this. I wonder if we should do more checking > > > LauncherHelper.validateMainClass to reject cases like this. > > > > > > I raised that same question, and people tends to agree launcher could > > reject anonymous/local classes. But pointed out that should require a CSR > > review. Therefore I chose to fix crash first, and we can file another > > ticket to address main class requirements. > > The requirement for a CSR and release note should not be a concern here. I > don't object to the fix but I think it would be very useful to document the > main class and whether local or anonymous classes can be used, its > accessibility, and the accessibility of the main method. We had to do > something similar recently with the premain method (java.lang.instrument). Absolutely we need to clarify that, however, the discussion of what should or should not be allowed may take some time. For example, if we limit such usage in launcher, should it be possible for custom launcher to start such a main method? Thus the recommendation to have a separate ticket for that. The current java document mostly only say to load the specify the class name, however there is a sentence `By default, the first argument that isn't an option of the java command is the fully qualified name of the class to be called. If -jar is specified, then its argument is the name of the JAR file containing class and resource files for the application. The startup class must be indicated by the Main-Class manifest header in its manifest file.`. Not that it says `fully qualified name of the class`. Filed [JDK-8263735](https://bugs.openjdk.java.net/browse/JDK-8263735) for the follow up, in the mean time, we should get this crash resolved. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]
On Tue, 16 Mar 2021 17:49:42 GMT, Henry Jen wrote: > > Using an anonymous class for the main class looks strange and hard to > > believe anyone is relying on this. I wonder if we should do more checking > > LauncherHelper.validateMainClass to reject cases like this. > > I raised that same question, and people tends to agree launcher could reject > anonymous/local classes. But pointed out that should require a CSR review. > Therefore I chose to fix crash first, and we can file another ticket to > address main class requirements. The requirement for a CSR and release note should not be a concern here. I don't object to the fix but I think it would be very useful to document the main class and whether local or anonymous classes can be used, its accessibility, and the accessibility of the main method. We had to do something similar recently with the premain method (java.lang.instrument). - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On 16/03/2021 2:59 pm, David Holmes wrote: On 16/03/2021 11:58 am, Sergey Bylokhov wrote: On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS. As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu. The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set. This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine. Both issues involve a problem trying to use the canonical name, but I'd consider both fixes deficient when an alternative name could be used. Except I overlooked that this is an anonymous class so no simple name either. I agree with Henry's later proposal - fix the crash simply then outlaw the usecase later. Cheers, David But this isn't my code so ... David - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]
> This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. Henry Jen has updated the pull request incrementally with one additional commit since the last revision: Add copyright and another test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/2999/files - new: https://git.openjdk.java.net/jdk/pull/2999/files/58f197f4..f68b0919 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2999&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2999&range=01-02 Stats: 30 lines in 2 files changed: 29 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2999.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999 PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v2]
On Tue, 16 Mar 2021 17:39:34 GMT, Henry Jen wrote: >> test/jdk/tools/launcher/8261785/CrashTheJVM.java line 1: >> >>> 1: import java.io.IOException; >> >> Copyright? > > This file is mostly based on the bug report as I just adjust static keyword > to make sure we cover different cases, thus I am not sure whether to add > copyright or not. you need to clarify this, if we cannot add copyright here, we should not use this file. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v2]
> This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. Henry Jen has updated the pull request incrementally with one additional commit since the last revision: Adjust width used for Copyright - Changes: - all: https://git.openjdk.java.net/jdk/pull/2999/files - new: https://git.openjdk.java.net/jdk/pull/2999/files/0fdea41c..58f197f4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2999&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2999&range=00-01 Stats: 15 lines in 1 file changed: 3 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/2999.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999 PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Tue, 16 Mar 2021 15:33:37 GMT, Alan Bateman wrote: > Using an anonymous class for the main class looks strange and hard to believe > anyone is relying on this. I wonder if we should do more checking > LauncherHelper.validateMainClass to reject cases like this. I raised that same question, and people tends to agree launcher could reject anonymous/local classes. But pointed out that should require a CSR review. Therefore I chose to fix crash first, and we can file another ticket to address main class requirements. > This is not the last attempt to set the name, the JAVA_MAIN_CLASS_ variable > is used in the middle of the name selection, there are some others. And the > "bin" is selected by some of the next step, I agree it is not a friendly name > that could be improved. I tried to do a quick search on JAVA_MAIN_CLASS_%pid variable, didn't find other code to set this. I had a version that would set the variable to "Java", I can extend that to cover exception case as well. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Tue, 16 Mar 2021 01:59:33 GMT, Sergey Bylokhov wrote: >> This patch ensure launcher won't crash JVM for the new static Methods from >> local/anonymous class on MacOS. >> >> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug >> when the launcher trying to grab class name to be displayed as the >> Application name on the menu. >> >> The fix is to not setting name, test shows that GUI java application shows >> 'bin' as the application name. It's possible for us to set the name to >> something more friendly, for example, "Java", but I am not sure that should >> be launcher's responsibility to choose such a default name. It seems to me >> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be >> responsible to pick such name in case the environment variable is not set. > > test/jdk/tools/launcher/8261785/CrashTheJVM.java line 1: > >> 1: import java.io.IOException; > > Copyright? This file is mostly based on the bug report as I just adjust static keyword to make sure we cover different cases, thus I am not sure whether to add copyright or not. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Tue, 16 Mar 2021 07:43:54 GMT, Sergey Bylokhov wrote: >> This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and >> the fix looks fine. > >> Maybe the AWT folk should decide what name should be displayed in this >> case. The canonical name was chosen when all main classes had to have a >> canonical name. So perhaps a simple name will suffice in the case where >> there is no canonical name? > > This is not the last attempt to set the name, the JAVA_MAIN_CLASS_ variable > is used in the middle of the name selection, there are some others. And the > "bin" is selected by some of the next step, I agree it is not a friendly name > that could be improved. Using an anonymous class for the main class looks strange and hard to believe anyone is relying on this. I wonder if we should do more checking LauncherHelper.validateMainClass to reject cases like this. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Tue, 16 Mar 2021 01:55:49 GMT, Sergey Bylokhov wrote: >> This patch ensure launcher won't crash JVM for the new static Methods from >> local/anonymous class on MacOS. >> >> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug >> when the launcher trying to grab class name to be displayed as the >> Application name on the menu. >> >> The fix is to not setting name, test shows that GUI java application shows >> 'bin' as the application name. It's possible for us to set the name to >> something more friendly, for example, "Java", but I am not sure that should >> be launcher's responsibility to choose such a default name. It seems to me >> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be >> responsible to pick such name in case the environment variable is not set. > > This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and > the fix looks fine. > Maybe the AWT folk should decide what name should be displayed in this > case. The canonical name was chosen when all main classes had to have a > canonical name. So perhaps a simple name will suffice in the case where > there is no canonical name? This is not the last attempt to set the name, the JAVA_MAIN_CLASS_ variable is used in the middle of the name selection, there are some others. And the "bin" is selected by some of the next step, I agree it is not a friendly name that could be improved. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On 16/03/2021 11:58 am, Sergey Bylokhov wrote: On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS. As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu. The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set. This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine. Both issues involve a problem trying to use the canonical name, but I'd consider both fixes deficient when an alternative name could be used. But this isn't my code so ... David - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: > This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. test/jdk/tools/launcher/8261785/CrashTheJVM.java line 1: > 1: import java.io.IOException; Copyright? test/jdk/tools/launcher/8261785/Test8261785.java line 5: > 3: * COPYRIGHT NOTICES OR THIS FILE HEADER. > 4: * > 5: * This code is free software; you can redistribute it and/or modify it > under the terms of the GNU Looks like formatting much wider than usual - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: > This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine. - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
Hi Henry, On 15/03/2021 9:40 am, Henry Jen wrote: This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS. As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu. The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set. Maybe the AWT folk should decide what name should be displayed in this case. The canonical name was chosen when all main classes had to have a canonical name. So perhaps a simple name will suffice in the case where there is no canonical name? Cheers, David - Commit messages: - 8261785: Calling "main" method in anonymous nested class crashes the JVM Changes: https://git.openjdk.java.net/jdk/pull/2999/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2999&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261785 Stats: 111 lines in 3 files changed: 110 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2999.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999 PR: https://git.openjdk.java.net/jdk/pull/2999
RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS. As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu. The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set. - Commit messages: - 8261785: Calling "main" method in anonymous nested class crashes the JVM Changes: https://git.openjdk.java.net/jdk/pull/2999/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2999&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261785 Stats: 111 lines in 3 files changed: 110 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2999.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999 PR: https://git.openjdk.java.net/jdk/pull/2999
