Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v5]
On Tue, 5 Oct 2021 00:25:41 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code Looks fine - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v4]
On Fri, 1 Oct 2021 21:10:27 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code I just pushed a small update to the new test. The compiled class file of the child process was not found when I ran it in our test framework even though jtreg run locally found it. I just had to explicitly add the location of the compiled classes to the classpath. So it is just about getting the test to run in that env. rather than any problem with the fix, or the test logic. This update HAS passed in that framework - as well as locally - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v5]
> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set > the name of the application in the system menu bar. > > Because this set shortly after the VM is running, it causes a thread safety > issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 > > Since the AWT already looks for the system property > "apple.awt.application.name" for this same purpose, > we can set that instead of the env. var and avoid the threading issue. > > This trades the JAVA_MAIN_CLASS_ pollution of the environment for > setting a system property which is visible to apps as well but it seems a > reasonable trade-off since we already (implicitly) support this variable > anyway in preference to the env. var. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code - Changes: - all: https://git.openjdk.java.net/jdk/pull/5724/files - new: https://git.openjdk.java.net/jdk/pull/5724/files/4c8fb9af..ffd77dd9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=03-04 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5724.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724 PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v4]
On Fri, 1 Oct 2021 21:10:27 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v3]
On Wed, 29 Sep 2021 03:39:09 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code I've now pushed the new test to verify the system property. I've verified the test passes on my local machine but can't (until Monday) be sure it passes in the CI test framework because of power maintenance that has just started. - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v4]
> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set > the name of the application in the system menu bar. > > Because this set shortly after the VM is running, it causes a thread safety > issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 > > Since the AWT already looks for the system property > "apple.awt.application.name" for this same purpose, > we can set that instead of the env. var and avoid the threading issue. > > This trades the JAVA_MAIN_CLASS_ pollution of the environment for > setting a system property which is visible to apps as well but it seems a > reasonable trade-off since we already (implicitly) support this variable > anyway in preference to the env. var. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code - Changes: - all: https://git.openjdk.java.net/jdk/pull/5724/files - new: https://git.openjdk.java.net/jdk/pull/5724/files/96a5590c..4c8fb9af Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=02-03 Stats: 112 lines in 2 files changed: 112 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5724.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724 PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v3]
On Wed, 29 Sep 2021 03:39:09 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code And, oh, the env. var test code I needed to delete was checking not for a useful value but just that the env var was there. I wrote a simple jtreg test or the new code that set the system property and tested the expected value (default or set) but it seems jtreg makes MainWrapper the main class that is found regardless of main/othervm so I am currently grumbling quietly to myself about whether to add a test which is equivalent to the previous one which just tests the property has a value, or to (I suppose) write a more sophisticated test that has to exec another VM where it *should* be able to properly verify it. - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
On Mon, 27 Sep 2021 23:50:38 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher > code I've added code in the launcher part that strips the package name from what is seen. This was previously done in the AWT code for MAIN_CLASS_ since it was presumably the only code setting that but I didn't do it because before because I didn't want to interfere with what an app might have set as the system property .. but .. if the app didn't set it and we derived it, then I realised we probably should be consistent with what happened before and only the launcher code knows whether it was set by itself or the app - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v3]
> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set > the name of the application in the system menu bar. > > Because this set shortly after the VM is running, it causes a thread safety > issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 > > Since the AWT already looks for the system property > "apple.awt.application.name" for this same purpose, > we can set that instead of the env. var and avoid the threading issue. > > This trades the JAVA_MAIN_CLASS_ pollution of the environment for > setting a system property which is visible to apps as well but it seems a > reasonable trade-off since we already (implicitly) support this variable > anyway in preference to the env. var. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8274397: Stop setting env. var JAVA_MAIN_CLASS_ in launcher code - Changes: - all: https://git.openjdk.java.net/jdk/pull/5724/files - new: https://git.openjdk.java.net/jdk/pull/5724/files/93108c59..96a5590c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=01-02 Stats: 24 lines in 1 file changed: 23 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5724.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724 PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
On Wed, 29 Sep 2021 01:46:32 GMT, Sergey Bylokhov wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher >> code > > src/java.base/macosx/native/libjli/java_md_macosx.m line 879: > >> 877: } >> 878: >> 879: (*env)->DeleteLocalRef(env, jKey); > > I am not sure about error handling, the jkey is not removed when NULL_CHECK > is used. Also an exceptions are not cleared in case of NULL_CHECK like in > other cases, is it intentional? Well .. they aren't removed by the existing code either. And this is the launcher. So far as I can tell if we error out of here (as I found when I made a typo in a method signature) the VM exits very rapidly. So if I do anything here, it would be to remove DeleteLocalRef since it really doesn't matter to clean up local refs when you are either bailing from native .. or the entire process .. cleaning up when you are continuing on (as the code does) is perhaps more important since you might need more local refs before you are done. Or file a separate bug against the launcher and JNI clean up after error handling. java.base/share/native/libjli/java.c is a good example of where the same pattern exists. - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
On Mon, 27 Sep 2021 23:50:38 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher > code src/java.base/macosx/native/libjli/java_md_macosx.m line 879: > 877: } > 878: > 879: (*env)->DeleteLocalRef(env, jKey); I am not sure about error handling, the jkey is not removed when NULL_CHECK is used. Also an exceptions are not cleared in case of NULL_CHECK like in other cases, is it intentional? - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
On Mon, 27 Sep 2021 23:50:38 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher > code src/java.base/macosx/native/libjli/java_md_macosx.m line 842: > 840: * of the app as it appears in the system menu bar. > 841: * > 842: * Not idea if how much external code ever sets it, but use it if > set, else "Not idea if" -> "No idea of" - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
On Mon, 27 Sep 2021 23:50:38 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher > code LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set > the name of the application in the system menu bar. > > Because this set shortly after the VM is running, it causes a thread safety > issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 > > Since the AWT already looks for the system property > "apple.awt.application.name" for this same purpose, > we can set that instead of the env. var and avoid the threading issue. > > This trades the JAVA_MAIN_CLASS_ pollution of the environment for > setting a system property which is visible to apps as well but it seems a > reasonable trade-off since we already (implicitly) support this variable > anyway in preference to the env. var. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code - Changes: - all: https://git.openjdk.java.net/jdk/pull/5724/files - new: https://git.openjdk.java.net/jdk/pull/5724/files/232bfae4..93108c59 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=00-01 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5724.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724 PR: https://git.openjdk.java.net/jdk/pull/5724
RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code
macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set the name of the application in the system menu bar. Because this set shortly after the VM is running, it causes a thread safety issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 Since the AWT already looks for the system property "apple.awt.application.name" for this same purpose, we can set that instead of the env. var and avoid the threading issue. This trades the JAVA_MAIN_CLASS_ pollution of the environment for setting a system property which is visible to apps as well but it seems a reasonable trade-off since we already (implicitly) support this variable anyway in preference to the env. var. - Commit messages: - 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code - 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code Changes: https://git.openjdk.java.net/jdk/pull/5724/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5724=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274397 Stats: 75 lines in 2 files changed: 33 ins; 33 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5724.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724 PR: https://git.openjdk.java.net/jdk/pull/5724
