Comment 1 Andrew Clement 2006-05-16 08:17:37 EDT

There are certainly situations where errors only occur on incremental compilation - we record state during the full build to optimize subsequent incremental builds.  
I don't fully understand what is going wrong in this scenario though - from the description I might expect it to happen on a full build, so there is possibly something more serious lurking.  It needs the combination of a perclause of some kind and using @AJ syntax, is your aspect particularly complicated?

To just help you progress I am tempted to put in your simple null check extension to that test if we can't work out a simplistic scenario in which this happens reliably.

You could try running with the AJDT event trace open - it will tell us exactly what is getting recompiled/rewoven on an incremental build and that may give us a clue as to what is happening.  (You might have to press some magic switch to get the event trace to produce all its diagnostics info, either on the view window itself or in the AspectJ project/workbench properties).

Comment 2 Daniel Tabuenca 2006-05-16 11:37:26 EDT

The aspect is fairly simple: 

@Aspect
public class ExpirableToucher {

	@Before("within(com.residencycentral.app.Expirable+) && execution(public * *(..)) && !execution(* touch()) && target(target)")
	public void touchBeforeExecute(Expirable target) {
		target.touch();
	}
}


I will try my best to figure out the circumstances that reproduce this bug.

Comment 3 Daniel Tabuenca 2006-05-16 12:22:26 EDT

Is this the trace?

trouble in: 
public abstract class com.residencycentral.tapestry.pages.residency.html.SurveyReport extends com.residencycentral.tapestry.pages.residency.html.ModuleBasedPage:
  private static final String SURVEY_REPORT_MODULE_NAME = "SurveyReportModule"
  public void <init>():
                    ALOAD_0     // Lcom/residencycentral/tapestry/pages/residency/html/SurveyReport; this   (line 11)
                    INVOKESPECIAL com.residencycentral.tapestry.pages.residency.html.ModuleBasedPage.<init> ()V
    constructor-execution(void com.residencycentral.tapestry.pages.residency.html.SurveyReport.<init>())
    |               RETURN
    constructor-execution(void com.residencycentral.tapestry.pages.residency.html.SurveyReport.<init>())
  end public void <init>()

  public abstract org.apache.tapestry.IAsset getStyleSheet()    org.aspectj.weaver.MethodDeclarationLineNumber: 17:584
;

  protected void attachNewModule()    org.aspectj.weaver.MethodDeclarationLineNumber: 20:633
:
                    ALOAD_0     // Lcom/residencycentral/tapestry/pages/residency/html/SurveyReport; this   (line 21)
                    LDC "SurveyReportModule"
                    INVOKEVIRTUAL com.residencycentral.tapestry.pages.residency.html.SurveyReport.attachNewModule (Ljava/lang/String;)V
                    RETURN   (line 22)
  end protected void attachNewModule()

  private com.residencycentral.survey.modules.SurveyReportModule getSurveyReportModule()    org.aspectj.weaver.MethodDeclarationLineNumber: 24:740
:
                    ALOAD_0     // Lcom/residencycentral/tapestry/pages/residency/html/SurveyReport; this   (line 25)
                    INVOKEVIRTUAL com.residencycentral.tapestry.pages.residency.html.SurveyReport.getModule ()Lcom/residencycentral/app/ApplicationModule;
                    CHECKCAST com.residencycentral.survey.modules.SurveyReportModule
                    ARETURN
  end private com.residencycentral.survey.modules.SurveyReportModule getSurveyReportModule()

  public java.util.List getReportEntries()    org.aspectj.weaver.MethodDeclarationLineNumber: 28:849
:
                    ALOAD_0     // Lcom/residencycentral/tapestry/pages/residency/html/SurveyReport; this   (line 29)
                    INVOKESPECIAL com.residencycentral.tapestry.pages.residency.html.SurveyReport.getSurveyReportModule ()Lcom/residencycentral/survey/modules/SurveyReportModule;
                    INVOKEINTERFACE com.residencycentral.survey.modules.SurveyReportModule.getReportEntries ()Ljava/util/List;
                    ARETURN
  end public java.util.List getReportEntries()

  public String getColumnsExpression()    org.aspectj.weaver.MethodDeclarationLineNumber: 34:952
