Bug 158957 "NPE in LTW with RMI dynamic proxies w/ pointcut reuse" (restore catch(Throwable) in Aj.preProcess() and make adaptor disabled by default)

11 files changed, 164 insertions, 33 deletions

diff --git a/loadtime/src/org/aspectj/weaver/loadtime/Aj.java b/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
index 2f775a0b1..e4155d833 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
@@ -78,12 +78,13 @@ public class Aj implements ClassPreProcessor {
         		if (trace.isTraceEnabled()) trace.exit("preProcess",newBytes);
                 return newBytes;
 			}
-        } catch (Exception ex) {
-    		trace.error("preProcess",ex);
+        
+        /* Don't like to do this but JVMTI swallows all exceptions */
+        } catch (Throwable th) {
+    		trace.error("preProcess",th);
             //FIXME AV wondering if we should have the option to fail (throw runtime exception) here
             // would make sense at least in test f.e. see TestHelper.handleMessage()
-            ex.printStackTrace();
-    		if (trace.isTraceEnabled()) trace.exit("preProcess",ex);
+    		if (trace.isTraceEnabled()) trace.exit("preProcess",th);
             return bytes;
         }
     }
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
index 6d814ef95..0a210f0e3 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
@@ -98,9 +98,9 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
     }
 
     protected void initialize (final ClassLoader classLoader, IWeavingContext context) {
-        //super(null);// at this stage we don't have yet a generatedClassHandler to define to the VM the closures
     	if (initialized) return;
 
+    	boolean success = true;
     	if (trace.isTraceEnabled()) trace.enter("initialize",this,new Object[] { classLoader, context });
 
     	this.weavingContext = context;
@@ -131,8 +131,9 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
         };
 
         List definitions = parseDefinitions(classLoader);
-        if (!isEnabled()) {
-        	if (trace.isTraceEnabled()) trace.exit("initialize",false);
+        if (definitions.isEmpty()) {
+        	disable(); // TODO maw Needed to ensure messages are flushed
+        	if (trace.isTraceEnabled()) trace.exit("initialize",definitions);
         	return;
         }
         
@@ -144,26 +145,25 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
                     }
                 }
         );
-//            //TODO this AJ code will call
-//            //org.aspectj.apache.bcel.Repository.setRepository(this);
-//            //ie set some static things
-//            //==> bogus as Bcel is expected to be
-//            org.aspectj.apache.bcel.Repository.setRepository(new ClassLoaderRepository(loader));
 
         weaver = new BcelWeaver(bcelWorld);
 
         // register the definitions
