Fix Quarkus ClassLoader's jar ProtectionDomain handling

[geoand]

Bring it inline with what URLClassLoader does

Fixes: #11549

[geoand]

@stuartwdouglas I assume that the current code was just the result of a bug? Or did you have something specific in mind?

[geoand]

The CI is due to this PR - looking at it now

[geoand]

CI issue should now be fixed - Pinging @jaikiran as well since I made a change to his code

[jaikiran]

I'm a bit surprised that it's failing in JRE, because from what I can tell, I don't think the current code in JarClassPathElement is doing anything incorrect. Let me see if I can reproduce this standalone outside of Quarkus.

[geoand]

@jaikiran #11549 (comment) (which I was able to reproduce) was the reason I made this change

[jaikiran]

I could reproduce this outside of Quarkus in a simple Java class. So I asked for inputs from the JDK team[1]. The initial assessment is that, the construct used by Kotlin(?) and that other library in the linked issue:

new JarInputStream(kotlinUnit.getProtectionDomain().getCodeSource().getLocation().openStream())

isn't expected to work when the URL in question is a jar:file URL.

So I went ahead and tried out simple test to see how URLClassLoader behaves when it comes to CodeSource location and what URL it returns for classes loaded out of jar files in the classpath. My tests show that it matches with your changes and it returns a plain file: URL (example file:/tmp/some-jar.jar).

So this change looks fine to me.

[1] https://mail.openjdk.java.net/pipermail/net-dev/2020-September/014389.html

[geoand]

Awesome, thanks for the great analysis @jaikiran!

[jaikiran]

These URI/URL APIs are sometimes a bit too complex and I usually have to go back read their docs multiple times and even try out a few things to be sure that whatever construct is being used/changed will work fine without issues. So, I'm unsure if this change will cause any issues. However, given that this specific area of code has relevant tests[1] that cover Java 8, 11, 14 and even native-image, we can go ahead with this change if the CI shows all green.

[1] #9474

[geoand]

Agreed :)

[geoand]

CI failure seems totally unrelated, but since this change could potentially have a wide impact, I will restart CI since I want to be totally sure

[gsmet]

@stuartwdouglas I let you check this one? I will backport it tomorrow morning.