[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 9133843.

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 SpongePowered/Mixin#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.