ICEfaces
  1. ICEfaces
  2. ICE-8747

FixViewState use of StateManager.getViewState(FC) disrupts state saving under MyFaces

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 3.3
    • Component/s: Framework
    • Labels:
      None
    • Environment:
      MyFaces
    • Assignee Priority:
      P2

      Description


      While investigating ICE-8324, it was found that component rendering code in FixViewState.ScriptWriter that calls Application.getStateManager.getViewState(FacesContext) triggers the StateManager's state saving of the view, in order to get the ViewState object. This then causes clearInitialState() to be called on all the UIComponents, which would not happen under Mojarra. Since state saving has happened in the midst of rendering, it then does not happen after rendering, as it should. The affect is that any state that is changed while rendering, such as from lazily loaded or set state, will then be lost between lifecycles.

      With ICE-8324 and the tabSet, a component work-around was employed, but the larger issue still remains.

      http://jira.icesoft.org/browse/ICE-8324?focusedCommentId=42816&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-42816

        Issue Links

          Activity

          Hide
          Mark Collette added a comment - - edited

          It turns out, this issue is broader and more serious than originally thought. It triggers state saving in Mojarra as well, at the incorrect time, mid-render, it's just that the ill affects are different. This is the root cause of both ICE-8303 and IPCK-418.

          Show
          Mark Collette added a comment - - edited It turns out, this issue is broader and more serious than originally thought. It triggers state saving in Mojarra as well, at the incorrect time, mid-render, it's just that the ill affects are different. This is the root cause of both ICE-8303 and IPCK-418 .
          Hide
          Deryk Sinotte added a comment -

          Been looking at this for a bit. One thing that would be helpful is a simple definitive test case that shows the bad behaviour in question for both Mojarra and MyFaces. I know we've worked around some problems in the components. Do we a small, straightforward app or test case that shows the state saving causing havoc mid-render? This would be for both diagnostic purposes and future regression testing.

          A couple of things I wanted to capture about the behaviour....

          My first assumption is that this is not a problem for non-Ajax requests. From what I can tell, the FixViewState logic only comes into play during partial page updates. Or perhaps I haven't yet found a use case where it affects a non-Ajax request.

          When doing an initial page request (so non-Ajax), I see our FormBoost encoding called once for each form. This is the mechanism we use to add some of our own scripts and hidden fields to each form. This is called multiple times and results in calls to writeState as the JSF implementation calls during FormRenderer.encodeBegin. For non-Ajax requests, Mojarra writes a "placeholder" for the viewstate value. During final rendering it replaces the placeholder with the actual viewstate value.

          During Ajax requests, this placeholder strategy is not done. However, the FixViewState encoding is done and apparently results calling some aspect of state-saving that causes the problem.

          If it's only Ajax requests that are the problem, it may be possible to get and store the view state value before the RENDER_RESPONSE phase and simply use that when it's required. From what I can tell, both Mojarra and MyFaces, during Ajax calls do not mutate or change the viewState value. So it should be possible to store it as a request or view attribute and simple use that when we need it rather than triggering any inadvertent state saving.

          Show
          Deryk Sinotte added a comment - Been looking at this for a bit. One thing that would be helpful is a simple definitive test case that shows the bad behaviour in question for both Mojarra and MyFaces. I know we've worked around some problems in the components. Do we a small, straightforward app or test case that shows the state saving causing havoc mid-render? This would be for both diagnostic purposes and future regression testing. A couple of things I wanted to capture about the behaviour.... My first assumption is that this is not a problem for non-Ajax requests. From what I can tell, the FixViewState logic only comes into play during partial page updates. Or perhaps I haven't yet found a use case where it affects a non-Ajax request. When doing an initial page request (so non-Ajax), I see our FormBoost encoding called once for each form. This is the mechanism we use to add some of our own scripts and hidden fields to each form. This is called multiple times and results in calls to writeState as the JSF implementation calls during FormRenderer.encodeBegin. For non-Ajax requests, Mojarra writes a "placeholder" for the viewstate value. During final rendering it replaces the placeholder with the actual viewstate value. During Ajax requests, this placeholder strategy is not done. However, the FixViewState encoding is done and apparently results calling some aspect of state-saving that causes the problem. If it's only Ajax requests that are the problem, it may be possible to get and store the view state value before the RENDER_RESPONSE phase and simply use that when it's required. From what I can tell, both Mojarra and MyFaces, during Ajax calls do not mutate or change the viewState value. So it should be possible to store it as a request or view attribute and simple use that when we need it rather than triggering any inadvertent state saving.
          Hide
          Mark Collette added a comment -

          The simplest test case is in the linked IPCK-418 jira. The linkage must have disappeared in the jira migration outage.

          Show
          Mark Collette added a comment - The simplest test case is in the linked IPCK-418 jira. The linkage must have disappeared in the jira migration outage.
          Hide
          Deryk Sinotte added a comment - - edited

          I've run the test case from IPCK-418 against:

          • Mojarra 2.1.6
          • Mojarra 2.1.17
          • MyFaces 2.1.9

          and don't see the problem with FixViewState leading to a ClassCastException. Is there something else that needs to be done to reproduce?

          Show
          Deryk Sinotte added a comment - - edited I've run the test case from IPCK-418 against: Mojarra 2.1.6 Mojarra 2.1.17 MyFaces 2.1.9 and don't see the problem with FixViewState leading to a ClassCastException. Is there something else that needs to be done to reproduce?
          Hide
          Deryk Sinotte added a comment - - edited

          We discussed this in the core meeting today and made some notes:

          The reason that the FixViewState class exists is to fix a long standing problem with JSF related to multiple forms on a page and having them updated via Ajax. This scenario quickly gets complicated when you talk about multiple forms not only on a single page but within includes or, especially, portlets where there can be multiple view roots on the same page. There have been ongoing discussions for some time for both Mojarra and MyFaces about this specification hole:

          http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-790

          Other relevant cases:

          http://java.net/jira/browse/JAVASERVERFACES-2199
          http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1024

          However, it hasn't actually been fixed yet. As per 790 above, it's scheduled to be in 2.2 but details are still outstanding. So turning off/removing FixViewState isn't really a viable option for the vast majority of applications.

          So if we concede that it's necessary, why is calling FacesContext.getApplication().getStateManager().getViewState(context) a problem? Since it triggers state saving mid-way through a render, the issues that have been reported suggest that saving the state at that point can cause a component to get into an unpredictable or erroneous condition. Unfortunately, calling getViewState() is pretty much the only way we can get at this value. The fact that it triggers stateSaving is perhaps unfortunate but I couldn't find any documentation indicating that it shouldn't be done.

          However, it was also the core group's opinion that the components should likely be able to have their state saved without impact during the render phase. There doesn't appear to be a way for us to tell Mojarra to return the current ViewState value without triggering state saving.

          Correct if this is a wrong assumption but the current situation seems to be that this only impacts one or two components and/or a small subset of situations. Because of this, it seems better that we handle it on a per-component basis for now (until Mojarra get's it act straightened out) rather than try to work around it in the core. So for the time being, I'm resolving this as "Won't Fix".

          Show
          Deryk Sinotte added a comment - - edited We discussed this in the core meeting today and made some notes: The reason that the FixViewState class exists is to fix a long standing problem with JSF related to multiple forms on a page and having them updated via Ajax. This scenario quickly gets complicated when you talk about multiple forms not only on a single page but within includes or, especially, portlets where there can be multiple view roots on the same page. There have been ongoing discussions for some time for both Mojarra and MyFaces about this specification hole: http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-790 Other relevant cases: http://java.net/jira/browse/JAVASERVERFACES-2199 http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1024 However, it hasn't actually been fixed yet. As per 790 above, it's scheduled to be in 2.2 but details are still outstanding. So turning off/removing FixViewState isn't really a viable option for the vast majority of applications. So if we concede that it's necessary, why is calling FacesContext.getApplication().getStateManager().getViewState(context) a problem? Since it triggers state saving mid-way through a render, the issues that have been reported suggest that saving the state at that point can cause a component to get into an unpredictable or erroneous condition. Unfortunately, calling getViewState() is pretty much the only way we can get at this value. The fact that it triggers stateSaving is perhaps unfortunate but I couldn't find any documentation indicating that it shouldn't be done. However, it was also the core group's opinion that the components should likely be able to have their state saved without impact during the render phase. There doesn't appear to be a way for us to tell Mojarra to return the current ViewState value without triggering state saving. Correct if this is a wrong assumption but the current situation seems to be that this only impacts one or two components and/or a small subset of situations. Because of this, it seems better that we handle it on a per-component basis for now (until Mojarra get's it act straightened out) rather than try to work around it in the core. So for the time being, I'm resolving this as "Won't Fix".
          Hide
          Mark Collette added a comment -

          Components don't directly go into an inconsistent state from being state saved. Since there are several scenarios where the execution portion of a lifecycle may not happen, from an initial GET and from validation failures, there are cases where components need to do processing during rendering, and will need to have the results of that processing state saved, so that on the next lifecycle they'll be in a consistent state. When getViewState triggers state saving before the component has even rendered, and then there is no state saving at the end of the rendering, then that state gets lost. This might be internal state that we don't want exposed as a property, so there is no application work-around of using a ValueExpression. So for the components, the solutions are either for getViewState to get called after they render, or for state saving to happen again, after all components have been rendered.

          For IPCK-418, the issue also that getViewState is getting called too early, in that we have not yet returned the ValueExpressions to their correct real values by the time state saving is getting triggered.

          So, if we could just sequence things like this, everything should work:

          1. Render the whole component tree
          2. Return the partial submit ValueExpressions to their original values
          3. Call getViewState
          4. Transmit the response back to the client

          This might be possible by using the JavascriptRunner to execute the javascript that FixViewState needs rendered, after steps 1 and 2, but before step 4. We might not need to actually render script tags into the forms themselves, we just need script rendered per form, and that was the most straight forward way of accomplishing it. But maybe it wasn't actually, since we need to update all forms, even if we've only done a partial sub-tree rendering in one form.

          Show
          Mark Collette added a comment - Components don't directly go into an inconsistent state from being state saved. Since there are several scenarios where the execution portion of a lifecycle may not happen, from an initial GET and from validation failures, there are cases where components need to do processing during rendering, and will need to have the results of that processing state saved, so that on the next lifecycle they'll be in a consistent state. When getViewState triggers state saving before the component has even rendered, and then there is no state saving at the end of the rendering, then that state gets lost. This might be internal state that we don't want exposed as a property, so there is no application work-around of using a ValueExpression. So for the components, the solutions are either for getViewState to get called after they render, or for state saving to happen again, after all components have been rendered. For IPCK-418 , the issue also that getViewState is getting called too early, in that we have not yet returned the ValueExpressions to their correct real values by the time state saving is getting triggered. So, if we could just sequence things like this, everything should work: 1. Render the whole component tree 2. Return the partial submit ValueExpressions to their original values 3. Call getViewState 4. Transmit the response back to the client This might be possible by using the JavascriptRunner to execute the javascript that FixViewState needs rendered, after steps 1 and 2, but before step 4. We might not need to actually render script tags into the forms themselves, we just need script rendered per form, and that was the most straight forward way of accomplishing it. But maybe it wasn't actually, since we need to update all forms, even if we've only done a partial sub-tree rendering in one form.
          Hide
          Ken Fyten added a comment -

          Re-open to consider Mark's suggestions.

          Show
          Ken Fyten added a comment - Re-open to consider Mark's suggestions.
          Hide
          Deryk Sinotte added a comment -

          Checked in a fix that changes the logic so that the call to getViewState() happens after rendering is complete.

          The new strategy is handled much like we handle the "extensions" scripts. During rendering, we track the forms that are affected by storing their ids in the FacesContext attributes map. Then, during DOMPartialViewContext.processPartial, we check for any ids and write out an eval script that contains the form ids and the new ViewState value which is aligned with how we also render out the extensions for the components.

          I did some basic testing using Mojorra and MyFaces as well as some portlets. If further testing reveals a problem we can re-open the case.

          Show
          Deryk Sinotte added a comment - Checked in a fix that changes the logic so that the call to getViewState() happens after rendering is complete. The new strategy is handled much like we handle the "extensions" scripts. During rendering, we track the forms that are affected by storing their ids in the FacesContext attributes map. Then, during DOMPartialViewContext.processPartial, we check for any ids and write out an eval script that contains the form ids and the new ViewState value which is aligned with how we also render out the extensions for the components. I did some basic testing using Mojorra and MyFaces as well as some portlets. If further testing reveals a problem we can re-open the case.

            People

            • Assignee:
              Deryk Sinotte
              Reporter:
              Mark Collette
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: