Fix 488216: Load-time weaver loses class changes of preceding -javaagent

5 files changed, 137 insertions, 131 deletions

diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/tools/AbstractTrace.java b/org.aspectj.matcher/src/org/aspectj/weaver/tools/AbstractTrace.java
index 2ec0aa3a9..c736de886 100644
--- a/org.aspectj.matcher/src/org/aspectj/weaver/tools/AbstractTrace.java
+++ b/org.aspectj.matcher/src/org/aspectj/weaver/tools/AbstractTrace.java
@@ -16,12 +16,15 @@ import java.net.URL;
 import java.text.SimpleDateFormat;
 import java.util.Collection;
 import java.util.Date;
+import java.util.regex.Pattern;
 
 import org.aspectj.bridge.IMessage.Kind;
 
 public abstract class AbstractTrace implements Trace {
 
-	protected Class tracedClass;
+	private static final Pattern packagePrefixPattern = Pattern.compile("([^.])[^.]*(\\.)");
+
+	protected Class<?> tracedClass;
 
 	private static SimpleDateFormat timeFormat;
 	
@@ -85,13 +88,21 @@ public abstract class AbstractTrace implements Trace {
 		message.append(formatDate(now)).append(" ");
 		message.append(Thread.currentThread().getName()).append(" ");
 		message.append(kind).append(" ");
-		message.append(className);
+		message.append(formatClassName(className));
 		message.append(".").append(methodName);
 		if (thiz != null) message.append(" ").append(formatObj(thiz));
 		if (args != null) message.append(" ").append(formatArgs(args));
 		return message.toString();
 	}
 	
+	/**
+	 * @param className full dotted class name
+	 * @return short version of class name with package collapse to initials
+	 */
+	private String formatClassName(String className) {
+		return packagePrefixPattern.matcher(className).replaceAll("$1.");
+	}
+	
 	protected String formatMessage(String kind, String text, Throwable th) {
 		StringBuffer message = new StringBuffer();
 		Date now = new Date();
@@ -116,7 +127,7 @@ public abstract class AbstractTrace implements Trace {
 	 * NullPointerExceptions or highly verbose results.
 	 *  
 	 * @param obj parameter to be formatted
-	 * @return the formated parameter
+	 * @return the formatted parameter
 	 */
 	protected Object formatObj(Object obj) {
 		
@@ -141,14 +152,13 @@ public abstract class AbstractTrace implements Trace {
 		}
 		else try {
 			
-			/* Classes can provide an alternative implementation of toString() */
+			// Classes can provide an alternative implementation of toString()
 			if (obj instanceof Traceable) {
-				Traceable t = (Traceable)obj;
-				return t.toTraceString();
+				return ((Traceable)obj).toTraceString();
 			}
 			
-			/* Use classname@hashcode */
-			else return obj.getClass().getName() + "@" + Integer.toHexString(System.identityHashCode(obj));
+			// classname@hashcode
+			else return formatClassName(obj.getClass().getName()) + "@" + Integer.toHexString(System.identityHashCode(obj));
 		
 		/* Object.hashCode() can be override and may thow an exception */	
 		} catch (Exception ex) {
diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/tools/DefaultTrace.java b/org.aspectj.matcher/src/org/aspectj/weaver/tools/DefaultTrace.java
index ad95bbb3e..7e2ab0d8f 100644
--- a/org.aspectj.matcher/src/org/aspectj/weaver/tools/DefaultTrace.java
+++ b/org.aspectj.matcher/src/org/aspectj/weaver/tools/DefaultTrace.java
@@ -12,109 +12,112 @@ package org.aspectj.weaver.tools;
 
 import java.io.PrintStream;
 
-
 public class DefaultTrace extends AbstractTrace {
-	
+
 	private boolean traceEnabled = false;
 	private PrintStream print = System.err;
-	
-	public DefaultTrace (Class clazz) {
+
+	public DefaultTrace(Class clazz) {
 		super(clazz);
 	}
-	
-	public boolean isTraceEnabled () {
+
+	public boolean isTraceEnabled() {
 		return traceEnabled;
 	}
-	
-	public void setTraceEnabled (boolean b) {
+
+	public void setTraceEnabled(boolean b) {
 		traceEnabled = b;
 	}
-	
-	public void enter (String methodName, Object thiz, Object[] args) {
+
+	public void enter(String methodName, Object thiz, Object[] args) {
 		if (traceEnabled) {
-			println(formatMessage(">",tracedClass.getName(),methodName,thiz, args));
+			println(formatMessage(">", tracedClass.getName(), methodName, thiz, args));
 		}
 	}
-	
-	public void enter (String methodName, Object thiz) {
+
+	public void enter(String methodName, Object thiz) {
 		if (traceEnabled) {
-			println(formatMessage(">",tracedClass.getName(),methodName,thiz, null));
+			println(formatMessage(">", tracedClass.getName(), methodName, thiz, null));
 		}
 	}
 
-	public void exit (String methodName, Object ret) {
+	public void exit(String methodName, Object ret) {
 		if (traceEnabled) {
-			println(formatMessage("<",tracedClass.getName(),methodName,ret, null));
+			println(formatMessage("<", tracedClass.getName(), methodName, ret, null));
 		}
 	}
 
-	public void exit (String methodName) {
+	public void exit(String methodName) {
 		if (traceEnabled) {
-			println(formatMessage("<",tracedClass.getName(),methodName,null, null));
+			println(formatMessage("<", tracedClass.getName(), methodName, null, null));
 		}
 	}
 
 	public void exit(String methodName, Throwable th) {
 		if (traceEnabled) {
-			println(formatMessage("<",tracedClass.getName(),methodName,th, null));
+			println(formatMessage("<", tracedClass.getName(), methodName, th, null));
 		}
 	}
-	
+
 	public void event(String methodName, Object thiz, Object[] args) {
 		if (traceEnabled) {
-			println(formatMessage("-",tracedClass.getName(),methodName,thiz, args));
+			println(formatMessage("-", tracedClass.getName(), methodName, thiz, args));
 		}
 	}
 
 	public void event(String methodName) {
 		if (traceEnabled) {
-			println(formatMessage("-",tracedClass.getName(),methodName,null,null));
+			println(formatMessage("-", tracedClass.getName(), methodName, null, null));
 		}
 	}
 
-	public void debug (String message) {
-		println(formatMessage("?",message,null));
+	public void debug(String message) {
+		println(formatMessage("?", message, null));
 	}
 
 	public void info(String message) {
-		println(formatMessage("I",message,null));
+		println(formatMessage("I", message, null));
 	}
 
-	public void warn (String message, Throwable th) {
-		println(formatMessage("W",message,th));
-		if (th != null) th.printStackTrace();
+	public void warn(String message, Throwable th) {
+		println(formatMessage("W", message, th));
+		if (th != null)
+			th.printStackTrace();
 	}
 
-
-	public void error (String message, Throwable th) {
-		println(formatMessage("E",message,th));
-		if (th != null) th.printStackTrace();
+	public void error(String message, Throwable th) {
+		println(formatMessage("E", message, th));
+		if (th != null)
+			th.printStackTrace();
 	}
 
-	public void fatal (String message, Throwable th) {
-		println(formatMessage("X",message,th));
-		if (th != null) th.printStackTrace();
+	public void fatal(String message, Throwable th) {
+		println(formatMessage("X", message, th));
+		if (th != null)
+			th.printStackTrace();
 	}
 
 	/**
 	 * Template method that allows choice of destination for output
 	 * 
-	 * @param s message to be traced
+	 * @param s
+	 *            message to be traced
 	 */
-	protected void println (String s) {
+	protected void println(String s) {
 		print.println(s);
 	}
 
-	public void setPrintStream (PrintStream printStream) {
+	public void setPrintStream(PrintStream printStream) {
 		this.print = printStream;
 	}
 
-//	private static boolean isTracingEnabled = getBoolean("org.aspectj.weaver.tools.tracing",false);
-//
-//	private static boolean getBoolean (String name, boolean def) {
-//		String defaultValue = String.valueOf(def);
-//		String value = System.getProperty(name,defaultValue);
-//		return Boolean.valueOf(value).booleanValue();
-//	}
+	// private static boolean isTracingEnabled =
+	// getBoolean("org.aspectj.weaver.tools.tracing",false);
+	//
+	// private static boolean getBoolean (String name, boolean def) {
+	// String defaultValue = String.valueOf(def);
+	// String value = System.getProperty(name,defaultValue);
+	// return Boolean.valueOf(value).booleanValue();
+	// }
 
 }
diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/tools/Trace.java b/org.aspectj.matcher/src/org/aspectj/weaver/tools/Trace.java
index c1a040d36..8b20032aa 100644
--- a/org.aspectj.matcher/src/org/aspectj/weaver/tools/Trace.java
+++ b/org.aspectj.matcher/src/org/aspectj/weaver/tools/Trace.java
@@ -12,51 +12,47 @@ package org.aspectj.weaver.tools;
 
 public interface Trace {
 
-	public void enter (String methodName, Object thiz, Object[] args);
+	public void enter(String methodName, Object thiz, Object[] args);
 
-	public void enter (String methodName, Object thiz);
+	public void enter(String methodName, Object thiz);
 
-	public void exit (String methodName, Object ret);
+	public void exit(String methodName, Object ret);
 
-	public void exit (String methodName, Throwable th);
+	public void exit(String methodName, Throwable th);
 
-	public void exit (String methodName);
+	public void exit(String methodName);
 
-	public void event (String methodName);
+	public void event(String methodName);
 
-	public void event (String methodName, Object thiz, Object[] args);
-	
-	public void debug (String message);
-	
-	public void info (String message);
+	public void event(String methodName, Object thiz, Object[] args);
 
-	public void warn (String message);
+	public void debug(String message);
 
-	public void warn (String message, Throwable th);
+	public void info(String message);
 
-	public void error (String message);
+	public void warn(String message);
 
-	public void error (String message, Throwable th);
+	public void warn(String message, Throwable th);
 
-	public void fatal (String message);
+	public void error(String message);
 
-	public void fatal (String message, Throwable th);
-	
-	
-	/*
-	 * Convenience methods
-	 */
-	public void enter (String methodName, Object thiz, Object arg);
+	public void error(String message, Throwable th);
 
-	public void enter (String methodName, Object thiz, boolean z);
+	public void fatal(String message);
 
-	public void exit (String methodName, boolean b);
+	public void fatal(String message, Throwable th);
 
-	public void exit (String methodName, int i);
+	public void enter(String methodName, Object thiz, Object arg);
 
-	public void event (String methodName, Object thiz, Object arg);
-	
-	public boolean isTraceEnabled ();
+	public void enter(String methodName, Object thiz, boolean z);
 
-	public void setTraceEnabled (boolean b);
+	public void exit(String methodName, boolean b);
+
+	public void exit(String methodName, int i);
+
+	public void event(String methodName, Object thiz, Object arg);
+
+	public boolean isTraceEnabled();
+
+	public void setTraceEnabled(boolean b);
 }
\ No newline at end of file
diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelObjectType.java b/weaver/src/org/aspectj/weaver/bcel/BcelObjectType.java
index 584a14c8d..710eb6dc7 100644
--- a/weaver/src/org/aspectj/weaver/bcel/BcelObjectType.java
+++ b/weaver/src/org/aspectj/weaver/bcel/BcelObjectType.java
@@ -484,6 +484,9 @@ public class BcelObjectType extends AbstractReferenceTypeDelegate {
 		return javaClass;
 	}
 
+	/**
+	 * @return true if built from bytes obtained from somewhere. False if built from bytes retrieved from disk.
+	 */
 	public boolean isArtificial() {
 		return artificial;
 	}
diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelWorld.java b/weaver/src/org/aspectj/weaver/bcel/BcelWorld.java
index de9e29aba..395e07e10 100644
--- a/weaver/src/org/aspectj/weaver/bcel/BcelWorld.java
+++ b/weaver/src/org/aspectj/weaver/bcel/BcelWorld.java
@@ -434,12 +434,13 @@ public class BcelWorld extends World implements Repository {
 			if (file == null) {
 				return null;
 			}
-
 			ClassParser parser = new ClassParser(file.getInputStream(), file.getPath());
-
 			JavaClass jc = parser.parse();
 			return jc;
 		} catch (IOException ioe) {
+			if (trace.isTraceEnabled()) {
+				trace.error("IOException whilst processing class",ioe);
+			}
 			return null;
 		} finally {
 			if (file != null) {
@@ -448,10 +449,6 @@ public class BcelWorld extends World implements Repository {
 		}
 	}
 
-	// public BcelObjectType addSourceObjectType(JavaClass jc) {
-	// return addSourceObjectType(jc.getClassName(), jc, -1);
-	// }
-
 	public BcelObjectType addSourceObjectType(JavaClass jc, boolean artificial) {
 		return addSourceObjectType(jc.getClassName(), jc, artificial);
 	}
@@ -463,19 +460,19 @@ public class BcelWorld extends World implements Repository {
 		}
 		String signature = UnresolvedType.forName(jc.getClassName()).getSignature();
 
-		ResolvedType fromTheMap = typeMap.get(signature);
+		ResolvedType resolvedTypeFromTypeMap = typeMap.get(signature);
 
-		if (fromTheMap != null && !(fromTheMap instanceof ReferenceType)) {
+		if (resolvedTypeFromTypeMap != null && !(resolvedTypeFromTypeMap instanceof ReferenceType)) {
 			// what on earth is it then? See pr 112243
 			StringBuffer exceptionText = new StringBuffer();
 			exceptionText.append("Found invalid (not a ReferenceType) entry in the type map. ");
-			exceptionText.append("Signature=[" + signature + "] Found=[" + fromTheMap + "] Class=[" + fromTheMap.getClass() + "]");
+			exceptionText.append("Signature=[" + signature + "] Found=[" + resolvedTypeFromTypeMap + "] Class=[" + resolvedTypeFromTypeMap.getClass() + "]");
 			throw new BCException(exceptionText.toString());
 		}
 
-		ReferenceType nameTypeX = (ReferenceType) fromTheMap;
+		ReferenceType referenceTypeFromTypeMap = (ReferenceType) resolvedTypeFromTypeMap;
 
-		if (nameTypeX == null) {
+		if (referenceTypeFromTypeMap == null) {
 			if (jc.isGeneric() && isInJava5Mode()) {
 				ReferenceType rawType = ReferenceType.fromTypeX(UnresolvedType.forRawTypeName(jc.getClassName()), this);
 				ret = buildBcelDelegate(rawType, jc, artificial, true);
@@ -486,67 +483,64 @@ public class BcelWorld extends World implements Repository {
 				rawType.setGenericType(genericRefType);
 				typeMap.put(signature, rawType);
 			} else {
-				nameTypeX = new ReferenceType(signature, this);
-				ret = buildBcelDelegate(nameTypeX, jc, artificial, true);
-				typeMap.put(signature, nameTypeX);
+				referenceTypeFromTypeMap = new ReferenceType(signature, this);
+				ret = buildBcelDelegate(referenceTypeFromTypeMap, jc, artificial, true);
+				typeMap.put(signature, referenceTypeFromTypeMap);
 			}
 		} else {
-			ret = buildBcelDelegate(nameTypeX, jc, artificial, true);
+			ret = buildBcelDelegate(referenceTypeFromTypeMap, jc, artificial, true);
 		}
 		return ret;
 	}
 
 	public BcelObjectType addSourceObjectType(String classname, byte[] bytes, boolean artificial) {
-		BcelObjectType ret = null;
+		BcelObjectType retval = null;
 		String signature = UnresolvedType.forName(classname).getSignature();
-		ResolvedType fromTheMap = typeMap.get(signature);
+		ResolvedType resolvedTypeFromTypeMap = typeMap.get(signature);
 
-		if (fromTheMap != null && !(fromTheMap instanceof ReferenceType)) {
+		if (resolvedTypeFromTypeMap != null && !(resolvedTypeFromTypeMap instanceof ReferenceType)) {
 			// what on earth is it then? See pr 112243
 			StringBuffer exceptionText = new StringBuffer();
 			exceptionText.append("Found invalid (not a ReferenceType) entry in the type map. ");
-			exceptionText.append("Signature=[" + signature + "] Found=[" + fromTheMap + "] Class=[" + fromTheMap.getClass() + "]");
+			exceptionText.append("Signature=[" + signature + "] Found=[" + resolvedTypeFromTypeMap + "] Class=[" + resolvedTypeFromTypeMap.getClass() + "]");
 			throw new BCException(exceptionText.toString());
 		}
 
-		ReferenceType nameTypeX = (ReferenceType) fromTheMap;
+		ReferenceType referenceTypeFromTypeMap = (ReferenceType) resolvedTypeFromTypeMap;
 
-		if (nameTypeX == null) {
+		if (referenceTypeFromTypeMap == null) {
 			JavaClass jc = Utility.makeJavaClass(classname, bytes);
 			if (jc.isGeneric() && isInJava5Mode()) {
-				nameTypeX = ReferenceType.fromTypeX(UnresolvedType.forRawTypeName(jc.getClassName()), this);
-				ret = buildBcelDelegate(nameTypeX, jc, artificial, true);
+				referenceTypeFromTypeMap = ReferenceType.fromTypeX(UnresolvedType.forRawTypeName(jc.getClassName()), this);
+				retval = buildBcelDelegate(referenceTypeFromTypeMap, jc, artificial, true);
 				ReferenceType genericRefType = new ReferenceType(UnresolvedType.forGenericTypeSignature(signature,
-						ret.getDeclaredGenericSignature()), this);
-				nameTypeX.setDelegate(ret);
-				genericRefType.setDelegate(ret);
-				nameTypeX.setGenericType(genericRefType);
-				typeMap.put(signature, nameTypeX);
+						retval.getDeclaredGenericSignature()), this);
+				referenceTypeFromTypeMap.setDelegate(retval);
+				genericRefType.setDelegate(retval);
+				referenceTypeFromTypeMap.setGenericType(genericRefType);
+				typeMap.put(signature, referenceTypeFromTypeMap);
 			} else {
-				nameTypeX = new ReferenceType(signature, this);
-				ret = buildBcelDelegate(nameTypeX, jc, artificial, true);
-				typeMap.put(signature, nameTypeX);
+				referenceTypeFromTypeMap = new ReferenceType(signature, this);
+				retval = buildBcelDelegate(referenceTypeFromTypeMap, jc, artificial, true);
+				typeMap.put(signature, referenceTypeFromTypeMap);
 			}
 		} else {
-			Object o = nameTypeX.getDelegate();
-			if (!(o instanceof BcelObjectType)) {
-				throw new IllegalStateException("For " + classname + " should be BcelObjectType, but is " + o.getClass());
-			}
-			ret = (BcelObjectType) o;
-			// byte[] bs = ret.javaClass.getBytes();
-			// if (bs.length != bytes.length) {
-			// throw new RuntimeException("");
-			// }
-			// If the type is already exposed to the weaver (ret.isExposedToWeaver()) then this is likely
-			// to be a hotswap reweave so build a new delegate, dont accidentally use the old data
-			if (ret.isArtificial() || ret.isExposedToWeaver()) {
-				// System.out.println("Rebuilding " + nameTypeX.getName());
-				ret = buildBcelDelegate(nameTypeX, Utility.makeJavaClass(classname, bytes), artificial, true);
-			} else {
-				ret.setExposedToWeaver(true);
+			ReferenceTypeDelegate existingDelegate = referenceTypeFromTypeMap.getDelegate();
+			if (!(existingDelegate instanceof BcelObjectType)) {
+				throw new IllegalStateException("For " + classname + " should be BcelObjectType, but is " + existingDelegate.getClass());
 			}
+			retval = (BcelObjectType) existingDelegate;
+			// Note1: If the type is already exposed to the weaver (retval.isExposedToWeaver()) then this is likely
+			// to be a hotswap reweave so build a new delegate, don't accidentally use the old data.
+			// Note2: Also seen when LTW and another agent precedes the AspectJ agent.  Earlier in LTW
+			// a type is resolved (and ends up in the typemap but not exposed to the weaver at that time)
+			// then later LTW actually is attempted on this type. We end up here with different
+			// bytes to the current delegate if the earlier agent has modified them. See PR488216
+//			if (retval.isArtificial() || retval.isExposedToWeaver()) {
+			retval = buildBcelDelegate(referenceTypeFromTypeMap, Utility.makeJavaClass(classname, bytes), artificial, true);
+//			}
 		}
-		return ret;
+		return retval;
 	}
 
 	void deleteSourceObjectType(UnresolvedType ty) {