MyFaces Core
  1. MyFaces Core
  2. MYFACES-2924

Component bindings are not reset when explicit navigation to the same page is derived from action

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.6, 2.1.0
    • Component/s: JSR-314
    • Labels:
      None

      Description

      It is possible to cause a duplicate id exception doing the following steps:

      • Render one page (let's call mypage.xhtml) with a component that has a binding to a request scope managed bean
      • Click the button

      mypage.xhtml
      ....
      <x:component binding="#

      {bean.component}

      />
      <f:facet name="header">
      /.... some components here with non generated id ..../
      </f:facet name="header">
      </x:component>
      <h:commandButton action="#

      {bean.someMethod}

      ">
      ....
      bean

      public String someMethod()

      { return "mypage"; }

      That example should work without problem. The problem is raised by an optimization done in myfaces:

      UIInstructionHandler

      if (mctx.isRefreshingTransientBuild())
      {
      c = ComponentSupport.findChildByTagId(parent, id);
      /....../

      If we are not refreshing the current view, it does not have sense to try to find a component that will not be found, right?

      The spec is clear about the effect of the previous example:

      JSF 2.0 section 7.4.2 : "...A rule match always causes a new view to be created, losing the state of the old view. This includes clearing out the view map....."

      but if you have component bindings on the page, all components inside the component with the binding will preserve the state. There is one simple solution from the user point of view (and this is what everyone usually does in this case):

      public String someMethod()

      { return null; }

      no rule match means no navigation, the view state is preserved and finally no duplicate id exception.

      If we disable the optimization code, the code will work, but there is still one problem with the spec.

      1. MYFACES-2924-1.patch
        10 kB
        Leonardo Uribe

        Activity

        Hide
        Leonardo Uribe added a comment -

        Attached to this issue there is a proposal to handle "binding" cleanup gracefully.

        In few words, the idea is if there is a "binding" set for a component, attach a component system event listener that will set to null to the "binding" value expression (only if is readOnly, that means if there is a setter for it). Later, if there is a normal navigation on a POST request, before replace the view with the new one, do a full visitTree with SKIP_ITERATION enabled to publish PreDisposeViewEvent over all components, to give the chance to the listener to do the proper cleanup.

        Do it in this way is necessary to preserve t:aliasBean working (because use visitTree gives the chance to t:aliasBean to do its hack).

        It is known that do a full visitTree is expensive in time, so I did a profile session (using NetBeans Profiler and MyFaces Tests) to get a better idea if the patch proposed should be applied or not.

        The first thing I notice was ComponentSupport.findChildByTagId is expensive too, and in numbers is equivalent to a full visitTree (almost 1:1). If the view is small, visitTree becomes cheaper, but if the view is big and there are not too many NamingContainer components, findChildByTagId becomes more expensive.

        Now, the effect of the patch proposed is all code related to findChildByTagId is not called on the first request, and if we are considering a navigation case, we are saving twice (on restore view and on build the next view), so the final effect is the code will perform better (each buildView takes aprox. 10% less time!) and additionally this issue will be fixed, being conformant with the spec.

        I think this code should be definitively committed, but since it could be a controversial change, I want to let some time if anyone has something to say.

        If not objections I'll commit this code soon.

        Show
        Leonardo Uribe added a comment - Attached to this issue there is a proposal to handle "binding" cleanup gracefully. In few words, the idea is if there is a "binding" set for a component, attach a component system event listener that will set to null to the "binding" value expression (only if is readOnly, that means if there is a setter for it). Later, if there is a normal navigation on a POST request, before replace the view with the new one, do a full visitTree with SKIP_ITERATION enabled to publish PreDisposeViewEvent over all components, to give the chance to the listener to do the proper cleanup. Do it in this way is necessary to preserve t:aliasBean working (because use visitTree gives the chance to t:aliasBean to do its hack). It is known that do a full visitTree is expensive in time, so I did a profile session (using NetBeans Profiler and MyFaces Tests) to get a better idea if the patch proposed should be applied or not. The first thing I notice was ComponentSupport.findChildByTagId is expensive too, and in numbers is equivalent to a full visitTree (almost 1:1). If the view is small, visitTree becomes cheaper, but if the view is big and there are not too many NamingContainer components, findChildByTagId becomes more expensive. Now, the effect of the patch proposed is all code related to findChildByTagId is not called on the first request, and if we are considering a navigation case, we are saving twice (on restore view and on build the next view), so the final effect is the code will perform better (each buildView takes aprox. 10% less time!) and additionally this issue will be fixed, being conformant with the spec. I think this code should be definitively committed, but since it could be a controversial change, I want to let some time if anyone has something to say. If not objections I'll commit this code soon.
        Hide
        Jakob Korherr added a comment -

        The patch looks good. +1 from my side!

        Show
        Jakob Korherr added a comment - The patch looks good. +1 from my side!
        Hide
        Mike Kienenberger added a comment -

        Does this bug affect Myfaces 1.2 as well?
        I think I've seen this problem in my own 1.2 apps.

        Show
        Mike Kienenberger added a comment - Does this bug affect Myfaces 1.2 as well? I think I've seen this problem in my own 1.2 apps.
        Hide
        Leonardo Uribe added a comment -

        The bug is present in JSF 1.2 and 1.1. I haven't investigated too much if it is possible to fix it there.

        I theory, something should be done on the spec to solve this one, but after checking it more carefully I realized we can fix in myfaces code without include it on the spec, and instead consider it an implementation detail.

        Show
        Leonardo Uribe added a comment - The bug is present in JSF 1.2 and 1.1. I haven't investigated too much if it is possible to fix it there. I theory, something should be done on the spec to solve this one, but after checking it more carefully I realized we can fix in myfaces code without include it on the spec, and instead consider it an implementation detail.

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            Leonardo Uribe
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development