Bug 42438 - Duplicate JSP temp variable declaration when jsp:attribute used in conjunction with custom tags
Summary: Duplicate JSP temp variable declaration when jsp:attribute used in conjunctio...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Jasper (show other bugs)
Version: 5.0.20
Hardware: All All
: P2 major with 1 vote (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-16 17:43 UTC by Brian Lenz
Modified: 2007-05-21 19:31 UTC (History)
1 user (show)



Attachments
Test case WAR (362.03 KB, application/octet-stream)
2007-05-21 08:17 UTC, Brian Lenz
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Lenz 2007-05-16 17:43:51 UTC
Here is an example of the bug in its most primitive form:

WEB-INF/jsp/test.jsp:
===================================
<%@ page contentType="text/html;charset=UTF-8" language="java" %>

<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core"%>
<%@ taglib prefix="t" tagdir="/WEB-INF/tags" %>

<jsp:element name="a">
    <jsp:attribute name="href">http://www.apache.org</jsp:attribute>
</jsp:element>

<t:test />

<c:if test="${true}">it's true!</c:if>
===================================

WEB-INF/tags/test.tag:
===================================
<%@ tag pageEncoding="UTF-8" body-content="scriptless" %>
do nothing
===================================

WEB-INF/tagPlugins.xml:
===================================
<tag-plugins>
    <tag-plugin>
      <tag-class>org.apache.taglibs.standard.tag.rt.core.IfTag</tag-class>
      <plugin-class>org.apache.jasper.tagplugins.jstl.core.If</plugin-class>
    </tag-plugin>
</tag-plugins>
===================================

Once you run this through Jasper, you will get the following java code:
===================================
package org.apache.jsp.WEB_002dINF.jsp;

import javax.servlet.*;
import javax.servlet.http.*;
import javax.servlet.jsp.*;

public final class test_jsp extends org.apache.jasper.runtime.HttpJspBase
    implements org.apache.jasper.runtime.JspSourceDependent {

  private static java.util.List _jspx_dependants;

  static {
    _jspx_dependants = new java.util.ArrayList(1);
    _jspx_dependants.add("/WEB-INF/tags/test.tag");
  }

  private org.apache.jasper.runtime.TagHandlerPool _jspx_tagPool_c_if_test;

  public Object getDependants() {
    return _jspx_dependants;
  }

  public void _jspInit() {
    _jspx_tagPool_c_if_test =
org.apache.jasper.runtime.TagHandlerPool.getTagHandlerPool(getServletConfig());
  }

  public void _jspDestroy() {
    _jspx_tagPool_c_if_test.release();
  }

  public void _jspService(HttpServletRequest request, HttpServletResponse response)
        throws java.io.IOException, ServletException {

    JspFactory _jspxFactory = null;
    PageContext pageContext = null;
    HttpSession session = null;
    ServletContext application = null;
    ServletConfig config = null;
    JspWriter out = null;
    Object page = this;
    JspWriter _jspx_out = null;
    PageContext _jspx_page_context = null;


    try {
      _jspxFactory = JspFactory.getDefaultFactory();
      response.setContentType("text/html;charset=UTF-8");
      pageContext = _jspxFactory.getPageContext(this, request, response,
      			null, true, 8192, true);
      _jspx_page_context = pageContext;
      application = pageContext.getServletContext();
      config = pageContext.getServletConfig();
      session = pageContext.getSession();
      out = pageContext.getOut();
      _jspx_out = out;

      String _jspx_temp0 = "http://www.apache.org";
      out.write("<" + "a" + " href=\"" + _jspx_temp0 + "\"" + "/>");
      if (_jspx_meth_t_test_0(_jspx_page_context))
        return;
boolean _jspx_temp0=
((java.lang.Boolean)
org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate("${true}",
java.lang.Boolean.class, (PageContext)_jspx_page_context, null,
false)).booleanValue();
if (_jspx_temp0){
      out.write("it's true!");
}
    } catch (Throwable t) {
      if (!(t instanceof SkipPageException)){
        out = _jspx_out;
        if (out != null && out.getBufferSize() != 0)
          out.clearBuffer();
        if (_jspx_page_context != null) _jspx_page_context.handlePageException(t);
      }
    } finally {
      if (_jspxFactory != null) _jspxFactory.releasePageContext(_jspx_page_context);
    }
  }

  private boolean _jspx_meth_t_test_0(PageContext _jspx_page_context)
          throws Throwable {
    PageContext pageContext = _jspx_page_context;
    JspWriter out = _jspx_page_context.getOut();
    //  t:test
    org.apache.jsp.tag.web.test_tag _jspx_th_t_test_0 = new
org.apache.jsp.tag.web.test_tag();
    _jspx_th_t_test_0.setJspContext(_jspx_page_context);
    _jspx_th_t_test_0.doTag();
    return false;
  }
}
===================================

Scan that code for the temp variable named _jspx_temp0.  Notice that there is
one String with that name and one boolean with that name.  Thus, this results in
a java compilation error once you ultimately try to compile the page:
===================================
Compiling 2 source files to c:\work\marzen\tmp\jspc\classes
c:\work\marzen\tmp\jspc\src\org\apache\jsp\WEB_002dINF\jsp\test_jsp.java:61:
_jspx_temp0 is already defined in
_jspService(javax.servlet.http.HttpServletRequest,javax.servlet.http.HttpServletResponse)
boolean _jspx_temp0=
        ^
c:\work\marzen\tmp\jspc\src\org\apache\jsp\WEB_002dINF\jsp\test_jsp.java:63:
incompatible types
found   : java.lang.String
required: boolean
if (_jspx_temp0){
    ^
Note: c:\work\marzen\tmp\jspc\src\org\apache\jsp\WEB_002dINF\jsp\test_jsp.java
uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
2 errors
===================================

The problem is caused by the fact that the Node.NamedAttribute constructor
directly uses JspUtil.nextTemporaryVariableName() to generate a variable name. 
I *believe* that if the variable was initialized just-in-time in the getter,
that the problem would be solved.

The flow goes like this:

1) When compiling test.jsp, Compiler.generateJava() calls
JspUtil.resetTemporaryVariableName() to reset the temp variable name

2) The test.jsp file gets parsed immediately thereafter, thus creating a
variable with name _jspx_temp0 for the "href" attribute

