Bug 307120 - pipeline weaving problem when advising fields accessed through privileged aspect
Summary: pipeline weaving problem when advising fields accessed through privileged aspect
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Windows 7
: P2 critical (vote)
Target Milestone: 1.6.9M2   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-25 14:39 EDT by Andrew Clement CLA
Modified: 2010-05-13 13:46 EDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Clement CLA 2010-03-25 14:39:52 EDT
The problem here is that if you attempt to advise a field that is being accessed from an ITD, and that field is private in the target, you can fail to advise it depending on the order of processing the target type and the aspect.

class A {
  private int i;
}

privileged aspect X {
  public int getI() { 
    return i;
  }

  before(): get(int i) {}
}

the advice there may not affect the ITD depending on the compilation order.
Comment 1 Andrew Clement CLA 2010-03-25 14:44:28 EDT
the problem is the lookup of the joinpoint signature 'int A.i' which is done in the ResolvedType.lookupSyntheticMember() - when things are working the intertypemunger (a privileged access munger) is registered against A and it is found in the first loop and the resolvedmember returned.  When it fails the privilegedaccessmunger hasn't yet been added so it isn't found.
Comment 2 Andrew Clement CLA 2010-03-25 15:47:18 EDT
what makes this particularly complicated is that the privileged access munger is not something you know up front from simply looking at the diet parse of an aspect.  Other ITDs are found from that but the need for the privileged mungers only becomes apparent when the method bodies are processed and we discover an attempt to access a member that the aspect wouldn't normally be able to reach.

Everything works fine without pipeline compilation and so this is really just a matter of getting the necessary Eclipse form of the privileged munger onto the target to satisfy the lookup.

It can be made to work via this addition to PrivilegedHandler.getPrivilegedAccessField():

ResolvedType rt = inAspect.factory.fromEclipse(baseField.declaringClass);
ReferenceTypeDelegate rtd = ((ReferenceType)rt).getDelegate();
if (rtd instanceof EclipseSourceType) {
  rt.addInterTypeMunger(new EclipseTypeMunger(inAspect.factory,
    new PrivilegedAccessMunger(key, true),inAspect.typeX,null), true);
}

which discovers that the delegate for the target is still an Eclipse type and knows it needs to add the privileged access munger.
Comment 3 Andrew Clement CLA 2010-03-25 18:15:27 EDT
fixed as outlined in the previous comment.  I thank that fix is OK.
Comment 4 Andrew Clement CLA 2010-04-05 13:55:43 EDT
fix needed to be enhanced.  The check for instanceof EclipseSourceType means it won't work on an incremental build when the delegate has switched from the eclipse representation to a bcel rep.  Remove that check and it works ok - test added
Comment 5 Andrew Clement CLA 2010-04-08 16:24:20 EDT
holy cow this is the bug that keeps on giving.

Just discovered that the most recent changes won't help if the field is being matched based upon an annotation.  This is because the EclipseResolvedMember that is built to represent the field (which is being accessed in a privileged way) will be backed by a BinaryTypeBinding on an incremental build rather than a SourceTypeBinding.  When this is the case we can't go back to the declaration and grab the annotations from there.  The annotations will exist, they are in the ReferenceType (now backed by a BcelObject), we just have to tweak EclipseResolvedMember to look there if it is backed by a BinaryTypeBinding.
Comment 6 Andrew Clement CLA 2010-04-08 16:49:38 EDT
right solved that.  I bet it may address some cryptic failures in incremental building when annotations suddenly do or dont match.
Comment 7 Andrew Clement CLA 2010-04-22 15:18:42 EDT
and again... this time when pipeline compilation is on there is a problem with matching during a full build.

I wouldn't normally work on bugs relating to the pipeline being off, but they can sometimes suggest things that could appear during incremental builds with pipelining on - ie. it is some combination of file ordering that leads to the problem.

Here I'm finding that advice on the set of a private field, where the match is based on annotations and the advice is from a privileged aspect, will only work if the field is being represented as an EclipseResolvedMember (which is the case with pipelining off). With pipelining on it is a regular member with no annotations and not marked 'annotated elsewhere' so the match fails
Comment 8 Andrew Clement CLA 2010-04-22 16:16:35 EDT
I don't think 'isAnnotatedElsewhere' is the right approach to take here.  The right approach is to correctly resolve the members for which PrivilegedAccessMungers are created.  ResolvedMembers are stored in the class file attributes for these mungers but they don't have annotation information in them.

So the change to fix this is in CrosscuttingMembers.addPrivilegedAccesses() which attempts member lookup prior to building the privilegedaccessmunger.  With the correct one found the annotation match succeeds.

I can possibly see a problem with privileged access to ITD fields - but that sounds nasty anyway!  When that happens the lookup of the member doesn't quite work, the check in ResolvedTypeMunger.getMatchingSyntheticMember() which checks isPublic() will not pass for these kinds of signature (privileged access wouldn't be needed if they were public...)
Comment 9 Andrew Clement CLA 2010-04-22 16:56:56 EDT
now another strange situation... pipe=false

account field public.

public void Person.setAccount(Account account) {
        this.account = account;
    }

field-set is advised by aspect aspect S but not by aspect N.  On inc build (of aspect containing setAccount) same result.

with account private.

field-set advised by aspect N on full build (not S!) then on incremental it is advised by S instead of N.

So, should N match it at all?

pointcut from N involves a generic aspect and pointcut that uses type variables.  It is a complex pointcut but it should *not* be matching the set as there is a !(set(@Foo * *)) element to the pointcut and the field has that annotation.  See if I can create a testcase