Tapestry 5
  1. Tapestry 5
  2. TAP5-1601

Sometime a method that references a field with a conduit will not be instrumented, resulting in an NPE accessing the field itself

    Details

      Description

      From ProQuest, they have a component that has this code:

      public class Layout {

      @Inject
      private ComponentResources resources;

      @Cached
      public Component getPage()

      { return resources.getContainer(); }

      }

      The generated bytecode for the advised method looks ok:

      public getPage()Lorg/apache/tapestry5/runtime/Component;
      @Lorg/apache/tapestry5/annotations/Cached;()
      NEW com/proquest/apps/onesearch/components/PageLayoutBasic$Invocation_getPage_123876a5ccf1f215
      DUP
      ALOAD 0
      ALOAD 0
      GETFIELD com/proquest/apps/onesearch/components/PageLayoutBasic.instanceContext : Lorg/apache/tapestry5/plastic/InstanceContext;
      ALOAD 0
      GETFIELD com/proquest/apps/onesearch/components/PageLayoutBasic.methodinvocationbundle_getPage_123876a5ccf1f22d : Lorg/apache/tapestry5/internal/plastic/MethodInvocationBundle;
      INVOKESPECIAL com/proquest/apps/onesearch/components/PageLayoutBasic$Invocation_getPage_123876a5ccf1f215.<init> (Ljava/lang/Object;Lorg/apache/tapestry5/plastic/InstanceContext;Lorg/apache/tapestry5/internal/plastic/MethodInvocationBundle;)V
      L0
      DUP
      ASTORE 1
      INVOKEVIRTUAL org/apache/tapestry5/internal/plastic/AbstractMethodInvocation.proceed ()Lorg/apache/tapestry5/plastic/MethodInvocation;
      ALOAD 1
      GETFIELD com/proquest/apps/onesearch/components/PageLayoutBasic$Invocation_getPage_123876a5ccf1f215.returnValue : Lorg/apache/tapestry5/runtime/Component;
      ARETURN
      L1
      LOCALVARIABLE var0 Lcom/proquest/apps/onesearch/components/PageLayoutBasic$Invocation_getPage_123876a5ccf1f215; L0 L1 1
      MAXSTACK = 1
      MAXLOCALS = 1

      ... but the bytecode for the advised method is not instrumented:

      public advised$getPage_123876a5ccf1f216()Lorg/apache/tapestry5/runtime/Component;
      @Lorg/apache/tapestry5/annotations/Cached;()
      ALOAD 0
      GETFIELD com/proquest/apps/onesearch/components/PageLayoutBasic.resources : Lorg/apache/tapestry5/ComponentResources;
      INVOKEINTERFACE org/apache/tapestry5/ComponentResources.getContainer ()Lorg/apache/tapestry5/runtime/Component;
      ARETURN
      MAXSTACK = 1
      MAXLOCALS = 1

      ... even though the necessary field conduit support method is present:

      // access flags 0x1010
      final getfieldvalue_resources()Lorg/apache/tapestry5/ComponentResources;
      ALOAD 0
      GETFIELD com/proquest/apps/onesearch/components/PageLayoutBasic.resources_FieldConduit : Lorg/apache/tapestry5/plastic/FieldConduit;
      ALOAD 0
      ALOAD 0
      GETFIELD com/proquest/apps/onesearch/components/PageLayoutBasic.instanceContext : Lorg/apache/tapestry5/plastic/InstanceContext;
      INVOKEINTERFACE org/apache/tapestry5/plastic/FieldConduit.get (Ljava/lang/Object;Lorg/apache/tapestry5/plastic/InstanceContext;)Ljava/lang/Object;
      CHECKCAST org/apache/tapestry5/ComponentResources
      DUP
      ALOAD 0
      SWAP
      PUTFIELD com/proquest/apps/onesearch/components/PageLayoutBasic.resources : Lorg/apache/tapestry5/ComponentResources;
      ARETURN
      MAXSTACK = 0
      MAXLOCALS = 0

      ... also, looking at the real class, from the client, as transformed, many other methods containing references to the field are being transformed.

        Issue Links

          Activity

          Hide
          Howard M. Lewis Ship added a comment -

          At this time, the bug can not be reproduced. I've reviewed the code from several perspectives, and if there's a problem, I'm still not seeing it.

          Show
          Howard M. Lewis Ship added a comment - At this time, the bug can not be reproduced. I've reviewed the code from several perspectives, and if there's a problem, I'm still not seeing it.
          Hide
          Howard M. Lewis Ship added a comment -

          Reported still present in 5.2-beta-22.

          Show
          Howard M. Lewis Ship added a comment - Reported still present in 5.2-beta-22.
          Hide
          Andy Blower added a comment -

          Using 5.3-beta-27 this still happens. I found it occurring in other components today as well, and chose a small simple one to include with PageLayoutBasic. I've attached a zip file with java & class files of these classes working and not working along with exceptions thrown and tml file.

          I hope that comparing these two, and having a smaller simpler component may help narrow down what is happening. Again this only seems to occur when running the WAR file in tomcat and not Jetty from Eclipse, but as far as I can see the JVMs being used are the same.

          Show
          Andy Blower added a comment - Using 5.3-beta-27 this still happens. I found it occurring in other components today as well, and chose a small simple one to include with PageLayoutBasic. I've attached a zip file with java & class files of these classes working and not working along with exceptions thrown and tml file. I hope that comparing these two, and having a smaller simpler component may help narrow down what is happening. Again this only seems to occur when running the WAR file in tomcat and not Jetty from Eclipse, but as far as I can see the JVMs being used are the same.
          Hide
          Howard M. Lewis Ship added a comment -

          Found this, or something similar, for another client.

          It looks like an unwanted interaction between method advice and the way render phase methods are wired together.

          After the method advice is added, the "true" render phase method, such as public void afterRender(MarkupWriter, Event), has an empty implementation when it should be invoking the method(s) annotated with @AfterRender (or following the naming convention).

          Show
          Howard M. Lewis Ship added a comment - Found this, or something similar, for another client. It looks like an unwanted interaction between method advice and the way render phase methods are wired together. After the method advice is added, the "true" render phase method, such as public void afterRender(MarkupWriter, Event), has an empty implementation when it should be invoking the method(s) annotated with @AfterRender (or following the naming convention).
          Hide
          Howard M. Lewis Ship added a comment -

          So, the solution I believe I've found for Widen is that they have a ComponentClassTransformWorker2 that adds advice to the afterRender(MarkupWriter,Event) method.

          When that worker executes early, it ends up wiping out the replace implementation of afterRender(MarkupWriter,Event) (the one that invokes the afterRender() method), as part of how the method advice mechanism works.

          Ordering the CCTW2 that adds advice to execute "after:RenderPhase" appears to fix the problem.

          Why?

          When the worker introduces the method, an empty implementation is created. When it then adds advice to the method, a snapshot of the code is taken to form the implementation of the advised$ method. This new method, which as an "advised$" prefix and a unique id suffix, is supposed to be the original implementation of the method.

          Later, RenderPhaseWorker introduces the afterRender(MarkupWriter,Event) method and writes the implementation that invokes the afterRender() method.

          At the very end of the process, when method advice is applied, the afterRender(MarkupWriter,Event) method implementation is discarded and replaced with the code that instantiates the Invocation object; the Invocation object will ultimately invoke the advise$afterRender(MarkupWriter,Event) method ... but that method's implementation is empty. The code written by RenderPhaseWorker has been lost.

          TAP5-1979 identifies that either Tapestry should recognize that this situation has occured and throw an error OR it should handle this case seamlessly.

          This does explain the error that Widen has seen ... I'm not sure that it addresses the error that ProQuest has seen.

          Show
          Howard M. Lewis Ship added a comment - So, the solution I believe I've found for Widen is that they have a ComponentClassTransformWorker2 that adds advice to the afterRender(MarkupWriter,Event) method. When that worker executes early, it ends up wiping out the replace implementation of afterRender(MarkupWriter,Event) (the one that invokes the afterRender() method), as part of how the method advice mechanism works. Ordering the CCTW2 that adds advice to execute "after:RenderPhase" appears to fix the problem. Why? When the worker introduces the method, an empty implementation is created. When it then adds advice to the method, a snapshot of the code is taken to form the implementation of the advised$ method. This new method, which as an "advised$" prefix and a unique id suffix, is supposed to be the original implementation of the method. Later, RenderPhaseWorker introduces the afterRender(MarkupWriter,Event) method and writes the implementation that invokes the afterRender() method. At the very end of the process, when method advice is applied, the afterRender(MarkupWriter,Event) method implementation is discarded and replaced with the code that instantiates the Invocation object; the Invocation object will ultimately invoke the advise$afterRender(MarkupWriter,Event) method ... but that method's implementation is empty. The code written by RenderPhaseWorker has been lost. TAP5-1979 identifies that either Tapestry should recognize that this situation has occured and throw an error OR it should handle this case seamlessly. This does explain the error that Widen has seen ... I'm not sure that it addresses the error that ProQuest has seen.
          Hide
          Howard M. Lewis Ship added a comment -

          The fix for TAP5-1979 should, I think, fix this bug as well. How can we validate this so I can mark 1601 has fixed?

          Show
          Howard M. Lewis Ship added a comment - The fix for TAP5-1979 should, I think, fix this bug as well. How can we validate this so I can mark 1601 has fixed?
          Hide
          Howard M. Lewis Ship added a comment -

          Although this has not been confirmed by the customer, I am increasingly convinced that it is fixed by TAP5-1979.

          Show
          Howard M. Lewis Ship added a comment - Although this has not been confirmed by the customer, I am increasingly convinced that it is fixed by TAP5-1979 .
          Hide
          Andy Blower added a comment -

          Sorry was on holiday. I've looked though what you've done and it seems like it might fix the issue we encountered. I'm not sure I can remember enough about how we ran into it to confirm though. If I can find half a day to investigate and confirm I will do, but i cannot guarantee this.

          Show
          Andy Blower added a comment - Sorry was on holiday. I've looked though what you've done and it seems like it might fix the issue we encountered. I'm not sure I can remember enough about how we ran into it to confirm though. If I can find half a day to investigate and confirm I will do, but i cannot guarantee this.

            People

            • Assignee:
              Howard M. Lewis Ship
              Reporter:
              Howard M. Lewis Ship
            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development