Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_<pid> in launcher code #5724

Closed
wants to merge 6 commits into from

Conversation

prrace
Copy link
Contributor

@prrace prrace commented Sep 27, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724
$ git checkout pull/5724

Update a local copy of the PR:
$ git checkout pull/5724
$ git pull https://git.openjdk.java.net/jdk pull/5724/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5724

View PR using the GUI difftool:
$ git pr show -t 5724

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5724.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 27, 2021

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 27, 2021

@prrace The following labels will be automatically applied to this pull request:

  • client
  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added client core-libs labels Sep 27, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 27, 2021

Webrevs

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

LGTM

@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2021

@prrace This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_<pid> in launcher code

Reviewed-by: rriggs, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 94 new commits pushed to the master branch:

  • 8f7a37c: 8274434: move os::get_default_process_handle and os::dll_lookup to os_posix for POSIX platforms
  • 3953e07: 8271459: C2: Missing NegativeArraySizeException when creating StringBuilder with negative capacity
  • 53d7e95: 8274635: Use String.equals instead of String.compareTo in jdk.accessibility
  • e43f540: 8274651: Possible race in FontDesignMetrics.KeyReference.dispose
  • 2e542e3: 8274349: ForkJoinPool.commonPool() does not work with 1 CPU
  • 7e757f6: 8274559: JFR: Typo in 'jfr help configure' text
  • 75d6688: 8274745: ProblemList TestSnippetTag.java
  • 9914e5c: 8274610: Add linux-aarch64 to bootcycle build profiles
  • 0ca094b: 8273244: Improve diagnostic output related to ErroneousTree
  • 6f727d8: 8274666: rename HtmlStyle.descfrmTypeLabel to be less cryptic
  • ... and 84 more: https://git.openjdk.java.net/jdk/compare/252aaa9249d8979366b37d59487b5b039d923e35...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 28, 2021
*
* NOTE: It is used by SWT, and JavaFX.
* Not idea if how much external code ever sets it, but use it if set, else
Copy link
Member

@dougxc dougxc Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Not idea if" -> "No idea of"


(*env)->ReleaseStringUTFChars(env, mainClassString, mainClassName);
(*env)->DeleteLocalRef(env, jKey);
Copy link
Member

@mrserb mrserb Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@prrace prrace Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@prrace
Copy link
Contributor Author

@prrace prrace commented Sep 29, 2021

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

@prrace
Copy link
Contributor Author

@prrace prrace commented Sep 29, 2021

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.

@prrace
Copy link
Contributor Author

@prrace prrace commented Oct 1, 2021

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.

mrserb
mrserb approved these changes Oct 4, 2021
@prrace
Copy link
Contributor Author

@prrace prrace commented Oct 5, 2021

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

mrserb
mrserb approved these changes Oct 5, 2021
Copy link
Member

@mrserb mrserb left a comment

Looks fine

@prrace
Copy link
Contributor Author

@prrace prrace commented Oct 5, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2021

Going to push as commit 3789065.
Since your change was applied there have been 104 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 5, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2021

@prrace Pushed as commit 3789065.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client core-libs integrated
4 participants