MyFaces Core
  1. MyFaces Core
  2. MYFACES-1820

Infinite loop can occur when custom FacesContext subclass compiled against JSF1.1 but used with JSF1.2

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.2.2
    • Fix Version/s: None
    • Component/s: JSR-252
    • Labels:
      None

      Description

      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)

      1. FacesContext.patch.txt
        5 kB
        Simon Kitching
      2. patch-1820.txt
        3 kB
        Simon Kitching

        Activity

        Hide
        Simon Kitching added a comment -

        A patch that somewhat improves the situation by at least throwing a useful exception rather than entering an infinite loop.
        This is not a "fix" though..

        Show
        Simon Kitching added a comment - A patch that somewhat improves the situation by at least throwing a useful exception rather than entering an infinite loop. This is not a "fix" though..
        Hide
        Simon Kitching added a comment -

        Ok, here's a proper patch for this issue. Rather than trying to delegate to the most-recently-registered FacesContext instance, delegate to the first-registered instance instead.

        This means that when there is a chain of FacesContext wrappers, and one or more of them was compiled against JSF1.1 then earlier instances that override getELContext will not get their overriding method invoked. However this is not likely to cause problems; very few custom FacesContext classes will want to intercept that method.

        The positive side is that the first registered one (ie the "real" implementation) will always return a valid ELContext object, and we have no danger of infinite loops as with the current code. So at least it is better than the current implementation.

        In addition, I think this is what Sun Mojarra does. Their implementation of getELContext actually looks up key "com.sun.faces.FacesContextImpl" in request-scope and delegates to that object. I haven't tracked down where that property is set, but would bet a cold beer or two that it refers to the first-created instance for the request, ie the "real" instance.

        Show
        Simon Kitching added a comment - Ok, here's a proper patch for this issue. Rather than trying to delegate to the most-recently-registered FacesContext instance, delegate to the first-registered instance instead. This means that when there is a chain of FacesContext wrappers, and one or more of them was compiled against JSF1.1 then earlier instances that override getELContext will not get their overriding method invoked. However this is not likely to cause problems; very few custom FacesContext classes will want to intercept that method. The positive side is that the first registered one (ie the "real" implementation) will always return a valid ELContext object, and we have no danger of infinite loops as with the current code. So at least it is better than the current implementation. In addition, I think this is what Sun Mojarra does. Their implementation of getELContext actually looks up key "com.sun.faces.FacesContextImpl" in request-scope and delegates to that object. I haven't tracked down where that property is set, but would bet a cold beer or two that it refers to the first-created instance for the request, ie the "real" instance.
        Hide
        Simon Kitching added a comment -

        Actually, this problem exists for every class in the JSF spec that is meant to be decorated. They all share this common design flaw.

        The solution attached for FacesContext works, but cannot be applied to other classes (they do not have a setInstance method). It would be nice to have a consistent solution for every decorated class.

        One solution would be for the "impl" classes for all of these decoratable classes to always set a request-scoped var when they are created, called

        {class}

        :DFLT_INSTANCE or similar. Then the api base class for each of these can look for the request-scoped var and delegate.

        Show
        Simon Kitching added a comment - Actually, this problem exists for every class in the JSF spec that is meant to be decorated. They all share this common design flaw. The solution attached for FacesContext works, but cannot be applied to other classes (they do not have a setInstance method). It would be nice to have a consistent solution for every decorated class. One solution would be for the "impl" classes for all of these decoratable classes to always set a request-scoped var when they are created, called {class} :DFLT_INSTANCE or similar. Then the api base class for each of these can look for the request-scoped var and delegate.
        Hide
        Ashish Jain added a comment -

        How is that this problem can be recreated??

        Show
        Ashish Jain added a comment - How is that this problem can be recreated??
        Hide
        Ashish Jain added a comment -

        Any idea on a sample application will be really helpful.

        Show
        Ashish Jain added a comment - Any idea on a sample application will be really helpful.
        Hide
        Simon Kitching added a comment -

        For FacesContext subclasses, this issue could originally be triggered by:
        [1] create class CustomFacesContextFactory which returns a new CustomFacesContext instance
        [2] define class CustomFacesContext, and have its constructor call FacesContext.setCurrentInstance(this)
        (just as the default FacesContextImpl class does)
        [3] configure the faces-config.xml so that this factory is used.
        then accessing any standard jsf page. The faces contexts were created, and then when an EL expression is found in the page, JSF called FacesContext.getCurrentInstance().getELContext() and an infinite loop immediately occured.

        I committed the attached patch for the FacesContext class quite a while ago; see r634007. This is a partial fix, in that it does avoid infinite loops. However it does not properly delegate to the "wrapped" instance; instead, it delegates directly to the "last" instance in the chain of wrapped classes.

        The issue is not closed because
        (a) the solution is not right when multiple layers of wrappers exist, and
        (b) the same issue exists for JSF "decorator" classes other than FacesContext.

        It's been quite a while since I wrote this issue, so I can't remember exactly which other classes have problems. But they are those classes that may want to delegate to an "underlying" implementation (ie implement the decorator pattern), but JSF provides no way to locate that underlying implementation. I think they include:

        • Lifecycle
        • RenderKit
        • NavigationHandler?
        Show
        Simon Kitching added a comment - For FacesContext subclasses, this issue could originally be triggered by: [1] create class CustomFacesContextFactory which returns a new CustomFacesContext instance [2] define class CustomFacesContext, and have its constructor call FacesContext.setCurrentInstance(this) (just as the default FacesContextImpl class does) [3] configure the faces-config.xml so that this factory is used. then accessing any standard jsf page. The faces contexts were created, and then when an EL expression is found in the page, JSF called FacesContext.getCurrentInstance().getELContext() and an infinite loop immediately occured. I committed the attached patch for the FacesContext class quite a while ago; see r634007. This is a partial fix, in that it does avoid infinite loops. However it does not properly delegate to the "wrapped" instance; instead, it delegates directly to the "last" instance in the chain of wrapped classes. The issue is not closed because (a) the solution is not right when multiple layers of wrappers exist, and (b) the same issue exists for JSF "decorator" classes other than FacesContext. It's been quite a while since I wrote this issue, so I can't remember exactly which other classes have problems. But they are those classes that may want to delegate to an "underlying" implementation (ie implement the decorator pattern), but JSF provides no way to locate that underlying implementation. I think they include: Lifecycle RenderKit NavigationHandler?
        Hide
        Simon Kitching added a comment -

        This problem was noticed during the implementation of the Orchestra library. We wanted to "decorate" a number of standard classes, to add our own logic to the normal processing. But it was then impossible to find the "wrapped" instance to delegate to.

        Show
        Simon Kitching added a comment - This problem was noticed during the implementation of the Orchestra library. We wanted to "decorate" a number of standard classes, to add our own logic to the normal processing. But it was then impossible to find the "wrapped" instance to delegate to.
        Hide
        Ashish Jain added a comment -

        HI Simon, Thanks for your generous reply. I hope you mean if there is a subclass which subclasses FacesContext and is not correctly implemented as expected we may still hit the error.

        I was going through rev #634007. I am just trying to understand the following part of the code.

        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; }

        I see that getELContext() is repeatedly being called w/o any return condition. Can you please advice if this is correct?

        Show
        Ashish Jain added a comment - HI Simon, Thanks for your generous reply. I hope you mean if there is a subclass which subclasses FacesContext and is not correctly implemented as expected we may still hit the error. I was going through rev #634007. I am just trying to understand the following part of the code. 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; } I see that getELContext() is repeatedly being called w/o any return condition. Can you please advice if this is correct?
        Hide
        Leonardo Uribe added a comment -

        Just as a side note, the latest code in trunk looks like this:

        public ELContext getELContext()
        {
        // Do NOT use getCurrentInstance here. For FacesContext decorators that
        // register themselves as "the current instance" that will cause an
        // infinite loop. For FacesContext decorators that do not register
        // themselves as "the current instance", if they are themselves wrapped
        // by a decorator that does register itself, then an infinite loop
        // also occurs.
        //
        // As noted above, we really want to do something like
        // ctx = getWrappedInstance();
        // where the subclass can return the object it is delegating to.
        // As there is no such method, however, the best we can do is pass the
        // method call on to the first-registered FacesContext instance. That
        // instance will never be "this", as the real original FacesContext
        // object will provide a proper implementation of this method.
        FacesContext ctx = _firstInstance.get();

        if (ctx == null)

        { throw new NullPointerException(FacesContext.class.getName()); }

        ELContext elctx = ctx.getELContext();

        if (elctx == null)

        { throw new UnsupportedOperationException(); }

        return elctx;
        }

        maybe we could add an check for equals to prevent the infinite loop:

        if (ctx == this)

        { throw new UnsupportedOperationException(); }

        I remember this error usually happens when FacesContext instances not implementing this method are called, but the current version should works.

        Show
        Leonardo Uribe added a comment - Just as a side note, the latest code in trunk looks like this: public ELContext getELContext() { // Do NOT use getCurrentInstance here. For FacesContext decorators that // register themselves as "the current instance" that will cause an // infinite loop. For FacesContext decorators that do not register // themselves as "the current instance", if they are themselves wrapped // by a decorator that does register itself, then an infinite loop // also occurs. // // As noted above, we really want to do something like // ctx = getWrappedInstance(); // where the subclass can return the object it is delegating to. // As there is no such method, however, the best we can do is pass the // method call on to the first-registered FacesContext instance. That // instance will never be "this", as the real original FacesContext // object will provide a proper implementation of this method. FacesContext ctx = _firstInstance.get(); if (ctx == null) { throw new NullPointerException(FacesContext.class.getName()); } ELContext elctx = ctx.getELContext(); if (elctx == null) { throw new UnsupportedOperationException(); } return elctx; } maybe we could add an check for equals to prevent the infinite loop: if (ctx == this) { throw new UnsupportedOperationException(); } I remember this error usually happens when FacesContext instances not implementing this method are called, but the current version should works.
        Hide
        Simon Kitching added a comment -

        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.

        Show
        Simon Kitching added a comment - 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.

          People

          • Assignee:
            Unassigned
            Reporter:
            Simon Kitching
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development