Bug 46339

Thanks for the excellent test case. It made investigating this much, much easier.

Unfortunately, the behaviour you are expecting appears to be in breach of the JSP spec. I say "appears" since the JSP spec itself isn't 100% clear. JSP.5.12.3 states that:

"When a tag file invokes a fragment that appears in the calling page, the JSP container provides a way to synchronize variables between the local page scope in the tag file and the page scope of the calling page."

What wasn't clear to me was how should this be interpreted for iterative and/or nested tags. Should the tag's local page scope be synchronised with the page scope of the page/tag that calls it (i.e. its immediate parent) or should it be synchronised with the outermost calling JSP page?

Currently, Tomcat synchronises local page scope with the parent's page scope. Your test case requires that the local page scope is synchronised with the outermost calling JSP page.

I modified Tomcat so your test would pass and ran the JSP Technology Compatibility Kit (TCK). The changes caused several failures. Further investigation showed that the TCK expects the local page scope to be synchronised only with the immediate parent. Based on experience, if there is an ambiguity in the specification language and the TCK requires a particular interpretation of the spec language then the TCK interpretation is the correct one.

Therefore, I conclude that this bug in invalid since it attempts to do something in breach of the spec.

To be sure, I will raise this with the EG in case the TCK is based on an incorrect interpretation.

Created attachment 23055 [details]
Patch used for testing

I have attached the patch I used for testing. It enabled your test case to pass but it breaks Tomcat's spec compatibility. I have attached it in case you wish to apply it locally or if I need to return to it in light of the EG's response.

The proposed patch to synchronize tag variables to the outermost page context will work for this program, but will fail in other cases.  Consider the case where recursive.tag invokes foo.tag, passing another fragment.  It would be wrong to synchronize the variables in foo.tag with the outermost page context in this case, because that fragment is defined in foo.tag, and not index.jsp.

To fix this bug, the variable needs to be synchronized with the page context that the fragment is created, before the fragment is invoked.  I propose to add a parameter to org.apache.jasper.runtime.JspContextWrapper.syncBeforeInvoke().

    /**
     * Synchronize variables before fragment invokation
     * @param jspContext The Jsp context of the fragment
     */
    public void syncBeforeInvoke(JspContext jspContext) {
        copyTagToPageScope(jspContext, VariableInfo.NESTED);
        copyTagToPageScope(jspContext, VariableInfo.AT_BEGIN);
    }

I think we can safely leave the rest of JspContextWrapper the way it is.

Of course, Generator needs to be modified to get the JspContext from the fragment and pass it to syncBeforeInvoke().

I'll submit a patch when I get around to it.  :-)

It would be nice for the spec to clarify this.  JSP.8.9.1 and Table JSP.8-6 will need to be updated.

Re-opening as a result of Kin-Man's comment.

I'll add looking a this to my todo list but if you get there first with the patch that is fine too :)

Created attachment 23094 [details]
Patch based on glassfish source

The patch is based on glassfish source, so the lines may be off.

My reading of the spec (JSP 2.1, Final Release) is the following

1. The spec in many places uses "calling page" where "calling page or tag" is meant, and I think that it is only for brevity. The full sentence with "or tag" or "/tag" is usually written several lines below such a fragment.

E.g. JSP.5.12.3 that Mark cited in comment 1, continues with "The container
must then generate code to synchronize the page scope values for the variable in
the tag file with the page scope equivalent in the calling page or tag file."

Thus, the NESTED and AT_BEGIN variables update the page scope of the calling page or tag. It is said in JSP.5.12.3 (page "1-114"), in JSP.8.9 (page "1-186"), and elsewhere.

That explains why the first patch that breaks TCK is incorrect.

It might require a review by an editor to lessen such omissions, especially in the first sentences of a chapter and in tables.

2. Each JSP Fragment is associated with a JspContext of that page or tag in context of which it is defined.

It is first mentioned in JSP.5.12.1 (page "1-113") and explained in detail in Part II in chapter "7. JSP Fragments" in description of package "javax.servlet.jsp.tagext" in Part II (pages "2-77" through "2-79").

I find it understandable, because the page author may reference any other variable of the page, and those are of no concern for the tag authors.

This said, here is a solution for the recursive.tag file: redeclare the JSP fragment. That is, patching recursive.tag:

- <x:recursive content="${content}" depth="${depth-1}"/>
+ <x:recursive depth="${depth-1}">
+   <jsp:attribute name="content">
+     <jsp:invoke fragment="content"/>
+   </jsp:attribute>
+ </x:recursive>

--
A small thing that was bugging me was how this redeclaration will work in view of JSP.5.12.1 phrase that says "the results will be sent to the JspWriter of the JspContext associated with the JspFragment." and how it plays along with specifying an explicit Writer (var or varReader attributes of jsp:invoke action).

That is: how invoking a fragment from a fragment will work if the writer for the outer fragment was specified explicitly, but not for the inner fragment, and the inner fragment belongs to the parent page?
That is, will it bypass it and write to the page? The answer is that it would not. That is good.
As implemented in Tomcat, the JspWriter in not stored in JspContext of the tag, but in the one of the page. Thus, specifying an explicit writer (context.pushBody() call) is delegated to the page, thus affecting all the tags. Thus, all fragments in the same response do write to the same writer.
I do not know where in the specification it comes from (I think it should be written somewhere when defining a "JSP Context Wrapper"), but it works good.

The patched tag file works in Tomcat but not on Resin.

http://bugs.caucho.com/view.php?id=3228

To me, this code looks like it should evaluate the fragment and pass the result to the recursive invocation:

<x:recursive depth="${depth-1}">
  <jsp:attribute name="content">
    <jsp:invoke fragment="content"/>
  </jsp:attribute>
</x:recursive>

Apparently it passes the reference to the fragment instead. Strange...

Konstantin,

Agreed with you on your reading of the spec on both points.  However, I think the  current tag file (without your patch) can be made to work if we make the following modification to the spec:

   Before the invocation of the fragment, AT_BEGIN and NESTED variables are
   copied from the current JspContext to the JspContext of the fragment
   (**instead** of the JspContext of the calling page or tag file).

The JspContext of the fragment and that of the calling page or tag file is the same for first level calls, but would be different for 2nd level calls, when the fragment is passed from the called tag file to another tag file.  It really doesn't make sense to copy variables to the calling page before fragment invocation when the fragment is not defined in the calling page.  Making such a change would correct such oversight in the spec.  My patch reflects such a change.

I'll make sure that such change gets into the JSP MR or next spec, if there is no objections.

Your patched tag file works, because you are essentially chaining the fragment invocations, so there is a lot of copying going on here.

Thanks for the patch. I have applied it to trunk and proposed it for 6.0.x.

I reread the spec on this issue carefully, and after some consideration, I've come to the conclusion that it is best that we don't "fix" the spec and the implementation now.

The original intend of the spec is to set the variables in the calling page/tag before the invocation of the fragment.  This is problematic, since that means that fragments passed from one tag file to another cannot communicate with the original caller using variables, as demonstrated in the case here.  From the paragraphs on page 2-78, the original spec author seemed to know this.

The way the variables are synchronized between the calling page and the tag files  is reminiscent of passing information in global variables and we all know that global variables are evil.  Any attempt for a simple fix to one problem will cause problems in another area.  My proposed fix for this problem will cause problems with the save and restore operations at the tag entry and exit.  What is needed is a complete redesign of the way calling page and tag files communicate. The is of course impractical or impossible at this time.

Therefore I am withdrawing my patch.  Mark, please revert your changes. Sorry for the inconvenience.

No problem. Patch reverted.