Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

Sergey Bylokhov-2
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

Sergey Bylokhov-2
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

David Holmes
In reply to this post by Sergey Bylokhov-2
On 16/03/2021 11:58 am, Sergey Bylokhov wrote:
> On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen <[hidden email]> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

Sergey Bylokhov-2
In reply to this post by Sergey Bylokhov-2
On Tue, 16 Mar 2021 01:55:49 GMT, Sergey Bylokhov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

Alan Bateman-2
On Tue, 16 Mar 2021 07:43:54 GMT, Sergey Bylokhov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

Henry Jen-2
In reply to this post by Sergey Bylokhov-2
On Tue, 16 Mar 2021 01:59:33 GMT, Sergey Bylokhov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

Henry Jen-2
In reply to this post by Alan Bateman-2
On Tue, 16 Mar 2021 15:33:37 GMT, Alan Bateman <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v2]

Sergey Bylokhov-2
In reply to this post by Henry Jen-2
On Tue, 16 Mar 2021 17:39:34 GMT, Henry Jen <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

David Holmes
In reply to this post by David Holmes
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 <[hidden email]> 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
>>
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]

Alan Bateman-2
In reply to this post by Henry Jen-2
On Tue, 16 Mar 2021 17:49:42 GMT, Henry Jen <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]

Henry Jen-2
On Wed, 17 Mar 2021 08:55:54 GMT, Alan Bateman <[hidden email]> 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