Fix NPE in JRTUtil

[laeubi]

What it does

In some cases JRTUtil#getJrtSystem() can return null what results in a NPE a caller most likley do not exspect.

This adds required null checks and act accordingly.

Fix #1154

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[laeubi]

@ jarthana @iloveeclipse can you take a look here, this is really annoying and currently makes working with the Java Search impossible in my Platform-IDE-SDK as it seems I have some project imported that triggers the NPE every time.
Test failure seems unrelated as I see it on other changes as well (e.g. here: https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-676/10/)

[iloveeclipse]

Please rebase to get rid of #1150 issue.

Also I believe your issue could be caused by eclipse-jdt/eclipse.jdt.debug#231 and your fix is applied at wrong place.

I assume somehow the "autoimport" of JDK/JRE managed to setup invalid JDK/JRE.

Please check that: which is now the default one and how to prevent importing that one in first place.
Hint: I assume some project now uses JRE, not JDK as default, or something like this.

[laeubi]

Also I believe your issue could be caused by eclipse-jdt/eclipse.jdt.debug#231 and your fix is applied at wrong place.

I think this fix is good anyways, as obviously there can be null returned, so the code should handle this.

I assume somehow the "autoimport" of JDK/JRE managed to setup invalid JDK/JRE.

I'll check that, how would I identify such "invalid JDK"? If i look at the dialog it all looks quite usual

I also selected each JDK and hit "edit" but no error was reported.

[iloveeclipse]

I'll check that, how would I identify such "invalid JDK"?

If you can always reproduce your issue, try to debug it and see which JDK causes the problem.
Do you have other errors in the log that could give a hint?

[laeubi]

As it is on the batch-compiler it seems only system-out is printing something I started eclipse with a console now and see this error: #1154 (comment)

[laeubi]

@jarthana @iloveeclipse I have rebased this now and all tests seem green. If you think it should be fixed at another place I'm fine with that but this currently makes my Eclipse-Ide somewhat unuseable...

[iloveeclipse]

If you think it should be fixed at another place I'm fine

I think if possible we should check the presence of that file on adding such JRE's.

with that but this currently makes my Eclipse-Ide somewhat unuseable...

Just remove or fix that Java 9 installation (/usr/lib/jvm/java-9-openjdk-amd64/release is missing).

[laeubi]

Just remove or fix that Java 9 installation (/usr/lib/jvm/java-9-openjdk-amd64/release is missing).

If you can point to the spec that it requires a release file to be present for a JVM I can try to "fix" this, but unless then I fear I won't get much attention for that, I probably can even create an empty file, remove the JVM and so on, but to the core I think JDT can simply better handle the case, a NPE is from my point of view never acceptable here and the code obviously can return null, so what ever seems appropriate (ignore, throw an documented exception, ...) should be applied to JDT in parallel.

[iloveeclipse]

If you can point to the spec that it requires a release file to be present for a JVM I can try to "fix" this, but unless then I fear I won't get much attention for that

Well, almost all of JDK internal structure is not spec'd, even ct.sym is not - should we now rewrite all of JDT code that relies on the JDK internals? You've complained about, and I've offered a solution that would immediately help. You don't need to apply this solution.

Note: the problem with "broken" JDK will appear surely later on in some different code in JDT, you may not even notice where it will be because the search or call hierarchy would return only a subset of matches.

[laeubi]

Well, almost all of JDK internal structure is not spec'd

And that's why I suggest to handle such cases... some of the methods are even throwing an exception so callers can handle / forward this, but the NPE bubbles up into an ugly error dialog I have to dismiss all the time.

[iloveeclipse]

The change itself is not complete, but can be merged. I will push a follow up PR.

[iloveeclipse]

I will push a follow up PR to cleanup this.

See #1170

[laeubi]

deally getJrtSystem() should re-throw original IO exception where possible, not simply return null or false (the caller is supposed to properly handle this case as exception).

Thanks I also has thought about it, but was not sure about the consequences (especially as it seems one needs to enable exceptions explicitly). The followup looks like a good path forward!