Properly remove previous hook, even if socket is NULL. FIXES Ticket-1.

ProtocolLib/dependency-reduced-pom.xml

@@ -4,7 +4,7 @@
   <groupId>com.comphenix.protocol</groupId>
   <artifactId>ProtocolLib</artifactId>
   <name>ProtocolLib</name>
-  <version>1.4.1</version>
+  <version>1.4.2-SNAPSHOT</version>
   <description>Provides read/write access to the Minecraft protocol.</description>
   <url>http://dev.bukkit.org/server-mods/protocollib/</url>
   <developers>

ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetLoginInjector.java

@@ -63,6 +63,9 @@ class NetLoginInjector {
 			InjectContainer container = (InjectContainer) fakePlayer;
 			container.setInjector(injector);
 			
+			// Save the login
+			injectedLogins.putIfAbsent(inserting, injector);
+			
 			// NetServerInjector can never work (currently), so we don't need to replace the NetLoginHandler
 			return inserting;	
 			
@@ -93,8 +96,30 @@ class NetLoginInjector {
 		PlayerInjector injected = injectedLogins.get(removing);
 		
 		if (injected != null) {
-			injected.cleanupAll();
+			PlayerInjector newInjector = null;
+			Player player = injected.getPlayer();
+			
+			// Clean up list
 			injectedLogins.remove(removing);
+			
+			// No need to clean up twice
+			if (injected.isClean())
+				return;
+			
+			// Hack to clean up other references
+			newInjector = injectionHandler.getInjectorByNetworkHandler(injected.getNetworkManager());
+			
+			// Update NetworkManager
+			if (newInjector == null) {
+				injectionHandler.uninjectPlayer(player);
+			} else {
+				injectionHandler.uninjectPlayer(player, false);
+				
+				if (injected instanceof NetworkObjectInjector)
+					newInjector.setNetworkManager(injected.getNetworkManager(), true);
+			}
+			
+			//logger.warning("Using alternative cleanup method.");
 		}
 	}
 	

ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkFieldInjector.java

@@ -166,7 +166,7 @@ class NetworkFieldInjector extends PlayerInjector {
 	}
 	
 	@SuppressWarnings("unchecked")
-	public void cleanupAll() {
+	protected void cleanHook() {
 		// Clean up
 		for (VolatileField overriden : overridenLists) {
 			List<Packet> minecraftList = (List<Packet>) overriden.getOldValue();

ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java

@@ -152,7 +152,7 @@ class NetworkObjectInjector extends PlayerInjector {
 	}
 	
 	@Override
-	public void cleanupAll() {
+	protected void cleanHook() {
 		// Clean up
 		if (networkManagerRef != null && networkManagerRef.isCurrentSet()) {
 			networkManagerRef.revertValue();

ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java

@@ -232,7 +232,7 @@ public class NetworkServerInjector extends PlayerInjector {
 	}
 		
 	@Override
-	public void cleanupAll() {
+	protected void cleanHook() {
 		if (serverHandlerRef != null && serverHandlerRef.isCurrentSet()) {
 			ObjectCloner.copyTo(serverHandlerRef.getValue(), serverHandlerRef.getOldValue(), serverHandler.getClass());
 			serverHandlerRef.revertValue();

ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java

@@ -281,28 +281,27 @@ public class PlayerInjectionHandler {
 						DataInputStream inputStream = injector.getInputStream(false);
 
 						Socket socket = injector.getSocket();
-						SocketAddress address = socket.getRemoteSocketAddress();
+						SocketAddress address = socket != null ? socket.getRemoteSocketAddress() : null;
 						
 						// Make sure the current player is not logged out
-						if (socket.isClosed()) {
+						if (socket != null && socket.isClosed()) {
 							throw new PlayerLoggedOutException();
 						}
 						
-						PlayerInjector previous = addressLookup.get(address);
+						// Guard against NPE here too
+						PlayerInjector previous = address != null ? addressLookup.get(address) : null;
 						
 						// Close any previously associated hooks before we proceed
 						if (previous != null) {
-							uninjectPlayer(previous.getPlayer());
+							uninjectPlayer(previous.getPlayer(), false, true);
-
-							// Remove the "hooked" network manager in our instance as well
-							if (previous instanceof NetworkObjectInjector) {
-								injector.setNetworkManager(previous.getNetworkManager(), true);
-							}
 						}
 	
 						injector.injectManager();
-						dataInputLookup.put(inputStream, injector);
+						
-						addressLookup.put(address, injector);
+						if (inputStream != null)
+							dataInputLookup.put(inputStream, injector);
+						if (address != null) 
+							addressLookup.put(address, injector);
 						break;
 					}
 					
@@ -362,8 +361,30 @@ public class PlayerInjectionHandler {
 	/**
 	 * Unregisters the given player.
 	 * @param player - player to unregister.
+	 * @return TRUE if a player has been uninjected, FALSE otherwise.
 	 */
-	public void uninjectPlayer(Player player) {
+	public boolean uninjectPlayer(Player player) {
+		return uninjectPlayer(player, true, false);
+	}
+	
+	/**
+	 * Unregisters the given player.
+	 * @param player - player to unregister.
+	 * @param removeAuxiliary - TRUE to remove auxiliary information, such as input stream and address.
+	 * @return TRUE if a player has been uninjected, FALSE otherwise.
+	 */
+	public boolean uninjectPlayer(Player player, boolean removeAuxiliary) {
+		return uninjectPlayer(player, removeAuxiliary, false);
+	}
+	
+	/**
+	 * Unregisters the given player.
+	 * @param player - player to unregister.
+	 * @param removeAuxiliary - TRUE to remove auxiliary information, such as input stream and address.
+	 * @param prepareNextHook - whether or not we need to fix any lingering hooks.
+	 * @return TRUE if a player has been uninjected, FALSE otherwise.
+	 */
+	private boolean uninjectPlayer(Player player, boolean removeAuxiliary, boolean prepareNextHook) {
 		if (!hasClosed && player != null) {
 			
 			PlayerInjector injector = playerInjection.remove(player);
@@ -373,26 +394,54 @@ public class PlayerInjectionHandler {
 				InetSocketAddress address = player.getAddress();
 				injector.cleanupAll();
 				
-				dataInputLookup.remove(input);
+				// Remove the "hooked" network manager in our instance as well
+				if (prepareNextHook && injector instanceof NetworkObjectInjector) {
+					try {
+						PlayerInjector dummyInjector = getHookInstance(player, PlayerInjectHooks.NETWORK_SERVER_OBJECT);
+						dummyInjector.initializePlayer(player);
+						dummyInjector.setNetworkManager(injector.getNetworkManager(), true);
 
-				if (address != null)
+					} catch (IllegalAccessException e) {
-					addressLookup.remove(address);
+						// Let the user know
+						logger.log(Level.WARNING, "Unable to fully revert old injector. May cause conflicts.", e);
+					}
+				}
+				
+				// Clean up
+				if (removeAuxiliary) {
+					if (input != null) 
+						dataInputLookup.remove(input);
+					if (address != null) 
+						addressLookup.remove(address);
+				}
+				return true;
 			}
 		}
+		
+		return false;
 	}
 	
 	/**
 	 * Unregisters a player by the given address.
+	 * <p>
+	 * If the server handler has been created before we've gotten a chance to unject the player,
+	 * the method will try a workaround to remove the injected hook in the NetServerHandler.
+	 * 
 	 * @param address - address of the player to unregister.
+	 * @param serverHandler - whether or not the net server handler has already been created.
+	 * @return TRUE if a player has been uninjected, FALSE otherwise.
 	 */
-	public void uninjectPlayer(InetSocketAddress address) {
+	public boolean uninjectPlayer(InetSocketAddress address) {
 		if (!hasClosed && address != null) {
 			PlayerInjector injector = addressLookup.get(address);
 			
 			// Clean up
 			if (injector != null)
-				uninjectPlayer(injector.getPlayer());
+				uninjectPlayer(injector.getPlayer(), false, true);
+			return true;
 		}
+		
+		return false;
 	}
 	
 	/**
@@ -445,6 +494,26 @@ public class PlayerInjectionHandler {
 		return playerInjection.get(player);
 	}
 	
+	/**
+	 * Retrieve a player injector by looking for its NetworkManager.
+	 * @param networkManager - current network manager.
+	 * @return Related player injector.
+	 */
+	PlayerInjector getInjectorByNetworkHandler(Object networkManager) {
+		// That's not legal
+		if (networkManager == null)
+			return null;
+		
+		// O(n) is okay in this instance. This is only a backup solution.
+		for (PlayerInjector injector : playerInjection.values()) {
+			if (injector.getNetworkManager() == networkManager)
+				return injector;
+		}
+		
+		// None found
+		return null;
+	}
+	
 	/**
 	 * Determine if the given listeners are valid.
 	 * @param listeners - listeners to check.

ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjector.java

@@ -23,6 +23,7 @@ import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.Socket;
+import java.net.SocketAddress;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -104,6 +105,9 @@ abstract class PlayerInjector {
 	// Scheduled action on the next packet event
 	protected Runnable scheduledAction;
 
+	// Whether or not the injector has been cleaned
+	private boolean clean;
+	
 	// Whether or not to update the current player on the first Packet1Login
 	boolean updateOnLogin;
 	Player updatedPlayer;
@@ -256,6 +260,21 @@ abstract class PlayerInjector {
 		}
 	}
 	
+	/**
+	 * Retrieve the associated address of this player.
+	 * @return The associated address.
+	 * @throws IllegalAccessException If we're unable to read the socket field.
+	 */
+	public SocketAddress getAddress() throws IllegalAccessException {
+		Socket socket = getSocket();
+		
+		// Guard against NULL
+		if (socket != null)
+			return socket.getRemoteSocketAddress();
+		else
+			return null;
+	}
+	
 	/**
 	 * Attempt to disconnect the current client.
 	 * @param message - the message to display.
@@ -427,7 +446,24 @@ abstract class PlayerInjector {
 	/**
 	 * Remove all hooks and modifications.
 	 */
-	public abstract void cleanupAll();
+	public final void cleanupAll() {
+		if (!clean)
+			cleanHook();
+		clean = true;
+	}
+	
+	/**
+	 * Override to add custom cleanup behavior.
+	 */
+	protected abstract void cleanHook();
+	
+	/**
+	 * Determine whether or not this hook has already been cleaned.
+	 * @return TRUE if it has, FALSE otherwise.
+	 */
+	public boolean isClean() {
+		return clean;
+	}
 	
 	/**
 	 * Determine if this inject method can even be attempted.

ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java

@@ -76,13 +76,16 @@ class TemporaryPlayerFactory {
 				String methodName = method.getName();
 				PlayerInjector injector = ((InjectContainer) obj).getInjector();
 				
+				if (injector == null)
+					throw new IllegalStateException("Unable to find injector.");
+				
 				// Use the socket to get the address
 				if (methodName.equalsIgnoreCase("getName"))
 					return "UNKNOWN[" + injector.getSocket().getRemoteSocketAddress() + "]";
 				if (methodName.equalsIgnoreCase("getPlayer"))
 					return injector.getUpdatedPlayer();
 				if (methodName.equalsIgnoreCase("getAddress")) 
-					return injector.getSocket().getRemoteSocketAddress();
+					return injector.getAddress();
 				if (methodName.equalsIgnoreCase("getServer"))
 					return server;
 				

ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java

@@ -206,7 +206,6 @@ public class DefaultInstances {
 		// Just check if any of them are NULL
 		for (Class<?> type : types) {
 			if (getDefaultInternal(type, providers, recursionLevel) == null) {
-				System.out.println(type.getName() + " is NULL!");
 				return true;
 			}
 		}