Fix cloning in 1.17

Addresses #1222

Author: dmulloy2

.travis.yml

@@ -1,6 +1,8 @@
 language: java
+os: linux
+dist: focal
 jdk:
-  - openjdk8
+  - openjdk16
 script: mvn clean test
 install: true
 notifications:

src/main/java/com/comphenix/protocol/events/PacketContainer.java

@@ -35,6 +35,7 @@
 import com.comphenix.protocol.reflect.cloning.AggregateCloner.BuilderParameters;
 import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract;
 import com.comphenix.protocol.reflect.instances.DefaultInstances;
+import com.comphenix.protocol.reflect.instances.InstanceProvider;
 import com.comphenix.protocol.utility.MinecraftMethods;
 import com.comphenix.protocol.utility.MinecraftReflection;
 import com.comphenix.protocol.utility.MinecraftVersion;
@@ -82,11 +83,11 @@ public class PacketContainer implements Serializable {
 	// Support for serialization
 	private static ConcurrentMap<Class<?>, Method> writeMethods = Maps.newConcurrentMap();
 	private static ConcurrentMap<Class<?>, Method> readMethods = Maps.newConcurrentMap();
-	
+
 	// Used to clone packets
 	private static final AggregateCloner DEEP_CLONER = AggregateCloner
 			.newBuilder()
-			.instanceProvider(DefaultInstances.DEFAULT)
+			.instanceProvider(StructureCache::newPacket)
 			.andThen(BukkitCloner.class)
 			.andThen(ImmutableDetector.class)
 			.andThen(JavaOptionalCloner.class)
@@ -97,7 +98,7 @@ public class PacketContainer implements Serializable {
 
 	private static final AggregateCloner SHALLOW_CLONER = AggregateCloner
 			.newBuilder()
-			.instanceProvider(DefaultInstances.DEFAULT)
+			.instanceProvider(StructureCache::newPacket)
 			.andThen(param -> {
 				if (param == null)
 					throw new IllegalArgumentException("Cannot be NULL.");
@@ -110,8 +111,12 @@ public class PacketContainer implements Serializable {
 			.build();
 
 	// Packets that cannot be cloned by our default deep cloner
-	private static final Set<PacketType> CLONING_UNSUPPORTED = Sets.newHashSet(
-		PacketType.Play.Server.UPDATE_ATTRIBUTES, PacketType.Status.Server.SERVER_INFO);
+	private static final Set<PacketType> FAST_CLONE_UNSUPPORTED = Sets.newHashSet(
+		PacketType.Play.Server.BOSS,
+		PacketType.Play.Server.ADVANCEMENTS,
+		PacketType.Play.Client.USE_ENTITY,
+		PacketType.Status.Server.SERVER_INFO
+	);
 
 	/**
 	 * Creates a packet container for a new packet.
@@ -1155,14 +1160,16 @@ public PacketContainer shallowClone() {
 	 */
 	public PacketContainer deepClone() {
 		Object clonedPacket = null;
-		
-		// Fall back on the alternative (but slower) method of reading and writing back the packet
-		if (!CLONING_UNSUPPORTED.contains(type)) {
+
+		if (!FAST_CLONE_UNSUPPORTED.contains(type)) {
 			try {
 				clonedPacket = DEEP_CLONER.clone(getHandle());
-			} catch (Exception ignored) {}
+			} catch (Exception ex) {
+				FAST_CLONE_UNSUPPORTED.add(type);
+			}
 		}
 
+		// Fall back on the slower alternative method of reading and writing back the packet
 		if (clonedPacket == null) {
 			clonedPacket = SerializableCloner.clone(this).getHandle();
 		}

src/main/java/com/comphenix/protocol/injector/StructureCache.java

@@ -26,7 +26,6 @@
 import com.comphenix.protocol.injector.packet.PacketRegistry;
 import com.comphenix.protocol.reflect.StructureModifier;
 import com.comphenix.protocol.reflect.compiler.BackgroundCompiler;
-import com.comphenix.protocol.reflect.compiler.CompileListener;
 import com.comphenix.protocol.reflect.compiler.CompiledStructureModifier;
 import com.comphenix.protocol.reflect.instances.DefaultInstances;
 import com.comphenix.protocol.utility.MinecraftReflection;
@@ -41,9 +40,28 @@
  */
 public class StructureCache {
 	// Structure modifiers
-	private static ConcurrentMap<PacketType, StructureModifier<Object>> structureModifiers = new ConcurrentHashMap<>();
+	private static final ConcurrentMap<PacketType, StructureModifier<Object>> structureModifiers = new ConcurrentHashMap<>();
 
-	private static Set<PacketType> compiling = new HashSet<>();
+	private static final Set<PacketType> compiling = new HashSet<>();
+
+	public static Object newPacket(Class<?> clazz) {
+		Object result = DefaultInstances.DEFAULT.create(clazz);
+
+		// TODO make these generic
+		if (result == null) {
+			try {
+				return clazz.getConstructor(PacketDataSerializer.class).newInstance(new PacketDataSerializer(new ZeroBuffer()));
+			} catch (ReflectiveOperationException ex) {
+				try {
+					return clazz.getConstructor(PacketDataSerializer.class).newInstance(new ZeroPacketDataSerializer());
+				} catch (ReflectiveOperationException ex1) {
+					throw new IllegalArgumentException("Failed to create packet: " + clazz, ex);
+				}
+			}
+		}
+
+		return result;
+	}
 
 	/**
 	 * Creates an empty Minecraft packet of the given type.
@@ -52,25 +70,8 @@ public class StructureCache {
 	 */
 	public static Object newPacket(PacketType type) {
 		Class<?> clazz = PacketRegistry.getPacketClassFromType(type, true);
-
-		// Check the return value
 		if (clazz != null) {
-			// TODO: Optimize DefaultInstances
-			Object result = DefaultInstances.DEFAULT.create(clazz);
-
-			if (result == null) {
-				try {
-					return clazz.getConstructor(PacketDataSerializer.class).newInstance(new PacketDataSerializer(new ZeroBuffer()));
-				} catch (ReflectiveOperationException ex) {
-					try {
-						return clazz.getConstructor(PacketDataSerializer.class).newInstance(new ZeroPacketDataSerializer());
-					} catch (ReflectiveOperationException ex1) {
-						throw new IllegalArgumentException("Failed to create packet for type: " + type, ex);
-					}
-				}
-			}
-
-			return result;
+			return newPacket(clazz);
 		}
 		throw new IllegalArgumentException("Cannot find associated packet class: " + type);
 	}
@@ -115,13 +116,13 @@ public static StructureModifier<Object> getStructure(Class<?> packetType, boolea
 	 * @return A structure modifier.
 	 */
 	public static StructureModifier<Object> getStructure(final PacketType type, boolean compile) {
-		Preconditions.checkNotNull(type);
+		Preconditions.checkNotNull(type, "type cannot be null");
 		StructureModifier<Object> result = structureModifiers.get(type);
 
 		// We don't want to create this for every lookup
 		if (result == null) {
 			// Use the vanilla class definition
-			final StructureModifier<Object> value = new StructureModifier<Object>(
+			final StructureModifier<Object> value = new StructureModifier<>(
 					PacketRegistry.getPacketClassFromType(type, true), MinecraftReflection.getPacketClass(), true);
 
 			result = structureModifiers.putIfAbsent(type, value);
@@ -139,12 +140,8 @@ public static StructureModifier<Object> getStructure(final PacketType type, bool
 				final BackgroundCompiler compiler = BackgroundCompiler.getInstance();
 
 				if (!compiling.contains(type) && compiler != null) {
-					compiler.scheduleCompilation(result, new CompileListener<Object>() {
-						@Override
-						public void onCompiled(StructureModifier<Object> compiledModifier) {
-							structureModifiers.put(type, compiledModifier);
-						}
-					});
+					compiler.scheduleCompilation(result,
+							compiledModifier -> structureModifiers.put(type, compiledModifier));
 					compiling.add(type);
 				}
 			}

src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java

@@ -117,12 +117,11 @@ private void copyToInternal(Object source, Object destination, Class<?> commonTy
 			}
 
 			// Copy private fields underneath
-			Class<?> superclass = commonType.getSuperclass();
-			
-			if (superclass != null && !superclass.equals(Object.class)) {
-				copyToInternal(source, destination, superclass, false);
-			}
-			
+//			Class<?> superclass = commonType.getSuperclass();
+//
+//			if (superclass != null && !superclass.equals(Object.class)) {
+//				copyToInternal(source, destination, superclass, false);
+//			}
 		} catch (FieldAccessException e) {
 			throw new RuntimeException("Unable to copy fields from " + commonType.getName(), e);
 		}

src/main/java/com/comphenix/protocol/reflect/cloning/AggregateCloner.java

@@ -256,11 +256,11 @@ public Object clone(Object source) {
 		if (index < cloners.size()) {
 			Cloner cloner = cloners.get(index);
 
-			try {
-				return cloner.clone(source);
-			} catch (Exception ex) {
-				throw new RuntimeException("Failed to clone " + source + " (" + source.getClass() + ") with " + cloner, ex);
-			}
+//			try {
+			return cloner.clone(source);
+//			} catch (Exception ex) {
+//				throw new RuntimeException("Failed to clone " + source + " (" + source.getClass() + ") with " + cloner, ex);
+//			}
 		}
 
 		// Damn - failure

src/test/java/com/comphenix/protocol/events/PacketContainerTest.java

@@ -413,7 +413,7 @@ public void testAttributeList() {
 				}
 
 				field.setAccessible(true);
-				assertEquals(field.get(snapshot), field.get(clonedSnapshot));
+				testEquality(field.get(snapshot), field.get(clonedSnapshot));
 			} catch (AssertionError e) {
 				throw e;
 			} catch (Throwable ex) {
@@ -611,8 +611,27 @@ public void testComponentArrays() {
 		assertArrayEquals(components, back);
 	}
 
+	private void assertPacketsEqual(PacketContainer constructed, PacketContainer cloned) {
+		StructureModifier<Object> firstMod = constructed.getModifier(), secondMod = cloned.getModifier();
+		assertEquals(firstMod.size(), secondMod.size());
+
+		if (PacketType.Status.Server.SERVER_INFO.equals(constructed.getType())) {
+			assertArrayEquals(SerializationUtils.serialize(constructed), SerializationUtils.serialize(cloned));
+		} else {
+			// Make sure all the fields are equivalent
+			for (int i = 0; i < firstMod.size(); i++) {
+				if (firstMod.getField(i).getType().isArray())
+					assertArrayEquals(getArray(firstMod.read(i)), getArray(secondMod.read(i)));
+				else
+					testEquality(firstMod.read(i), secondMod.read(i));
+			}
+		}
+	}
+
 	@Test
-	public void testDeepClone() {
+	public void testCloning() {
+		boolean failed = false;
+
 		// Try constructing all the packets
 		for (PacketType type : PacketType.values()) {
 			if (type.isDeprecated() || type.name().contains("CUSTOM_PAYLOAD") || type.name().contains("TAGS") || !type.isSupported()
@@ -644,28 +663,19 @@ public void testDeepClone() {
 					//constructed.getModifier().write(1, TEST_COMPONENT);
 				}
 
-				// Clone the packet
-				PacketContainer cloned = constructed.deepClone();
-
-				// Make sure they're equivalent
-				StructureModifier<Object> firstMod = constructed.getModifier(), secondMod = cloned.getModifier();
-				assertEquals(firstMod.size(), secondMod.size());
-
-				if (PacketType.Status.Server.SERVER_INFO.equals(type)) {
-					assertArrayEquals(SerializationUtils.serialize(constructed), SerializationUtils.serialize(cloned));
-				} else {
-					// Make sure all the fields are equivalent
-					for (int i = 0; i < firstMod.size(); i++) {
-						if (firstMod.getField(i).getType().isArray())
-							assertArrayEquals(getArray(firstMod.read(i)), getArray(secondMod.read(i)));
-						else
-							testEquality(firstMod.read(i), secondMod.read(i));
-					}
-				}
+				// Clone the packet both ways
+				PacketContainer shallowCloned = constructed.shallowClone();
+				assertPacketsEqual(constructed, shallowCloned);
+
+				PacketContainer deepCloned = constructed.deepClone();
+				assertPacketsEqual(constructed, deepCloned);
 			} catch (Exception ex) {
 				ex.printStackTrace();
+				failed = true;
 			}
 		}
+
+		assertFalse("Packet(s) failed to clone", failed);
 	}
 
 	// Convert to objects that support equals()