Implement CloseContext remapper for PlayerQuitEvent handling#260
Implement CloseContext remapper for PlayerQuitEvent handling#260twisti-dev merged 2 commits intoversion/1.21.11from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a runtime ByteBuddy/ASM-based remap to make the shaded inventory-framework close handling tolerant to non-InventoryCloseEvent origins (notably PlayerQuitEvent), preventing runtime ClassCastExceptions during view close handling.
Changes:
- Added
CloseContextRemapperto rewriteCloseContextandBukkitElementFactorybytecode so the close-origin is treated asObjectand the unsafe cast is removed. - Hooked the new remapper into
InventoryLoaderinitialization so it runs as soon as the inventory framework is loaded. - Bumped project version to
1.21.11-2.70.2.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| surf-api-bukkit/surf-api-bukkit-server/src/main/kotlin/dev/slne/surf/surfapi/bukkit/server/inventory/framework/InventoryLoader.kt | Calls the new remapper during inventory framework bootstrap. |
| surf-api-bukkit/surf-api-bukkit-server/src/main/kotlin/dev/slne/surf/surfapi/bukkit/server/inventory/framework/CloseContextRemapper.kt | Implements the ByteBuddy/ASM rewrite for CloseContext + BukkitElementFactory. |
| gradle.properties | Version bump for the release containing the compatibility fix. |
| init { | ||
| InventoryViewRemapper.remap() | ||
| CloseContextRemapper.remap() | ||
| } |
There was a problem hiding this comment.
CloseContextRemapper.remap() is invoked during InventoryLoader static initialization without any failure isolation. If the target classes are missing/renamed or injection fails, this will throw during object init and can prevent the plugin from enabling. Consider wrapping the remap call in a guarded/optional path (e.g., catch Throwable and log a warning) so startup doesn’t hard-fail on incompatible inventory-framework versions.
| private fun remapCloseContext() { | ||
| val locator = ClassFileLocator.ForClassLoader.of(javaClass.classLoader) | ||
| val typePool = TypePool.Default.of(locator) | ||
| val typeDescription = typePool.describe(CLOSE_CONTEXT_INTERNAL.replace("/", ".")).resolve() | ||
|
|
||
| ByteBuddy() | ||
| .redefine<Any>(typeDescription, locator) | ||
| .visit(CloseContextClassVisitorWrapper()) | ||
| .make() | ||
| .load(javaClass.classLoader, ClassLoadingStrategy.Default.INJECTION) | ||
| } |
There was a problem hiding this comment.
remapCloseContext()/remapBukkitElementFactory() unconditionally resolve types via TypePool.describe(...).resolve() and then inject with ClassLoadingStrategy.Default.INJECTION. Both steps can throw (type not found, class already loaded -> LinkageError, etc.), but the errors aren’t handled here. To make this safer in production, add a cheap “is remap necessary?” check (similar to InventoryViewRemapper.isRemapNecessary()), and/or catch Throwable and log/skip when remapping can’t be applied.
| override fun visitMethod( | ||
| access: Int, | ||
| name: String, | ||
| descriptor: String, | ||
| signature: String?, | ||
| exceptions: Array<String>? | ||
| ): MethodVisitor { | ||
| // Change method descriptor: replace InventoryCloseEvent with Object | ||
| val remappedDescriptor = remapDescriptor(descriptor) | ||
| val mv = super.visitMethod(access, name, remappedDescriptor, signature, exceptions) | ||
| return CloseContextMethodVisitor(mv) | ||
| } |
There was a problem hiding this comment.
CloseContextClassVisitor.visitMethod() remaps the descriptor for every method in CloseContext. That widens the blast radius beyond the constructor/field and can cause linkage issues if any other inventory-framework classes (or already-loaded code) still reference the original method descriptors. Consider restricting descriptor rewriting to the specific members that need to change (e.g., <init> and the closeOrigin accessor/field), so the patch is as minimal as possible.
| override fun visitMethod( | ||
| access: Int, | ||
| name: String, | ||
| descriptor: String, | ||
| signature: String?, | ||
| exceptions: Array<String>? | ||
| ): MethodVisitor { | ||
| val mv = super.visitMethod(access, name, descriptor, signature, exceptions) | ||
|
|
||
| // Only remap the createCloseContext method | ||
| if (name == "createCloseContext") { | ||
| return BukkitElementFactoryMethodVisitor(mv) | ||
| } | ||
|
|
||
| return mv | ||
| } |
There was a problem hiding this comment.
BukkitElementFactoryClassVisitor targets methods solely by name == "createCloseContext". If the upstream library adds an overload with the same name, this visitor could unintentionally rewrite the wrong method. Matching on both name and descriptor (or verifying the method bytecode actually contains the InventoryCloseEvent cast / CloseContext.<init> call) would make this remap more robust across library updates.
This pull request introduces a new runtime bytecode remapping solution to address a compatibility issue with the inventory-framework library, specifically around the handling of close events in inventory views. The main change is the addition of the
CloseContextRemapper, which rewrites the bytecode of third-party classes so that theCloseContextclass and related factory methods can accept any event object (not justInventoryCloseEvent), preventing runtimeClassCastExceptions when handling player quit events.The most important changes are:
Bytecode Remapping for Inventory Framework Compatibility:
CloseContextRemapperinCloseContextRemapper.kt, which uses ByteBuddy and ASM to rewrite theCloseContextandBukkitElementFactoryclasses at runtime so that thecloseOriginparameter and field are typed asObjectinstead ofInventoryCloseEvent. This removes unsafe casts and makes the code compatible with bothInventoryCloseEventandPlayerQuitEvent.InventoryLoader.ktto invokeCloseContextRemapper.remap()during initialization, ensuring the remapping is applied as soon as the inventory framework is loaded.Versioning:
gradle.propertiesfrom1.21.11-2.70.1to1.21.11-2.70.2to reflect these changes.