[
https://issues.apache.org/jira/browse/MYFACES-1820?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12792419#action_12792419
]
Simon Kitching commented on MYFACES-1820:
-----------------------------------------
Yes, Leonardo's posting shows the latest code.
I'm not sure Leonardo's suggested change is needed though. The whole point of
the patch I applied is that we can have this situation:
custom FacesContext "decorator" object #1 (eg from Orchestra)
--> custom FacesContext "decorator object #2" (eg from PrettyFaces) [1]
--> the "base" implementation (eg the standard MyFaces FacesContextImpl
object)
There are two situations when a decorator class wants to do some of its own
logic *then* delegate to the next instance in the chain:
(a) when it wants to do something *in addition to the standard logic*
(b) when the class wants to be useable with versions of JSF earlier than the
one it was compiled with; in this case it needs to provide "stubs" for new
methods, which just delegate to the wrapped instance.
The custom subclasses of FacesContext are supposed to just call the
corresponding method on the super-class (FacesContext), and let that delegate
to the wrapped instance. However in the original code, what was delegated to
was whatever object setCurrentInstance() had been called on. Obviously, when
the wrapper has set itself as the "current instance" a loop occurs [2].
Unfortunately, JSF makes finding the "wrapped" instance very difficult. The
current code (this _firstInstance stuff) just returns the "base" implementation
every time, which means it can skip objects in the middle of the chain. But it
is better than nothing.
Leonardo's suggested check for
if (ctx == this)
should not be necessary as far as I can see, because "ctx" will always be the
"base" implementation, and that should never call methods on the superclass;
the "base" implementation is required to override the getELContext method in
the FacesContext class with a proper implementation.
[1] I'm not 100% sure that PrettyFaces customises FacesContext, but certainly
some libraries out there do. Sorry, I've forgotten exactly which ones
definitely do it..
[2] I guess that the JSF designers had assumed that the ability to customise
the FacesContextFactory would be used only to completely replace a FacesContext
implementation, rather than to write a decorator. But FacesContext decorators
are very useful - in fact, critical in some cases. And several libraries out
there already use them.
> Infinite loop can occur when custom FacesContext subclass compiled against
> JSF1.1 but used with JSF1.2
> ------------------------------------------------------------------------------------------------------
>
> Key: MYFACES-1820
> URL: https://issues.apache.org/jira/browse/MYFACES-1820
> Project: MyFaces Core
> Issue Type: Bug
> Components: JSR-252
> Affects Versions: 1.2.2
> Reporter: Simon Kitching
> Attachments: FacesContext.patch.txt, patch-1820.txt
>
>
> The problem is method FacesContext.getELContext. JSF1.2 added a method to
> this base class that was not there in JSF1.1. This makes life difficult for
> existing JSF1.1 code that already subclasses that class.
> A default concrete implementation needs to exist, in order not to break
> existing JSF1.1 code, but (a) the current one gets it wrong, and (b) defining
> a correct one is difficult (impossible?)
> (1) Stan Silvert initially defined this method like this:
> // The following concrete method was added for JSF 1.2.
> // It supplies a default
> // implementation that throws UnsupportedOperationException.
> // This allows old FacesContext implementations to still work.
> public ELContext getELContext() {
> throw new UnsupportedOperationException();
> }
> (2) Dennis Byrne changed it to its current form:
> public ELContext getELContext() {
> FacesContext ctx = getCurrentInstance();
> if (ctx == null)
> throw new NullPointerException(FacesContext.class.getName());
> ELContext elctx = ctx.getELContext();
> if (elctx == null)
> throw new UnsupportedOperationException();
> return elctx;
> }
> However (2) assumes that custom subclasses never set themselves as the
> current instance, instead only ever *delegating* to the "real" instance.
> If someone's custom subclass of FacesContext ever calls
> setCurrentInstance(this), then an infinite loop will occur here.
> And in fact, this is just what we get:
> java.lang.StackOverflowError
> at java.lang.ThreadLocal$ThreadLocalMap.getEntry(ThreadLocal.java:357)
> at java.lang.ThreadLocal$ThreadLocalMap.access$000(ThreadLocal.java:242)
> at java.lang.ThreadLocal.get(ThreadLocal.java:127)
> at
> javax.faces.context.FacesContext.getCurrentInstance(FacesContext.java:98)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:35)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
> at javax.faces.context.FacesContext.getELContext(FacesContext.java:40)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
