Bug 129525 "Don't Dump Bytecodes to Syserr in LTW" (don't throw AbortException in LTW message handler)

9 files changed, 75 insertions, 9 deletions

diff --git a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
index 2e53d0cfa..7e2edb5c0 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
@@ -355,6 +355,8 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
      */
     private void registerAspects(final BcelWeaver weaver, final ClassLoader loader, final List definitions) {
     	if (trace.isTraceEnabled()) trace.enter("registerAspects",this, new Object[] { weaver, loader, definitions} );
+    	boolean success = true;
+    	
         //TODO: the exclude aspect allow to exclude aspect defined upper in the CL hierarchy - is it what we want ??
         // if not, review the getResource so that we track which resource is defined by which CL
 
@@ -393,6 +395,7 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
                     ConcreteAspectCodeGen gen = new ConcreteAspectCodeGen(concreteAspect, weaver.getWorld());
                     if (!gen.validate()) {
                         error("Concrete-aspect '"+concreteAspect.name+"' could not be registered");
+                        success = false;
                         break;
                     }
                     this.generatedClassHandler.acceptClass(
@@ -412,8 +415,14 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
         }
 //        System.out.println("ClassLoaderWeavingAdaptor.registerAspects() classloader=" + weavingContext.getClassLoaderName() + ", namespace=" + namespace);
         
+        /* We couldn't register one or more aspects so disable the adaptor */
+        if (!success) {
+        	disable();
+    		warn("failure(s) registering aspects. Disabling weaver for class loader " + getClassLoaderName(loader));
+        }
+        
         /* We didn't register any aspects so disable the adaptor */
-        if (namespace == null) {
+        else if (namespace == null) {
         	disable();
     		info("no aspects registered. Disabling weaver for class loader " + getClassLoaderName(loader));
         }
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java b/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java
index 0dc1506df..fca4a23a5 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java
@@ -24,7 +24,10 @@ import java.util.List;
 import java.util.Map;
 import java.util.StringTokenizer;
 
+import org.aspectj.bridge.AbortException;
 import org.aspectj.weaver.ExtensibleURLClassLoader;
+import org.aspectj.weaver.tools.Trace;
+import org.aspectj.weaver.tools.TraceFactory;
 import org.aspectj.weaver.tools.WeavingAdaptor;
 import org.aspectj.weaver.tools.WeavingClassLoader;
 
@@ -37,6 +40,8 @@ public class WeavingURLClassLoader extends ExtensibleURLClassLoader implements W
 	private WeavingAdaptor adaptor;
 	private boolean initializingAdaptor;
 	private Map generatedClasses = new HashMap(); /* String -> byte[] */ 
+	
+	private static Trace trace = TraceFactory.getTraceFactory().getTrace(WeavingURLClassLoader.class);
 
 	/*
 	 * This constructor is needed when using "-Djava.system.class.loader". 
@@ -48,7 +53,9 @@ public class WeavingURLClassLoader extends ExtensibleURLClassLoader implements W
 	
 	public WeavingURLClassLoader (URL[] urls, ClassLoader parent) {
 		super(urls,parent);
+		if (trace.isTraceEnabled()) trace.enter("<init>",this,new Object[] { urls, parent });
 //		System.out.println("WeavingURLClassLoader.WeavingURLClassLoader()");
+		if (trace.isTraceEnabled()) trace.exit("<init>");
 	}
 	
 	public WeavingURLClassLoader (URL[] classURLs, URL[] aspectURLs, ClassLoader parent) {
@@ -106,6 +113,7 @@ public class WeavingURLClassLoader extends ExtensibleURLClassLoader implements W
 	 * Override to weave class using WeavingAdaptor 
 	 */
 	protected Class defineClass(String name, byte[] b, CodeSource cs) throws IOException {
+		if (trace.isTraceEnabled()) trace.enter("defineClass",this,new Object[] { name, b, cs });
 //		System.err.println("? WeavingURLClassLoader.defineClass(" + name + ", [" + b.length + "])");
 
 		/* Avoid recursion during adaptor initialization */
@@ -116,9 +124,20 @@ public class WeavingURLClassLoader extends ExtensibleURLClassLoader implements W
 				createAdaptor();
 			}
 			
-			b = adaptor.weaveClass(name,b);
+			try {
+				b = adaptor.weaveClass(name,b);
+			}
+			catch (AbortException ex) {
+	    		trace.error("defineClass",ex);
+	    		throw ex;
+			}
+			catch (Throwable th) {
+	    		trace.error("defineClass",th);
+			}
 		}
-		return super.defineClass(name, b, cs);
+		Class clazz = super.defineClass(name, b, cs);
+		if (trace.isTraceEnabled()) trace.exit("defineClass",clazz);
+		return clazz;
 	}
 	
 	private void createAdaptor () {
diff --git a/tests/ltw/aop-missingaspect.xml b/tests/ltw/aop-missingaspect.xml
new file mode 100644
index 000000000..27235c38b
--- /dev/null
+++ b/tests/ltw/aop-missingaspect.xml
@@ -0,0 +1,5 @@
+<aspectj>
+	<aspects>
+		<aspect name="EmptyAspect"/>
+	</aspects>
+</aspectj>
\ No newline at end of file
diff --git a/tests/ltw/pakkage/EmptyAspect.aj b/tests/ltw/pakkage/EmptyAspect.aj
new file mode 100644
index 000000000..e24bad3e8
--- /dev/null
+++ b/tests/ltw/pakkage/EmptyAspect.aj
@@ -0,0 +1,3 @@
+public aspect EmptyAspect {
+	
+}
\ No newline at end of file
diff --git a/tests/src/org/aspectj/systemtest/ajc152/ajc152.xml b/tests/src/org/aspectj/systemtest/ajc152/ajc152.xml
index 812d15830..3e7b08082 100644
--- a/tests/src/org/aspectj/systemtest/ajc152/ajc152.xml
+++ b/tests/src/org/aspectj/systemtest/ajc152/ajc152.xml
@@ -483,7 +483,8 @@
         <run class="HelloWorld" ltw="aop-tracing.xml">
           <stderr>
             <line text="error Abstract method 'void ConcreteMethods.foo(int)' cannot be concretized in XML:"/>
-            <line text="warning register definition failed"/>
+            <line text="error Concrete-aspect 'TraceHelloWorld' could not be registered"/>
+            <line text="warning failure(s) registering aspects. Disabling weaver for class loader"/>
           </stderr>
           <stdout>
             <line text="Hello World!"/>
@@ -497,7 +498,8 @@
         <run class="HelloWorld" ltw="aop-tracing.xml">
           <stderr>
             <line text="error Abstract method 'void AbstractMethods.test()' cannot be concretized in XML"/>
-            <line text="warning register definition failed"/>
+            <line text="error Concrete-aspect 'TraceHelloWorld' could not be registered"/>
+            <line text="warning failure(s) registering aspects. Disabling weaver for class loader"/>
           </stderr>
           <stdout>
             <line text="Hello World!"/>
diff --git a/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java b/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java
index b366d6798..9ae265bd1 100644
--- a/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java
+++ b/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java
@@ -66,6 +66,10 @@ public class Ajc153Tests extends org.aspectj.testing.XMLBasedAjcTestCase {
   public void testDuplicateJVMTIAgents_pr151938() {runTest("Duplicate JVMTI agents");};
   public void testLTWWorldWithAnnotationMatching_pr153572() { runTest("LTWWorld with annotation matching");}
   
+  public void testReweavableAspectNotRegistered_pr129525 () {
+	  runTest("reweavableAspectNotRegistered error");
+  }
+  
   /////////////////////////////////////////
   public static Test suite() {
     return XMLBasedAjcTestCase.loadSuite(Ajc153Tests.class);
diff --git a/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml b/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml
index f7586c342..b1910ee63 100644
--- a/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml
+++ b/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml
@@ -333,4 +333,20 @@
       </run>
     </ajc-test>
     
+    <ajc-test dir="ltw" title="reweavableAspectNotRegistered error">
+      <compile files="HelloWorld.java, ExceptionHandler.aj" options="-showWeaveInfo">
+        <message kind="weave" text="Join point 'method-execution(void HelloWorld.main(java.lang.String[]))' in Type 'HelloWorld' (HelloWorld.java:3) advised by around advice from 'ExceptionHandler' (ExceptionHandler.aj:2)"/>
+      </compile>
+      <compile files="EmptyAspect.aj">
+      </compile>
+      <run class="HelloWorld" ltw="aop-missingaspect.xml">
+        <stdout>
+          <line text="Hello World!"/>
+        </stdout>
+        <stderr>
+        	<line text="error aspect 'ExceptionHandler' woven into 'HelloWorld' must be defined to the weaver (placed on the aspectpath, or defined in an aop.xml file if using LTW)."/>
+        </stderr>
+      </run>
+    </ajc-test>
+    
 </suite>
\ No newline at end of file
diff --git a/tests/src/org/aspectj/systemtest/tracing/tracing.xml b/tests/src/org/aspectj/systemtest/tracing/tracing.xml
index 6a5d4881e..bed65f12d 100644
--- a/tests/src/org/aspectj/systemtest/tracing/tracing.xml
+++ b/tests/src/org/aspectj/systemtest/tracing/tracing.xml
@@ -52,8 +52,10 @@
 	  	    	<line text="warning aspect ExcludedAspect exluded"/>
 	  	    	<line text="error Cannot find m_parent aspect"/>
 	  	    	<line text="error Cannot find m_parent aspect"/>
-	  	    	<line text="warning register definition failed"/>
-	  	    	<line text="warning register definition failed"/>
+	  	    	<line text="error Concrete-aspect 'IncludedMissingAspect' could not be registered"/>
+	  	    	<line text="error Concrete-aspect 'IncludedMissingAspect' could not be registered"/>
+	  	    	<line text="warning failure(s) registering aspects. Disabling weaver for class loader"/>
+	  	    	<line text="warning failure(s) registering aspects. Disabling weaver for class loader"/>
             </stderr>
         </ant>
     </ajc-test>
diff --git a/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java b/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
index b1adcfb8f..26244e2a0 100644
--- a/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
+++ b/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
@@ -80,6 +80,7 @@ public class WeavingAdaptor implements IMessageContext {
 	protected BcelWeaver weaver;
 	private IMessageHandler messageHandler;
 	private WeavingAdaptorMessageHandler messageHolder;
+	private boolean abortOnError = false;
 	protected GeneratedClassHandler generatedClassHandler;
 	protected Map generatedClasses = new HashMap(); /* String -> UnwovenClassFile */
 	protected BcelObjectType delegateForCurrentClass; // lazily initialized, should be used to prevent parsing bytecode multiple times
@@ -153,6 +154,7 @@ public class WeavingAdaptor implements IMessageContext {
 	}
 	
 	private void init(List classPath, List aspectPath) {
+		abortOnError = true;
 		createMessageHandler();
 		
 		info("using classpath: " + classPath); 
@@ -191,8 +193,12 @@ public class WeavingAdaptor implements IMessageContext {
 	}
 	
 	protected void disable () {
+		if (trace.isTraceEnabled()) trace.enter("disable",this);
+
 		enabled = false;
 		messageHolder.flushMessages();
+		
+		if (trace.isTraceEnabled()) trace.exit("disable");
 	}
 	
 	protected boolean isEnabled () {
@@ -469,7 +475,7 @@ public class WeavingAdaptor implements IMessageContext {
 			if (traceMessages) traceMessage(message);
 			if (accumulating) {
 				boolean result = addMessage(message);
-				if (0 <= message.getKind().compareTo(IMessage.ERROR)) {
+				if (abortOnError && 0 <= message.getKind().compareTo(IMessage.ERROR)) {
 					throw new AbortException(message);
 				}
 				return result;
@@ -553,7 +559,7 @@ public class WeavingAdaptor implements IMessageContext {
 
 		public boolean handleMessage(IMessage message) throws AbortException {
 			boolean result = super.handleMessage(message);
-			if (0 <= message.getKind().compareTo(failKind)) {
+			if (abortOnError && 0 <= message.getKind().compareTo(failKind)) {
 				throw new AbortException(message);
 			}
 			return true;