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

Prevent FaceletContext.FACELET_CONTEXT_KEY constant to be inlined by compiler #1257

Closed
glassfishrobot opened this Issue Jan 27, 2014 · 12 comments

Comments

Projects
None yet
2 participants
@glassfishrobot

glassfishrobot commented Jan 27, 2014

Affected Versions

[2.3]

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Jan 27, 2014

Reported by lfryc

glassfishrobot commented Jan 27, 2014

Reported by lfryc

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot commented Jan 27, 2014

Issue-Links:
depends on
JAVASERVERFACES-3372

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Jan 27, 2014

lfryc said:
I can't see the description of the issue even though I added it.


Re-posting:

In RichFaces 5 when we compile ActionListenerHandler against Mojarra 2.2.5 and then run the code against older 2.1 release (2.1.7), we get NullPointerException.

The issue is well described in RF-13472.

The problem here is that a value of FACELET_CONTEXT_KEY contstant is inlined by compiler, but the value has changed between Mojarra 2.1 ("com.sun.faces.facelets.FACELET_CONTEXT") and 2.2 ("javax.faces.FACELET_CONTEXT"). This means the code compiled against one 2.2 won't work on 2.1 and vice versa.


What we can do is prevent compiler from inlining the FACELET_CONTEXT_KEY constant.

As suggested on stackoverflow, it is possible to use String#intern() or String#toString():

public static final String FACELET_CONTEXT_KEY = 
            "javax.faces.FACELET_CONTEXT".intern();

http://stackoverflow.com/questions/377819/are-all-compile-time-constants-inlined
http://stackoverflow.com/questions/1833581/when-to-use-intern-on-string-literals


I believe the fix should go to 2.1.x branch as well.

glassfishrobot commented Jan 27, 2014

lfryc said:
I can't see the description of the issue even though I added it.


Re-posting:

In RichFaces 5 when we compile ActionListenerHandler against Mojarra 2.2.5 and then run the code against older 2.1 release (2.1.7), we get NullPointerException.

The issue is well described in RF-13472.

The problem here is that a value of FACELET_CONTEXT_KEY contstant is inlined by compiler, but the value has changed between Mojarra 2.1 ("com.sun.faces.facelets.FACELET_CONTEXT") and 2.2 ("javax.faces.FACELET_CONTEXT"). This means the code compiled against one 2.2 won't work on 2.1 and vice versa.


What we can do is prevent compiler from inlining the FACELET_CONTEXT_KEY constant.

As suggested on stackoverflow, it is possible to use String#intern() or String#toString():

public static final String FACELET_CONTEXT_KEY = 
            "javax.faces.FACELET_CONTEXT".intern();

http://stackoverflow.com/questions/377819/are-all-compile-time-constants-inlined
http://stackoverflow.com/questions/1833581/when-to-use-intern-on-string-literals


I believe the fix should go to 2.1.x branch as well.

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot commented Jan 27, 2014

File: changebundle.txt
Attached By: @manfredriem

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Jan 27, 2014

@manfredriem said:
Lukas, I have attached the change bundle and once review has done I will commit it, then when it passes the 2.2 TCK I'll will close this issue. For the 2.1 work please file a back port task pointing back to this issue. Thanks!

glassfishrobot commented Jan 27, 2014

@manfredriem said:
Lukas, I have attached the change bundle and once review has done I will commit it, then when it passes the 2.2 TCK I'll will close this issue. For the 2.1 work please file a back port task pointing back to this issue. Thanks!

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Jan 28, 2014

@manfredriem said:
Applied to 2.2 branch,

svn commit -m "Fixes https://java.net/jira/browse/JAVASERVERFACES-3155, r=edburns, intern the FACELET_CONTEXT_KEY so it does not get inlined."
Sending jsf-api/src/main/java/javax/faces/view/facelets/FaceletContext.java
Transmitting file data .
Committed revision 12799.

Once the 2.2 TCK passes I will go ahead and close the issue. Thanks!

glassfishrobot commented Jan 28, 2014

@manfredriem said:
Applied to 2.2 branch,

svn commit -m "Fixes https://java.net/jira/browse/JAVASERVERFACES-3155, r=edburns, intern the FACELET_CONTEXT_KEY so it does not get inlined."
Sending jsf-api/src/main/java/javax/faces/view/facelets/FaceletContext.java
Transmitting file data .
Committed revision 12799.

Once the 2.2 TCK passes I will go ahead and close the issue. Thanks!

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Jan 29, 2014

@manfredriem said:
Applied to 2.2 branch,

svn commit -m "Reverting change as it breaks the TCK"
Sending jsf-api/src/main/java/javax/faces/view/facelets/FaceletContext.java
Transmitting file data .
Committed revision 12804.

glassfishrobot commented Jan 29, 2014

@manfredriem said:
Applied to 2.2 branch,

svn commit -m "Reverting change as it breaks the TCK"
Sending jsf-api/src/main/java/javax/faces/view/facelets/FaceletContext.java
Transmitting file data .
Committed revision 12804.

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Jan 29, 2014

@manfredriem said:
Unfortunately as the proposed change breaks the TCK I have moved it to the spec issue tracker to put it on the table for 2.3.

glassfishrobot commented Jan 29, 2014

@manfredriem said:
Unfortunately as the proposed change breaks the TCK I have moved it to the spec issue tracker to put it on the table for 2.3.

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Aug 1, 2014

@edburns said:
Set priority to baseline ahead of JSF 2.3 triage. Priorities will be assigned accurately after this exercise.

glassfishrobot commented Aug 1, 2014

@edburns said:
Set priority to baseline ahead of JSF 2.3 triage. Priorities will be assigned accurately after this exercise.

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Aug 1, 2014

@manfredriem said:
Setting priority to Critical

glassfishrobot commented Aug 1, 2014

@manfredriem said:
Setting priority to Critical

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot Aug 19, 2014

Marked as fixed on Tuesday, August 19th 2014, 8:27:55 am

glassfishrobot commented Aug 19, 2014

Marked as fixed on Tuesday, August 19th 2014, 8:27:55 am

@glassfishrobot

This comment has been minimized.

Show comment
Hide comment
@glassfishrobot

glassfishrobot May 5, 2017

This issue was imported from java.net JIRA JAVASERVERFACES_SPEC_PUBLIC-1257

glassfishrobot commented May 5, 2017

This issue was imported from java.net JIRA JAVASERVERFACES_SPEC_PUBLIC-1257

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