[JVM Crash] Remove AllowEnhancedClassRedefinition capability override

[SirYwell]

We experience a crash when attaching async-profiler to a running JVM in combination with the AllowEnhancedClassRedefinition flag. Debugging showed that this is caused by https://github.com/JetBrains/JetBrainsRuntime/blob/41109ce8ba5e7ca202739ea4c03c9c4d7ce2dcff/src/hotspot/share/prims/jvmtiEnv.cpp#L2985 because JvmtiExport::can_maintain_original_method_order() is true but the method ordering array wasn't initialized.

We don't quite understand why the flag unconditionally overrides the capability, as the method ordering array is set when the instance class is parsed initially. Retroactively changing the capability does not re-initialize the existing classes, leading to a state mismatch and a JVM crash. Therefore, we propose removing this override.

Alternatively, the AllowEnhancedClassRedefinition flag can be added to more places to ensure that the method ordering array is populated during class file parsing. We explored this change in https://github.com/SirYwell/JetBrainsRuntime/commit/62fd69e82d40976a128a3d2cbc8e3a543243178f.

This was initially reported by https://github.com/lucko/spark/issues/426.

I'm not sure how you deal with backports, I'm targetting the jbr21 branch here because that's the version this bug was observed on. Please let me know if you want me to target a different branch.

cc @I-Al-Istannen @intrigus-lgtm

[skybber]

Thank you for the report. I have created a YouTrack issue for it:

YouTrack Issue JBR-7442

The error occurred because I wanted to safe some testing of the flag and didn't see crash location. I reviewed your commit and it looks good. Could you please add this commit to the current PR and modify the commit message to include the prefix [JBR-7442](https://youtrack.jetbrains.com/issue/JBR-7442/dcevm-crash-remove-AllowEnhancedClassRedefinition-capability-override)? If you don't have enough time, I will do it for you.

[SirYwell]

I reviewed your commit and it looks good. Could you please add this commit to the current PR and modify the commit message to include the prefix [JBR-7442](https://youtrack.jetbrains.com/issue/JBR-7442/dcevm-crash-remove-AllowEnhancedClassRedefinition-capability-override)? If you don't have enough time, I will do it for you.

Just to make sure, you want me to combine both changes? From our investigation, only one of them is needed, we just weren't sure what the reason for the capability override is.

[skybber]

The old/new method matcher algorithm in DCEVM is based on strict method ordering. The fix was originally introduced with JBR-6363. The issue arose because some new classes had methods in a different order compared to the old classes, leading the method matching algorithm in DCEVM to incorrectly mark some methods as deleted. It seems to me that the checking of AllowEnhancedClassRedefinition is necessary in classFileParser and jvmtiClassFileReconstituter

[SirYwell]

I see, so the capabilities don't require the override but the places where the ordering is created require an override to ensure DCEVM has the information available?

I assume GetClassMethods therefore does not need the override but only rely on the capability?

Do you want me to squash the commits directly?

[skybber]

I see, so the capabilities don't require the override but the places where the ordering is created require an override to ensure DCEVM has the information available?

I assume GetClassMethods therefore does not need the override but only rely on the capability?

Do you want me to squash the commits directly?

Yes, GetClassMethods does not need it, but it is required in other places. Plaease squash it, thanks !

[SirYwell]

I went through the usages of JvmtiExport::can_maintain_original_method_order() again and spotted two more spots where the flag isn't necessarily required but I think it makes sense to use it: https://github.com/JetBrains/JetBrainsRuntime/blob/ff722c3b2a197f806e60c0e88da60c83393f1207/src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp#L870 https://github.com/JetBrains/JetBrainsRuntime/blob/b5d182dad5d8cad85ba5ad9a6797d4f114cc2ee5/src/hotspot/share/oops/instanceKlass.cpp#L4088

Please let me know if you want me to change anything else

[skybber]

Could you please modify commit message to prefix:

JBR-7442 move Allo...

[skybber]

Thank you for your contribution. Please be aware that the current DCEVM in the JBR21 branch is crashing. For more details, refer to JBR-7447.