3) Shortly thereafter, tag files are loaded, which means that the test.tag file
will be recompiled, thus resulting in JspUtil.resetTemporaryVariableName() being
invoked again (thus setting the variable back to 0).

4) Since test.tag doesn't do anything to cause a temporary variable name to be
recreated, the temp variable name remains at 0.

5) Upon returning to the compilation of the test.jsp, the If tag plugin is used
to create yet another variable name with name _jspx_temp0, hence the conflict.

I think step 2 is the root of the problem.  If the NamedAttribute didn't get a
temporary variable name assigned in the constructor and instead generated it
just-in-time in the getter, then this issue wouldn't exist since the "href"
attribute variable name would be created only after first compiling all of the
tag files.

I still think that the logic in the compilation is a bit strange given that the
JSP will use temporary variable names starting at whatever point the last
compiled tag file left off at.  I think that issue could be resolved by moving
the JspUtil.resetTemporaryVariableName() call in Compiler.generateJava() down
after the call to loadTagFiles().  Note that you would probably also want to
call JspUtil.resetTemporaryVariableName() in the ctxt.isPrototypeMode()
scenario.  Here is the diff that I would propose for Compiler.java based on the
5.5.20 codebase:

===================================
150,152d149
<             // Reset the temporary variable counter for the generator.
<             JspUtil.resetTemporaryVariableName();
<
157a155,156
>                 // Reset the temporary variable counter for the generator.
>                 JspUtil.resetTemporaryVariableName();
179a179,181
>             // Reset the temporary variable counter for the generator.
>             JspUtil.resetTemporaryVariableName();
>
===================================


Here is the diff I would propose for Node.java:
===================================
1791d1790
<             temporaryVariableName = JspUtil.nextTemporaryVariableName();
1837a1837,1839
>             if(temporaryVariableName==null) {
>                 temporaryVariableName = JspUtil.nextTemporaryVariableName();
>             }
===================================
Comment 1 Brian Lenz 2007-05-16 18:14:38 UTC
FWIW, I have built my own custom jasper-compiler.jar with the two patches I
mentioned, and it definitively solved my use case.
Comment 2 Brian Lenz 2007-05-18 08:49:48 UTC
I've checked the Tomcat 5.5.23 code, and it looks like it exhibits the exact
same problem.  I believe that the two patches I listed would solve the problem
in 5.5.23, as well.
Comment 3 Mark Thomas 2007-05-19 10:29:48 UTC
This works for me using the latest 5.5.x code either with Tomcat compiling on
the fly or using JspC.

If you still see this issue, please attach a test WAR file (including source)
that demonstrates this issue with a clean 5.5.23 installation.
Comment 4 Brian Lenz 2007-05-21 08:17:07 UTC
Created attachment 20224 [details]
Test case WAR

Here is a test case exhibiting the bug.  You need to make sure that the
test.tag file is compiled as part of the test.jsp file compilation.  If
test.tag is compiled independently first, then the problem will not occur.  The
easiest way to make sure this is the case is to just clean out your work
directory before loading test.jsp.
Comment 5 Brian Lenz 2007-05-21 08:20:25 UTC
Is it possible you weren't using the If tag plugin I specified?  Or is it
possible that you had precompiled the test.tag file?  The problem only occurs
when you are compiling test.jsp, which in turn results in compilation of
test.tag and you are using the If tag plugin.  This is definitely still a bug in
Tomcat 5.5.23.

Note that this is just one example exhibiting the problem.  There are a
multitude of other scenarios where this exact same problem can crop up (as it is
happening in our production JSP compilation).  I've just simplified the code
down to a very easy to trace test case to show exactly where the problem is
happening.

If you have any further questions on how to reproduce the bug, let me know.
Comment 6 Mark Thomas 2007-05-21 19:31:53 UTC
Thanks for the WAR. I see the error now. I have applied the Node.java patch to
svn and it will be included in the next release of 5.5.x and 6.0.x.

I didn't change Compiler.java as it isn't necessary for this fix.

Many thanks for both the test case and the patch.