[1.7.10] Core Mods Class Loading Problem (Forge 1399)

[lumien231]

Not sure whether that affects ALL core mods but i would assume it does.

Asm was added to the LaunchClassLoader exclusion list in 91338433fa74e782e237643632de2cc5e17ee280.

This causes crashes like these:

http://pastebin.com/NdHvExxP https://gist.github.com/Xee1/adbce07b4e64961d3d90

For creating the Stack Map Frame asm sometimes loads classes to determine a common super class of 2 classes, since its no longer loaded by the LaunchClassLoader it crashes while doing that. This can be fixed by extending ClassWriter and making it use the LaunchClassLoader.

I'm not sure whether this should just be fixed on the modders side or can be somehow resolved in Forge.

[cpw]

Well, to fix, we'd need to revert a change for mixin compatibility, something that is rather important..

[lumien231]

I'm not entirely sure what mixin does but why does it require asm in the system class loader?

[LexManos]

Neither am I and I figured something like this might happen. @Mumfrey However, coremods can fix it themselves so it's a non-issue.

[simon816]

See https://github.com/SpongePowered/Mixin/issues/39

[LexManos]

I have seen that and I don't accept it. It should be functional entirely inside the LCL.

[Kubuxu]

@LexManos IMHO it is an isssue as for now even forge contains possibly crashing ClassWriters.

[diesieben07]

The root of the issue (this one at least, not sure about what mixins do exactly) is that the default implementation of ClassWriter calls Class.forName on it's own ClassLoader. This can be partially fixed by either not using COMPUTE_FRAMES (only a partial workaround, since it only works with classfiles up to java 6, which are going away...) or letting it use the LaunchClassLoader, which is still only a partial workaround, calling Class.forName on arbitrary classes during transformation is a no-go. The "proper" way (imho) to do this would be to analyze the classes using ASM and compute the superclass that way. I do that in my coremod and it works fine.

[cpw]

@diesieben07 basically, the fix provided by sponge is wholly incomplete. They also needed to provide an alternative implementation of ClassWriter that avoids the use of Class.forName() in ClassWriter.getCommonSuperClass, so that coremods can use that alternative ClassWriter instead. Of course, all coremods using classwriters (every single one) would need to be changed to use this new factory instead, as well.

Additionally, it really has to be rolled back @ 1.7.10 since it's such a brutally coremod incompatible change. Thanks for that, sponge folks!

[diesieben07]

If desired I will make a PR with my implementation that uses ASM.

[cpw]

@diesieben07 I would let the sponge folks handle it. FML has rolled back the change @ 1.7.10, since it's ridiculously incompatible. It may or may not stay @ 1.8, but there, a factory for classwriters that is compatible with this change would be required.

[Mumfrey]

@diesieben07 basically, the fix provided by sponge is wholly incomplete. They also needed to provide an alternative implementation of ClassWriter that avoids the use of Class.forName() in ClassWriter.getCommonSuperClass, so that coremods can use that alternative ClassWriter instead. Of course, all coremods using classwriters (every single one) would need to be changed to use this new factory instead, as well.

That's what I do internally, but as you say it's other mods using ClassWriter which actually tend to become the issue then, and that's just not workable. I had anticipated that the exclusion for ASM may fix the issue but didnt' have time to do any testing of my own. I understood that it worked.

Additionally, it really has to be rolled back @ 1.7.10 since it's such a brutally coremod incompatible change. Thanks for that, sponge folks!

Roll it back if necessary. I don't believe in forcing other people to clean up my mess and wouldn't have endorsed the change if I'd realised that it affected behaviour this way.

@diesieben07 I would let the sponge folks handle it. FML has rolled back the change @ 1.7.10, since it's ridiculously incompatible. It may or may not stay @ 1.8, but there, a factory for classwriters that is compatible with this change would be required.

I'll handle it. In all honesty the solution is to crawl through ASM and find all the places I'm likely to trigger which cause a class load, at that point Mixin can come up into LCL as well. The reason it can't at the moment is there are still behaviours in ASM which cause class loads which subsequently lead to ClassCircularityErrors when the the classloader comes to consume the class.

