mojarra icon indicating copy to clipboard operation
mojarra copied to clipboard

The wrong content type of "text/html" is set for execute "@all" Ajax responses

Open stiemannkj1 opened this issue 7 years ago • 25 comments

Steps to reproduce:

  1. Clone the wrong-content-type-ajax-excecute-all-reproducer project:

    git clone https://github.com/stiemannkj1/wrong-content-type-ajax-excecute-all-reproducer.git
    
  2. Build the project:

    cd wrong-content-type-ajax-excecute-all-reproducer && mvn clean package
    
  3. Deploy the project to Tomcat:

    cp target/*.war $TOMCAT_HOME/webapps/wrong-content-type-ajax-excecute-all-reproducer.war
    
  4. Navigate to the deployed webapp at http://localhost:8080/wrong-content-type-ajax-excecute-all-reproducer/.

  5. Note that the External Context Calls: show that setResponseContentType("text/html") was correctly called before the first call to getResponseOutputWriter().

  6. Click the Execute (default) Ajax Request button.

  7. Note that the External Context Calls: show that setResponseContentType("text/xml") was correctly called before the first call to getResponseOutputWriter().

  8. Click the Execute @all Ajax Request button.

If the bug still exists, the External Context Calls: will show that setResponseContentType("text/html") was incorrectly called before the first call to getResponseOutputWriter().

If the bug is fixed, the External Context Calls: will show that setResponseContentType("text/xml") was called immediately before the first call to getResponseOutputWriter().

Additional Information:

This issue does not affect Servlets but it does affect portlets which do not allow changing the content type after the first call to ExternalContext.getResponseOutputWriter() (Portlet Spec 3.0 Section 15.5.1 Content Type):

The setContentType method must be called before the getWriter or getPortletOutputStream methods. Otherwise, the method will have no effect.

stiemannkj1 avatar Mar 29 '18 16:03 stiemannkj1

A similar issue (but worse since it affects all Ajax responses) exists in MyFaces: https://issues.apache.org/jira/browse/MYFACES-4214.

stiemannkj1 avatar Mar 29 '18 16:03 stiemannkj1

To work around this issue in portlets, avoid using execute="@all".

stiemannkj1 avatar Mar 29 '18 16:03 stiemannkj1

This issue has been fix in 2.3.x and 2.2.x. It still needs to be fixed in 2.4.x/master, but there is a code freeze right now. Once master can be changed, this PR should be merged: https://github.com/javaserverfaces/mojarra/pull/4359

stiemannkj1 avatar Mar 30 '18 19:03 stiemannkj1

Once master can be changed, this PR should be merged: #4359

I'm not sure if that branch will ever be unfrozen, or directly be set to read-only/archived mode.

As Mojarra will be transferred to Eclipse all development will continue there. At a minimum there won't be any more releases from the repos here, as the final release has just been done: https://maven.java.net/content/repositories/releases/org/glassfish/javax.faces/2.4.0/

arjantijms avatar Mar 30 '18 21:03 arjantijms

@arjantijms, thanks. My comment wasn't clear, but I meant that the commits could be merged into whatever version of Mojarra is given to the Eclipse foundation.

stiemannkj1 avatar Apr 03 '18 17:04 stiemannkj1

@stiemannkj1 If I set the execute attribute to "@all" my code is not working. That's correct? "@all" should have the same behavior like "@form", but that's not happening and I'm getting this error "java.lang.IllegalArgumentException: Unrecognized Content Type". This change was released in 2.3.4 and 2.3.5 but not in 2.4.0. Could you clarify this?

wsaca avatar Jun 04 '18 21:06 wsaca

@Lynx12, it's possible that I broke something with this change, could you provide more details about your app and the stack trace for the IllegalArgumentException?

stiemannkj1 avatar Jun 04 '18 22:06 stiemannkj1

@Lynx12, perhaps you need to make a change similar to the one the joinfaces folks made here: https://github.com/joinfaces/joinfaces-maven-jar-example/pull/158

stiemannkj1 avatar Jun 04 '18 22:06 stiemannkj1

@stiemannkj1 the fix was easy to apply, but not so quick to detect because the template is added as a dependency. Thanks for guiding me to the solution.

wsaca avatar Jun 05 '18 14:06 wsaca

After commits connected with this issue there is problems with <p:commandButton /> (tested on Primeface 4.0, but I think it is conneted with all versions). <p:commandButton update=".." /> generates Ajax request that is handled as a partial request, so inside method RenderKitImpl::createResponseWriter we get contentType="text/html" instead of contentType="application/xhtml+xml" as it used to be causing e.g. HtmlResponseWriter incorrectly handling <![CDATA[...]]> in <script> tags.

jacekzwroclawia avatar Nov 16 '18 13:11 jacekzwroclawia

@jacekzwroclawia, I've opened #4388 to track this issue.

stiemannkj1 avatar Nov 19 '18 23:11 stiemannkj1

I've reverted the commits that fix this issue since they caused so many issues, but that will cause the original issue to reappear. So if anyone wants to fix it in the future, here are the commits that fix #4358:

  • https://github.com/javaserverfaces/mojarra/commit/8a5a562e129a31f3655de86cfeb79c33d3eaa1f8
  • https://github.com/javaserverfaces/mojarra/commit/ee3671db95986395e3a40a591c52522421b25bf2

You can apply this patch on top of those commits in order to avoid #4388 (although you should probably also add a regression tester for #4358 to be sure):

diff --git a/jsf-ri/src/main/java/com/sun/faces/renderkit/html_basic/HtmlResponseWriter.java b/jsf-ri/src/main/java/com/sun/faces/renderkit/html_basic/HtmlResponseWriter.java
index f5198d7004..5ec2f26d96 100644
--- a/jsf-ri/src/main/java/com/sun/faces/renderkit/html_basic/HtmlResponseWriter.java
+++ b/jsf-ri/src/main/java/com/sun/faces/renderkit/html_basic/HtmlResponseWriter.java
@@ -55,6 +55,7 @@ import com.sun.faces.RIConstants;
 import com.sun.faces.config.WebConfiguration;
 import com.sun.faces.config.WebConfiguration.BooleanWebContextInitParameter;
 import com.sun.faces.io.FastStringWriter;
+import com.sun.faces.renderkit.RenderKitUtils;
 import com.sun.faces.util.HtmlUtils;
 import com.sun.faces.util.MessageUtils;
 import java.util.Map;
@@ -134,7 +135,7 @@ public class HtmlResponseWriter extends ResponseWriter {
     private boolean isPartial;
 
     // flag to indicate if the content type is XHTML
-    private boolean isXhtml;
+    private boolean isXml;
 
     // HtmlResponseWriter to use when buffering is required
     private Writer origWriter;
@@ -478,8 +479,7 @@ public class HtmlResponseWriter extends ResponseWriter {
             dontEscape = false;
         }
 
-        isXhtml = getContentType().equals(
-            RIConstants.XHTML_CONTENT_TYPE);
+        isXml = RenderKitUtils.isXml(getContentType());
 
         if (isScriptOrStyle(name)
              && !scriptOrStyleSrc
@@ -489,7 +489,7 @@ public class HtmlResponseWriter extends ResponseWriter {
 
             if (result != null) {
                 String trim = result.trim();
-                if (isXhtml) {
+                if (isXml) {
                     if (isScript) {
                         Matcher
                             cdataStartSlashSlash =
@@ -548,7 +548,7 @@ public class HtmlResponseWriter extends ResponseWriter {
                     }
                 }
             }
-            if (isXhtml) {
+            if (isXml) {
                 if (!writingCdata) {
                     if (isScript) {
                         writer.write("\n//]]>\n");
@@ -1130,9 +1130,8 @@ public class HtmlResponseWriter extends ResponseWriter {
             writer.write('>');
             closeStart = false;
             if (isScriptOrStyle() && !scriptOrStyleSrc) {
-                isXhtml = getContentType().equals(
-                     RIConstants.XHTML_CONTENT_TYPE);
-                if (isXhtml) {
+                isXml = RenderKitUtils.isXml(getContentType());
+                if (isXml) {
                     if (!writingCdata) {
                         if (isScript) {
                             writer.write("\n//<![CDATA[\n");

stiemannkj1 avatar Nov 20 '18 00:11 stiemannkj1

I don't have any problem with JSF 2.3, what version will this change be released?

wsaca avatar Nov 20 '18 01:11 wsaca

@wsaca, the change that caused regressions was only introduced int 2.2.17 so it only exists in 2.2.17 and 2.2.18. The change was never applied to 2.3.x, so no issues exist there.

stiemannkj1 avatar Nov 20 '18 01:11 stiemannkj1

@stiemannkj1 so when there will be new release of 2.2 version with reverted commits?

jacekzwroclawia avatar Feb 01 '19 15:02 jacekzwroclawia

@jacekzwroclawia, that's up to Oracle.

cc @ren-zhijun-oracle, @ruolli, @xinyuan-zhang, @edburns, @manfredriem

stiemannkj1 avatar Feb 01 '19 16:02 stiemannkj1

@stiemannkj1 Hi, Kyle,

I just noticed you ever submitted the fix for #4358 to branch : "MOJARRA_2_3X_ROLLING" in repository: "https://github.com/javaserverfaces/mojarra.git" . Would you help revert the fix from "MOJARRA_2_3X_ROLLING" as you did for "MOJARRA_2_2X_ROLLING" ?

Thanks. Ruolin

ruolli avatar Mar 28 '19 08:03 ruolli

@ruolli, I just reverted the commits from MOJARRA_2_3X_ROLLING.

stiemannkj1 avatar Mar 28 '19 14:03 stiemannkj1

@stiemannkj1 Thanks a lot, Kyle.

ruolli avatar Mar 28 '19 14:03 ruolli

Do I understand it correctly?

  1. 2.3+ will not be published from this repository/by Donor, but rather by Donee, and changes pushed to MOJARRA_2_3X_ROLLING/master - if not ported - will not be visible for Clients of Mojarra 2.3.9+
  2. #4359 wasn't merged to master and thus fix was also not donated

pzygielo avatar Mar 28 '19 14:03 pzygielo

@pzygielo 1 ... Yes, 2.3+ would be released from "github.com/eclipse-ee4j/mojarra" , instead of from this repository : "github.com/javaserverfaces/mojarra" . Meanwhile, legacy releases, including 2.3, would be released from here. Generally speaking, they are separate repositories, consequently, porting is necessary for certain fix to be merged from one to the other.

2... Seemingly, there was no submission via #4359.

Thanks. Ruolin

ruolli avatar Mar 28 '19 15:03 ruolli

Still not 100% clear for me...

Yes, 2.3+ would be released from "github.com/eclipse-ee4j/mojarra" , instead of from this repository : "github.com/javaserverfaces/mojarra" . Meanwhile, legacy releases, including 2.3, would be released from here.

Eclipse released 2.3.9, by 2.3+ I meant ANY future 2.3 release. Shall it be 2.3.9+?

Do you - by legacy releases including 2.3 mean (for 2.3) patched 2.3.8 like 2.3.8-1 or something like that?

pzygielo avatar Mar 28 '19 15:03 pzygielo

@pzygielo

By saying 2.3+, I actually meant Mojarra 2.4 or Mojarra 3.0 in future. Those releases would be released from "github.com/eclipse-ee4j/mojarra" . And, the artifact would be renamed to jakarta.faces , instead of javax.faces .

As to 2.3.* , they are maintained in "github.com/javaserverfaces/mojarra" "MOJARRA_2_3X_ROLLING" . And, 2.3.9 , 2.3.10 ... would be released from here.

Thanks. Ruolin

ruolli avatar Mar 28 '19 15:03 ruolli

Thanks for your clarifications, @ruolli (I'm aware of groupId/artifactId renames, thank you.)

pzygielo avatar Mar 28 '19 16:03 pzygielo

@pzygielo @ruolli @arjantijms

As far as I understand it (please confirm Arjan)

Oracle Mojarra supported releases will be coming from the github.com/javaserverfaces/mojarra for 2.3 and earlier.

Eclipse Mojarra supported and branded releases will be coming from github.com/eclipse-ee4j/mojarra for 2.3 and later

manfredriem avatar Mar 29 '19 14:03 manfredriem