Skip to content

VSNetBeans: Disable SecurityManager [compat:jdk19-24]. Avoid repeated starts on failure.#8159

Merged
sdedic merged 1 commit intoapache:masterfrom
sdedic:lsp/no-secman-restart
Jan 17, 2025
Merged

VSNetBeans: Disable SecurityManager [compat:jdk19-24]. Avoid repeated starts on failure.#8159
sdedic merged 1 commit intoapache:masterfrom
sdedic:lsp/no-secman-restart

Conversation

@sdedic
Copy link
Copy Markdown
Member

@sdedic sdedic commented Jan 16, 2025

This should go to VSNetbeans release branch as well.
This PR fixes two issues:

  1. NBLS now fails to start on JDK19+, because of
java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
	at java.base/java.lang.System.setSecurityManager(System.java:425)
	at org.netbeans.TopSecurityManager.install(TopSecurityManager.java:525)
	at org.netbeans.core.NbLifecycleManager.advancePolicy(NbLifecycleManager.java:69)
	at org.netbeans.core.GuiRunLevel.run(GuiRunLevel.java:84)

The option controlling SecurityManager is missing from the NBLS app configuration. For compatibility with JDK24, I've decided to disable TopSecurityManager completely (-J-DTopSecurityManager.disable=true).

  1. During debugging, I have noticed that NBLS starts repeatedly over and over again. It's annoying especially since the dialogs printed steal focus from e.g. command palette etc.
    This patch changes the startup so that automatic restart after 10sec happes ONLY if the language server initializes correctly; if it fails to start completely, it is not respawned.

If you test this, please test:

  • pristine start with NO JAVA on PATH and in JAVA_HOME
  • configure with unsuitable JDK, e.g. JDK8-11
  • valid configuration
  • start with no or invalid JDK then accept the offer to configure JDK for NBLS
  • crash (kill) of a NBLS server after successful startup
  • change to vital parameter such as "netbeans.javaSupport.enabled" that should purposely restart NBLS

@sdedic sdedic added kind:bug Bug report or fix VSCode Extension labels Jan 16, 2025
@sdedic sdedic added this to the NB25 milestone Jan 16, 2025
@sdedic sdedic self-assigned this Jan 16, 2025
lahodaj
lahodaj approved these changes Jan 16, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

I didn't test all the combinations, but I did try some cases, and it seems reasonable to me.

@jglick
Copy link
Copy Markdown
Contributor

jglick commented Jan 16, 2025

Maybe I am missing something but should

netbeans/platform/o.n.core/src/org/netbeans/core/NbLifecycleManager.java

Lines 67 to 69 in daa8132

if (!Boolean.getBoolean("TopSecurityManager.disable")) {
// set security manager
TopSecurityManager.install();
not automatically skip this when running on Java 21, since it is guaranteed to throw this exception? I just tried ant tryme on trunk and it failed for this reason, which seems like a pretty basic problem.

@mbien
Copy link
Copy Markdown
Member

mbien commented Jan 16, 2025 •
edited
Loading

Hi @jglick!

not automatically skip this when running on Java 21, since it is guaranteed to throw this exception?

SM itself works fine up until JDK 23 (inclusive) (outside of deprecation warnings and that JDK 18 made 'disallow' the default which can still be changed to 'allow'). NB 24 can be built and run on JDK 17-23 with SM still active.

JDK 24 is more interesting since it disabled SM. If you grab a dev build from master, you can start it on JDK 24 as experiment if you replace -J-Djava.security.manager=allow with -J-DTopSecurityManager.disable=true in the conf. But those flags made it into everything by now, so to run tests on JDK 24 etc more has to be done #7928.

Might be good to add VSNetbeans to the title so that this is not confused with classic NB - since NB doesn't restart ;)

dbalek
dbalek approved these changes Jan 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@dbalek dbalek left a comment

Choose a reason for hiding this comment

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

Looks fine.

@sdedic sdedic changed the title Disable SecurityManager [compat:jdk19-24]. Avoid repeated starts on failure. VSNetBeans: Disable SecurityManager [compat:jdk19-24]. Avoid repeated starts on failure. Jan 17, 2025
@sdedic sdedic merged commit cb55d6b into apache:master Jan 17, 2025
@sdedic
Copy link
Copy Markdown
Member Author

sdedic commented Jan 17, 2025

Cherrypicked into vsnetbeans_2499 as 43ceb60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbalek dbalek dbalek approved these changes

@MartinBalin MartinBalin MartinBalin approved these changes

@lahodaj lahodaj lahodaj approved these changes

Assignees

@sdedic sdedic

Labels

kind:bug Bug report or fix VSCode Extension

Projects

None yet

Milestone

NB25

Development

Successfully merging this pull request may close these issues.

6 participants

@sdedic @jglick @mbien @dbalek @MartinBalin @lahodaj