-        registerDefinitions(weaver, classLoader, definitions);
-        if (isEnabled()) {
-
-            //bcelWorld.setResolutionLoader(loader.getParent());//(ClassLoader)null);//
+        success = registerDefinitions(weaver, classLoader, definitions);
+        if (success) {
             
             // after adding aspects
             weaver.prepareForWeave();
-            boolean success = weaveAndDefineConceteAspects();
-            if (!success) disable();
+            
+            enable(); // TODO maw Needed to ensure messages are flushed
+            success = weaveAndDefineConceteAspects();
+        }
+
+        if (success) {
+            enable();
         }
         else {
+        	disable();
         	bcelWorld = null;
         	weaver = null;
         }
@@ -179,6 +179,8 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
      * @param loader
      */
     private List parseDefinitions(final ClassLoader loader) {
+        if (trace.isTraceEnabled()) trace.enter("parseDefinitions",this,loader);
+
         List definitions = new ArrayList();
     	try {
             info("register classloader " + getClassLoaderName(loader));
@@ -194,7 +196,7 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
             }
 
             String resourcePath = System.getProperty("org.aspectj.weaver.loadtime.configuration",AOP_XML);
-            trace.event("parseDefinitions",this,resourcePath);
+            if (trace.isTraceEnabled()) trace.event("parseDefinitions",this,resourcePath);
     		StringTokenizer st = new StringTokenizer(resourcePath,";");
 
     		while(st.hasMoreTokens()){
@@ -204,7 +206,7 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
     			Set seenBefore = new HashSet();
     			while (xmls.hasMoreElements()) {
     			    URL xml = (URL) xmls.nextElement();
-    	            trace.event("parseDefinitions",this,xml);
+    			    if (trace.isTraceEnabled()) trace.event("parseDefinitions",this,xml);
     			    if (!seenBefore.contains(xml)) {
     			    	info("using configuration " + weavingContext.getFile(xml));
     			    	definitions.add(DocumentParser.parse(xml));
@@ -216,28 +218,36 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
     			}
     		}
     		if (definitions.isEmpty()) {
-                disable();// will allow very fast skip in shouldWeave()
         		info("no configuration found. Disabling weaver for class loader " + getClassLoaderName(loader));
     		}
         } catch (Exception e) {
-            disable();// will allow very fast skip in shouldWeave()
+        	definitions.clear();
             warn("parse definitions failed",e);
         }
+        
+        if (trace.isTraceEnabled()) trace.exit("parseDefinitions",definitions);
 		return definitions;
     }
         
-    private void registerDefinitions(final BcelWeaver weaver, final ClassLoader loader, List definitions) {
-    	try {
+    private boolean registerDefinitions(final BcelWeaver weaver, final ClassLoader loader, List definitions) {
+        if (trace.isTraceEnabled()) trace.enter("registerDefinitions",this,definitions);
+        boolean success = true;
+
+        try {
             registerOptions(weaver, loader, definitions);
             registerAspectExclude(weaver, loader, definitions);
             registerAspectInclude(weaver, loader, definitions);
-            registerAspects(weaver, loader, definitions);
+            success = registerAspects(weaver, loader, definitions);
             registerIncludeExclude(weaver, loader, definitions);
             registerDump(weaver, loader, definitions);
-        } catch (Exception e) {
-            disable();// will allow very fast skip in shouldWeave()
-            warn("register definition failed",(e instanceof AbortException)?null:e);
+        } catch (Exception ex) {
+            trace.error("register definition failed",ex);
+            success = false;
+            warn("register definition failed",(ex instanceof AbortException)?null:ex);
         }
+        
+        if (trace.isTraceEnabled()) trace.exit("registerDefinitions",success);
+        return success;
     }
 
     private String getClassLoaderName (ClassLoader loader) {
@@ -370,7 +380,7 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
      * @param loader
      * @param definitions
      */
-    private void registerAspects(final BcelWeaver weaver, final ClassLoader loader, final List definitions) {
+    private boolean 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;
     	
@@ -435,17 +445,17 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
         
         /* 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 */
         else if (namespace == null) {
-        	disable();
+        	success = false;
     		info("no aspects registered. Disabling weaver for class loader " + getClassLoaderName(loader));
         }
 
-        if (trace.isTraceEnabled()) trace.exit("registerAspects",isEnabled());
+        if (trace.isTraceEnabled()) trace.exit("registerAspects",success);
+        return success;
     }
 
     private boolean weaveAndDefineConceteAspects () {
diff --git a/tests/bugs153/pr158957/HelloWorld.java b/tests/bugs153/pr158957/HelloWorld.java
new file mode 100644
index 000000000..8f62298fa
--- /dev/null
+++ b/tests/bugs153/pr158957/HelloWorld.java
@@ -0,0 +1,11 @@
+public class HelloWorld {
+
+	public void println () {
+		System.out.println("Hello World!");
+	}
+	
+	public static void main (String[] args) throws Exception {
+		new HelloWorld().println();
+	}
+	
+}
\ No newline at end of file
diff --git a/tests/bugs153/pr158957/Missing.java b/tests/bugs153/pr158957/Missing.java
new file mode 100644
index 000000000..a61ca7bfd
--- /dev/null
+++ b/tests/bugs153/pr158957/Missing.java
@@ -0,0 +1,3 @@
+public interface Missing {
+	public void doIt ();
+}
\ No newline at end of file
diff --git a/tests/bugs153/pr158957/PointcutLibrary.aj b/tests/bugs153/pr158957/PointcutLibrary.aj
new file mode 100644
index 000000000..ffda35e80
--- /dev/null
+++ b/tests/bugs153/pr158957/PointcutLibrary.aj
@@ -0,0 +1,9 @@
+public aspect PointcutLibrary {
+
+	public static pointcut doIt () :
+		execution(public void doIt()) && this(Missing);
+	
+	public static pointcut println () :
+		execution(public void println(..));
+	
+}
\ No newline at end of file
diff --git a/tests/bugs153/pr158957/Tracing.aj b/tests/bugs153/pr158957/Tracing.aj
new file mode 100644
index 000000000..5a974ad2e
--- /dev/null
+++ b/tests/bugs153/pr158957/Tracing.aj
@@ -0,0 +1,6 @@
+public aspect Tracing {
+	
+	before () : PointcutLibrary.println() {
+		System.out.println("? " + thisJoinPointStaticPart.getSignature().getName());
+	}
+}
\ No newline at end of file
diff --git a/tests/bugs153/pr158957/ant.xml b/tests/bugs153/pr158957/ant.xml
new file mode 100644
index 000000000..c2de686f1
--- /dev/null
+++ b/tests/bugs153/pr158957/ant.xml
@@ -0,0 +1,27 @@
+<!-- ajc-ant script, not to be used from Ant commant line - see AntSpec -->
+<project name="ltw">
+
+    <!-- using this we can debug the forked VM -->
+    <property
+        name="jdwp"
+        value="-Xdebug -Xnoagent -Djava.compiler=NONE -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=5005"/>
+	<property name="aj.bootpath" refid="aj.path"/>
+
+    <target name="NPE with LTW, pointcut library and missing aspect dependency">
+        <java fork="yes" classname="HelloWorld" failonerror="yes">
+            <classpath refid="aj.path"/>
+            <classpath>
+            	<!--<pathelement path="${aj.sandbox}/hello.jar:${aj.sandbox}/tracing.jar:${aj.sandbox}/missing.jar"/>-->
+            	<pathelement path="${aj.sandbox}/hello.jar:${aj.sandbox}/tracing.jar"/>
+            </classpath>
+            <jvmarg value="-Daj.weaving.verbose=true"/>
+            <jvmarg value="-Dorg.aspectj.weaver.showWeaveInfo=true"/>
+            <jvmarg value="-javaagent:${aj.root}/lib/test/loadtime5.jar"/>
+            <jvmarg value="-Dorg.aspectj.tracing.enabled=true"/>
+            <jvmarg value="-Dorg.aspectj.tracing.factory=default"/>
+            <jvmarg value="-Dorg.aspectj.tracing.messages=true"/>
+<!--            <jvmarg line="${jdwp}"/>-->
+        </java>
+    </target>
+
+</project>
diff --git a/tests/bugs153/pr158957/aop.xml b/tests/bugs153/pr158957/aop.xml
new file mode 100644
index 000000000..fec692fda
--- /dev/null
+++ b/tests/bugs153/pr158957/aop.xml
@@ -0,0 +1,9 @@
+<aspectj>
+	<aspects>
+	    <aspect name="PointcutLibrary"/>
+	    <aspect name="Tracing"/>
+	</aspects>
+	
+	<weaver options="-1.5"/>
+</aspectj>
+
diff --git a/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java b/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java
index caef4be71..3aa129227 100644
--- a/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java
+++ b/tests/src/org/aspectj/systemtest/ajc153/Ajc153Tests.java
@@ -150,6 +150,10 @@ public class Ajc153Tests extends org.aspectj.testing.XMLBasedAjcTestCase {
   public void testWeaveConcreteSubaspectWithCflow_pr132080() {
 	  runTest("Weave concrete sub-aspect with cflow");
   }
+
+  public void testNPEWithLTWPointcutLibraryAndMissingAspectDependency_pr158957 () {
+	  runTest("NPE with LTW, pointcut library and missing aspect dependency");
+  }
   
   public void testNoInvalidAbsoluteTypeNameWarning_pr156904_1() {runTest("ensure no invalidAbsoluteTypeName when do match - 1");}
   public void testNoInvalidAbsoluteTypeNameWarning_pr156904_2() {runTest("ensure no invalidAbsoluteTypeName when do match - 2");}
diff --git a/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml b/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml
index 9b64b631f..7e3f4e888 100644
--- a/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml
+++ b/tests/src/org/aspectj/systemtest/ajc153/ajc153.xml
@@ -544,6 +544,27 @@
         </ant>
     </ajc-test>
 
+	<ajc-test dir="bugs153/pr158957" title="NPE with LTW, pointcut library and missing aspect dependency" keywords="ltw">
+        <compile files="HelloWorld.java" options="-outjar hello.jar"/>
+        <compile files="Missing.java" options="-outjar missing.jar"/>
+        <compile files="Tracing.aj, PointcutLibrary.aj" options="-Xlint:ignore -1.5 -outxml -outjar tracing.jar" classpath="hello.jar,missing.jar"/>
+<!--        
+		<run class="HelloWorld" ltw="aop.xml">
+	        <stdout>
+    	      <line text="? main"/>
+    	      <line text="Hello World!"/>
+        	</stdout>
+        </run>
+-->        
+		<ant file="ant.xml" target="NPE with LTW, pointcut library and missing aspect dependency" verbose="true">
+	        <stdout>
+    	      <line text="? main"/>
+    	      <line text="Hello World!"/>
+        	</stdout>
+        </ant>
+
+    </ajc-test>
+
 	<ajc-test dir="bugs153/pr132080"
       title="Weave concrete sub-aspect with advice"
       keywords="aop.xml">
@@ -618,5 +639,25 @@
 		<run class="TestMain" ltw="aop-pr149096.xml"/>
     </ajc-test>
     
+	<ajc-test dir="bugs153/pr158957" title="NPE with LTW, pointcut library and missing aspect dependency" keywords="ltw">
+        <compile files="HelloWorld.java" options="-outjar hello.jar"/>
+        <compile files="Missing.java" options="-outjar missing.jar"/>
+        <compile files="Tracing.aj, PointcutLibrary.aj" options="-Xlint:ignore -1.5 -outxml -outjar tracing.jar" classpath="hello.jar,missing.jar"/>
+<!--        
+		<run class="HelloWorld" ltw="aop.xml">
+	        <stdout>
+    	      <line text="? main"/>
+    	      <line text="Hello World!"/>
+        	</stdout>
+        </run>
+-->        
+		<ant file="ant.xml" target="NPE with LTW, pointcut library and missing aspect dependency" verbose="true">
+	        <stdout>
+    	      <line text="? println"/>
+    	      <line text="Hello World!"/>
+        	</stdout>
+        </ant>
+
+    </ajc-test>
 
 </suite>
\ No newline at end of file
diff --git a/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java b/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
index 7d52c3afa..0d0420132 100644
--- a/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
+++ b/weaver/src/org/aspectj/weaver/tools/WeavingAdaptor.java
@@ -74,7 +74,7 @@ public class WeavingAdaptor implements IMessageContext {
 	public static final String SHOW_WEAVE_INFO_PROPERTY = "org.aspectj.weaver.showWeaveInfo"; 
 	public static final String TRACE_MESSAGES_PROPERTY = "org.aspectj.tracing.messages";
 
-	private boolean enabled = true;
+	private boolean enabled = false;
 	protected boolean verbose = getVerbose();
 	protected BcelWorld bcelWorld;
 	protected BcelWeaver weaver;
@@ -169,6 +169,8 @@ public class WeavingAdaptor implements IMessageContext {
 
 		weaver = new BcelWeaver(bcelWorld);
 		registerAspectLibraries(aspectPath);
+		
+		enabled = true;
 	}
 
 	protected void createMessageHandler() {
@@ -201,6 +203,11 @@ public class WeavingAdaptor implements IMessageContext {
 		if (trace.isTraceEnabled()) trace.exit("disable");
 	}
 	
+	protected void enable () {
+		enabled = true;
+		messageHolder.flushMessages();
+	}
+	
 	protected boolean isEnabled () {
 		return enabled;
 	}
@@ -227,6 +234,8 @@ public class WeavingAdaptor implements IMessageContext {
      * @exception IOException weave failed
 	 */
 	public byte[] weaveClass (String name, byte[] bytes) throws IOException {
+		if (trace.isTraceEnabled()) trace.enter("weaveClass",this,new Object[] {name, bytes});
+
 		if (enabled) {
 			try {
 				delegateForCurrentClass=null; 
@@ -258,6 +267,7 @@ public class WeavingAdaptor implements IMessageContext {
 			}
 		}
 
+		if (trace.isTraceEnabled()) trace.exit("weaveClass",bytes);
         return bytes;
 	}