An alternative short-term solution for Mixin might be to shade a renamed ASM library (I believe this is what some other libraries which use ASM do in order to avoid exactly this kind of issue) which was something I tabled before. If the exclusion causes this kind of grief for people then I will explore other avenues.

Again, I'm not trying to make this anyone else's problem. Throw it back in my court and I'll deal with it.

[cpw]

@Mumfrey it's rolled back @ 1.7 since the preponderance of coremods there really necessitated it. I think that simply making a classwriter factory that coremods can use would be the best solution. I tend to agree that ASM should be outside the LCL myself, so I didn't protest at the time. Coremods are a specialized usecase and they should understand the problem of class scoping. In my opinion, the best solution is a special ClassWriter factory that can be used by coremods to get a ClassWriter scoped inside or outside.

[Mumfrey]

I guess that overall it would be a better solution, since the root of the issue is in the assumptions ClassWriter makes about the environment (which are perfectly sensible assumptions, they just break in this situation). However this doesn't help the folks wanting to run Mixin on 1.7.10 (or does it?)

I'm perfectly happy with a short-term solution that keeps things rolling while a long term solution can be properly engineered/have time to reach the wild, so if shading the library makes the problems go away then I feel it's appropriate to do so. It will grow some jars a little in the short term, but that cost can be mitigated once the better solution can be implemented.

Appreciate any input you have on the matter to be perfeclty honest, I mainly just don't want anyone to feel like I'm shirking responsibility by making this an FML problem.

[yuuka-miya]

As one of "the folks wanting to run Mixin on 1.7.10", let me know if there's any solution that's available to test.

As a note: When I did some simple testing with Fastcraft 1.21 and a Mixin-enabled project (FE), everything seemed to work well. Other people I asked to help test also tried with popular modpacks and gave me the all-clear.

@mumfrey go ahead with the package-rename for ASM for now. Expect a PR from me shortly in regards to the FMLCorePluginContainsFMLMod issue I talked to you about.

[cpw]

@luacs1998 How? We change it at 1.7.10 and we break most of the coremod world, including possibly FML itself. The probability of updating every 1.7.10 coremod to use a ClassWriter factory is effectively nil. I welcome your proposed solution.

[cpw]

@Mumfrey you could shade a special 1.7.10 Mixin with a custom ASM? That might get @luacs1998 working...

[yuuka-miya]

I said "let me know if there's any solution available to test".

I understand the ramifications of breaking so many coremods in 1.7.10, so I don't mind if any solution is applied on 1.8 only.

Mixin is version-agnostic so if he shades ASM, it applies to all builds. I'll speak with the Gradle God Abrar on just shading ASM with FE, so no need to trouble him.

[Mumfrey]

@cpw yes, that would be my starter for 10. Because it would be an internal ASM used only by Mixin (and of course any mods providing IMixinConfigPlugin instances) so I'd need to shade-with-rename in the compiled mixin artefact rather than having coremod consumers do it themselves.

@luacs1998 Because of the need for the mixin processor to be (effectively) a singleton, or rather the environment be shared. It's not really possible for a consuming coremod to rename mixin or ASM, I will have to shade and rename at my end.

[Kubuxu]

The solution might be containing modified ClassWriter inside FML itself. During class-loading jars are searched in the order of mention in the classpath parameter. If FML contained ClassWriter class and would be ensured to be before ASM in classpath it would effectively shadowed the original ClassWriter.

[cpw]

@Kubuxu no. I'm not going to provide an alternative implementation of the actual ClassWriter class. That is ridiculously broken in many respects.

[Mumfrey]

For the time being I've gone down the route of shading a remapped ASM, I've put this on a branch for now until @luacs1998 can give me some feedback on how it works in the wild. The longer-term intention is to track down all of the places in ASM that trigger unwanted class load events and provide alternative implementations for them which use my internal class metadata model (as I already did with ClassWriter). Once this is achieved, I'll be able to finally remove the classloader exclusion for Mixin and run it in the LaunchClassLoader.

This effectively leaves you free to deal with this in FML for 1.8 as you see fit, either reverting the change or providing the relevant ClassWriter factory as mentioned above. Sorry for the inconvenience this has caused in the mean time.