:
                    LDC "procedure:Procedure:procedure.description"   (line 36)
                    ASTORE_1
                    ALOAD_0     // Lcom/residencycentral/tapestry/pages/residency/html/SurveyReport; this   (line 37)
                    INVOKESPECIAL com.residencycentral.tapestry.pages.residency.html.SurveyReport.getSurveyReportModule ()Lcom/residencycentral/survey/modules/SurveyReportModule;
                    INVOKEINTERFACE com.residencycentral.survey.modules.SurveyReportModule.getSurveyOptions ()Ljava/util/List;
                    INVOKEINTERFACE java.util.List.iterator ()Ljava/util/Iterator;
                    ASTORE_3
                    GOTO L1
                L0: ALOAD_3
                    INVOKEINTERFACE java.util.Iterator.next ()Ljava/lang/Object;
                    CHECKCAST com.residencycentral.survey.SurveyOption
                    ASTORE_2
                    NEW java.lang.StringBuilder   (line 38)
                    DUP
                    ALOAD_1     // Ljava/lang/String; expression
                    INVOKESTATIC java.lang.String.valueOf (Ljava/lang/Object;)Ljava/lang/String;
                    INVOKESPECIAL java.lang.StringBuilder.<init> (Ljava/lang/String;)V
                    LDC ","
                    INVOKEVIRTUAL java.lang.StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
                    ALOAD_2     // Lcom/residencycentral/survey/SurveyOption; option
                    INVOKEINTERFACE com.residencycentral.survey.SurveyOption.getId ()Ljava/lang/Long;
                    INVOKEVIRTUAL java.lang.StringBuilder.append (Ljava/lang/Object;)Ljava/lang/StringBuilder;
                    LDC ":"
                    INVOKEVIRTUAL java.lang.StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
                    ALOAD_2     // Lcom/residencycentral/survey/SurveyOption; option
                    INVOKEINTERFACE com.residencycentral.survey.SurveyOption.getLabel ()Ljava/lang/String;
                    INVOKEVIRTUAL java.lang.StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
                    LDC ":getCountForAnswer("   (line 39)
                    INVOKEVIRTUAL java.lang.StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
                    ALOAD_2     // Lcom/residencycentral/survey/SurveyOption; option
                    INVOKEINTERFACE com.residencycentral.survey.SurveyOption.getId ()Ljava/lang/Long;
                    INVOKEVIRTUAL java.lang.StringBuilder.append (Ljava/lang/Object;)Ljava/lang/StringBuilder;
                    LDC ")"
                    INVOKEVIRTUAL java.lang.StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
                    INVOKEVIRTUAL java.lang.StringBuilder.toString ()Ljava/lang/String;   (line 38)
                    ASTORE_1     // Ljava/lang/String; expression
                L1: ALOAD_3   (line 37)
                    INVOKEINTERFACE java.util.Iterator.hasNext ()Z
                    IFNE L0
                    ALOAD_1     // Ljava/lang/String; expression   (line 41)
                    ARETURN
  end public String getColumnsExpression()

end public abstract class com.residencycentral.tapestry.pages.residency.html.SurveyReport

when weaving type com.residencycentral.tapestry.pages.residency.html.SurveyReport
when weaving classes 
when weaving 
when incrementally building BuildConfig[H:\programming\workspace\.metadata\.plugins\org.eclipse.ajdt.core\residency.generated.lst] #Files=367

Comment 4 Daniel Tabuenca 2006-05-16 18:00:07 EDT

Ok I have debugged some more and I think I'm closer to the source of the bug.

When I do a clean and compile, in BcelWeaver.prepareForWeave() it sets the typeMunger list on line 464:
    typeMungerList = xcutSet.getTypeMungers();
    lateTypeMungerList = xcutSet.getLateTypeMungers();
    declareParentsList = xcutSet.getDeclareParents();

when going from a clean compile xcutSet.getTypeMungers() returns a bunch of mungers but not the BcelPerClauseAspectAdder munger that is the source of 
the segfault later on. 

the BcelPerClauseApectAdder is returned by the second call to xcutSet.getLateTypeMungers(), but this is fine because this list is not checked in the code that caused the segfault.

When doing the incremental compile,  the xcutSet.getTypeMungers() gets modified by org.aspectj.ajdt.internal.compiler.lookup.EclipseFactory.finishTypeMungers():

// XXX by Andy: why do we mix up the mungers here? it means later we know about 
// two sets and the late ones are a subset of the complete set? (see pr114436)
        baseTypeMungers.addAll(getWorld().getCrosscuttingMembersSet().getLateTypeMungers());

I'm not sure why this is done or what it does as I don't really understand all the inner workings of AspectJ. But what happens is now those LateTypeMungers (which include the BcelPerClauseAspectAdder mungers that cause the problem) now get added to the list returned by xcutSet.getTypeMungers();.

In fact, it seems this is not intended, because if I edit the code and recompile 
several times, it keeps appending those BcelPerClauseAspectAdder to the end of the list so after a while I have 5-6 mungers followed by 40+ BcelPerClauseAspectAdder mungers. 

I'd appreciate any further clarification of what this code does and how it could be fixed. I don't fully understand  what the BcelPerClauseAspectAdder does or what a LateTypeMunger is or why it is merged... etc... but I hope the debugging information I'm providing can get this solved soon.

Comment 5 Andrew Clement 2006-05-17 07:51:09 EDT

More good work Daniel ;)

based on what you have said, I've created a testcase that shows us leaking the typemungers on incremental compiles.

Late type mungers are 'special' ones that can only perform their job after the pointcuts have been matched and the advice applied.  (Normal type mungers are applied before the pointcuts are matched and advice is applied - here 'normal' means things like ITDs).

The BcelPerClauseAspectAdder creates the methods aspectOf() and hasAspect() - for each kind of aspect these vary depending on the per clause in use.  Specifying no perclause means you get the default of 'persingleton'.  In the old days before annotation style development came along, the compiler generated aspectOf() and hasAspect().  Nowadays with annotation style it is possible the aspect is built using regular javac - which won't create these aspectof()/hasAspect() methods.  So the BcelPerClauseAdder was created which adds these methods at weave time for @AJ aspects.

As you have discovered, I put a comment in the code against something another developer did:

// XXX by Andy: why do we mix up the mungers here? it means later we know about 
// two sets and the late ones are a subset of the complete set? (see pr114436)
       
baseTypeMungers.addAll(getWorld().getCrosscuttingMembersSet().getLateTypeMungers());

which I wanted to revisit at some point when I had time to investigate. Now based on my testcase and your observation about the collection constantly growing on compiles, I've removed that line of code.  All our tests continue to execute fine.  I have also put in the guard for a null munger that was described in the initial comment in this bug report.  So the code should be much more reliable now.

the fixes will be in a dev build shortly, then in an AJDT build a little after that.
thanks for the investigation work!

Comment 6 Andrew Clement 2006-05-17 11:18:07 EDT

fix available in latest AJ dev build.

Comment 7 Andrew Clement 2006-05-18 06:41:37 EDT

fixed.