From c315f9d9a754d01ea608d6d1ae020889d4018fc6 Mon Sep 17 00:00:00 2001 From: mwebster Date: Thu, 5 Oct 2006 10:13:54 +0000 Subject: [PATCH] Bug 158957 "NPE in LTW with RMI dynamic proxies w/ pointcut reuse" (restore catch(Throwable) in Aj.preProcess() and make adaptor disabled by default) --- .../src/org/aspectj/weaver/loadtime/Aj.java | 9 +-- .../loadtime/ClassLoaderWeavingAdaptor.java | 66 +++++++++++-------- tests/bugs153/pr158957/HelloWorld.java | 11 ++++ tests/bugs153/pr158957/Missing.java | 3 + tests/bugs153/pr158957/PointcutLibrary.aj | 9 +++ tests/bugs153/pr158957/Tracing.aj | 6 ++ tests/bugs153/pr158957/ant.xml | 27 ++++++++ tests/bugs153/pr158957/aop.xml | 9 +++ .../systemtest/ajc153/Ajc153Tests.java | 4 ++ .../org/aspectj/systemtest/ajc153/ajc153.xml | 41 ++++++++++++ .../aspectj/weaver/tools/WeavingAdaptor.java | 12 +++- 11 files changed, 164 insertions(+), 33 deletions(-) create mode 100644 tests/bugs153/pr158957/HelloWorld.java create mode 100644 tests/bugs153/pr158957/Missing.java create mode 100644 tests/bugs153/pr158957/PointcutLibrary.aj create mode 100644 tests/bugs153/pr158957/Tracing.aj create mode 100644 tests/bugs153/pr158957/ant.xml create mode 100644 tests/bugs153/pr158957/aop.xml 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 @@ + + + + + + + + + + + + + + + + + + + + + + + + + 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 @@ + + + + + + + + + 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 @@ -542,6 +542,27 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + \ 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; } -- 2.39.5