New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
stiemannkj1 opened this Issue Mar 29, 2018 · 14 comments

Comments

Projects
None yet
4 participants
@stiemannkj1
Collaborator

stiemannkj1 commented Mar 29, 2018

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 stiemannkj1 changed the title from The wrong content type of "text/html" is set when an Ajax request with execute "@all" is used to The wrong content type of "text/html" is set for Ajax request with execute "@all" Mar 29, 2018

@stiemannkj1 stiemannkj1 changed the title from The wrong content type of "text/html" is set for Ajax request with execute "@all" to The wrong content type of "text/html" is set for execute "@all" Ajax response Mar 29, 2018

@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Mar 29, 2018

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

@stiemannkj1 stiemannkj1 changed the title from The wrong content type of "text/html" is set for execute "@all" Ajax response to The wrong content type of "text/html" is set for execute "@all" Ajax responses Mar 29, 2018

@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Mar 29, 2018

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

@stiemannkj1 stiemannkj1 self-assigned this Mar 30, 2018

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Mar 30, 2018

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Mar 30, 2018

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Mar 30, 2018

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Mar 30, 2018

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Mar 30, 2018

@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Mar 30, 2018

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: #4359

@arjantijms

This comment has been minimized.

Collaborator

arjantijms commented Mar 30, 2018

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/

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Apr 3, 2018

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Apr 3, 2018

stiemannkj1 added a commit to stiemannkj1/mojarra that referenced this issue Apr 3, 2018

@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Apr 3, 2018

@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.

@wsaca

This comment has been minimized.

wsaca commented Jun 4, 2018

@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?

@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Jun 4, 2018

@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

This comment has been minimized.

Collaborator

stiemannkj1 commented Jun 4, 2018

@lynx12, perhaps you need to make a change similar to the one the joinfaces folks made here: joinfaces/joinfaces-maven-jar-example#158

@wsaca

This comment has been minimized.

wsaca commented Jun 5, 2018

@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.

@jacekzwroclawia

This comment has been minimized.

jacekzwroclawia commented Nov 16, 2018

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.

@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Nov 19, 2018

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

@stiemannkj1 stiemannkj1 reopened this Nov 19, 2018

stiemannkj1 added a commit that referenced this issue Nov 20, 2018

Fix #4388 Scripts with CDATA cause "empty response" error on Ajax render
Revert "Fix #4358: The wrong content type of "text/html" is set for execute "@ALL" Ajax responses"

This reverts commit ee3671d.

stiemannkj1 added a commit that referenced this issue Nov 20, 2018

Fix #4388 Scripts with CDATA cause "empty response" error on Ajax render
Revert "Fix #4358: The wrong content type of "text/html" is set for execute "@ALL" Ajax responses"

This reverts commit 8a5a562.
@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Nov 20, 2018

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:

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");
@wsaca

This comment has been minimized.

wsaca commented Nov 20, 2018

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

@stiemannkj1

This comment has been minimized.

Collaborator

stiemannkj1 commented Nov 20, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment