Issue Details (XML | Word | Printable)

Key: JAVASERVERFACES_SPEC_PUBLIC-806
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Major Major
Assignee: Ed Burns
Reporter: aschwart
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
javaserverfaces-spec-public

Simplify PostRestoreStateEvent delivery requirements

Created: 24/May/10 02:33 PM   Updated: 01/Feb/12 02:21 AM   Resolved: 22/Jun/10 11:41 AM
Component/s: Documentation: Javadoc, TLDDoc, RenderkitDoc, PDF
Affects Version/s: 2.0
Fix Version/s: 2.0 Rev a

Time Tracking:
Not Specified

File Attachments: 1. Text File 806-patch.txt (12 kB) 03/Jun/10 08:46 AM - Ed Burns
2. Text File 806-patch.txt (4 kB) 02/Jun/10 08:31 AM - Ed Burns
3. Text File changebundle.txt (17 kB) 01/Jun/10 12:59 PM - Ed Burns
4. Java Archive File glassfish-jsf-2.0.3-SNAPSHOT.jar (2.16 MB) 01/Jun/10 01:01 PM - Ed Burns

Environment:

Operating System: All
Platform: Macintosh


Issuezilla Id: 806
Status Whiteboard:

changelog

Tags:
Participants: aschwart, dougd, Ed Burns and rogerk


 Description  « Hide

JSF2 introduces the PostRestoreState system event which is delivered to each component after its state
has been restored. At the moment these events are delivered from three separate locations:

1. UIViewRoot.processRestoreState()

This handles the full state saving case.

2. RestoreViewPhase.execute()

This handles the pre-existing UIViewRoot case (ie. UIViewRoot is set before restore view phase is
executed).

3. StateManagementStrategyImpl.restoreView()

This handles the partial state saving case.

This approach seems more complex than necessary. A simpler approach would be to deliver these
events from one location - ie. from the lifecycle implementation at the end of the restore view phase.

A more specific problem that falls out of the current approach is that this places a burden on custom
StateManagers to also implement this behavior. For example, Trinidad's state manager implementation
that performs view root caching, see:

http://myfaces.apache.org/trinidad/devguide/configuration.html#org.apache.myfaces.trinidad.CACHE_
VIEW_ROOT

Must (re-)implement its own PostRestoreStateEvent delivery pass.

This leads to a related problem… PostRestoreStateEvent delivery requires that the UIViewRoot has
already been connected to the FacesContext via a call to FacesContext.setViewRoot(). However, at the
point where the StateManager is called to restore the view, the UIViewRoot has yet to be connected.
The view root is connected to the FacesContext by the lifecycle implementation, after the StateManager
has restored the view.

As a result, attempts to deliver PostRestoreStateEvents from custom state managers fail.

Rather than place the burden on state manager implementations to further duplicate the work of
delivering PostRestoreViewEvents (which also requires a new responsibility of attaching the view root to
the FacesContext), the spec should be simplified to state that PostRestoreStateEvents are delivered by
the lifecycle implementation after the state has been restored.



aschwart added a comment - 24/May/10 02:38 PM

In order to accommodate this change, there are three places in the spec that would need to be updated.
First, from the UIViewRoot.processRestoreState() javadoc:

> The try block must have a finally block that ensures that no FacesEvents remain
> in the event queue, and that the this.UIComponent.visitTree
> (javax.faces.component.visit.VisitContext,
> javax.faces.component.visit.VisitCallback) is called, passing a ContextCallback
> that takes the following action: call the UIComponent.processEvent
> (javax.faces.event.ComponentSystemEvent) method of the current component.
> The argument event must be an instance of PostRestoreStateEvent whose
> component property is the current component in the traversal.

This seems overly specific. The important point from the spec perspective is that
PostRestoreStateEvents are delivered after the component tree is restored and before we move on from
the restore view phase.

The second place is related - section 4.1.19.4 says:

> The UIViewRoot instance itself must override processRestoreState() and directly
> call processEvent(), passing a PostRestoreStateEvent instance as specified in
> the Javadocs for UIViewRoot.processRestoreState().

Again, this is more specific than it needs to be.

The third place is in section 2.2.1:

> Examine the FacesContext instance for the current request. If it already
> contains a UIViewRoot:
> * Set the locale on this UIViewRoot to the value returned by the
> getRequestLocale() method on the ExternalContext for this request.
> * Call doTreeTraversal() on the UIViewRoot, passing a ContextCallback
> implementation that calls the processEvent() method of the component. The
> argument event must be an instance of PostRestoreStateEvent whose
> component property is the current component in the traversal.

This covers the pre-existing UIViewRoot case (#2 above). Again, this is more specific than the spec
needs to be. One way for JSF implementations to ensure that PostRestoreStateEvents are delivered is to
deliver these differently for the full/pre-existing/partial state saving cases. Another equally acceptable
(or, better) way is to deliver these once for all three cases.

(BTW, the "doTreeTraversal()" and "ContextCallback" references are out of date as of the addition of our
final tree visiting API, so we'll need to revise this.)


aschwart added a comment - 24/May/10 02:53 PM

One other note... Mojarra also ran into this issue in another context:

https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1262

In that case, the RichFaces custom state manager was failing during the PostRestoreStateEvent delivery
due to the fact that the UIViewRoot was not attached to the FacesContext when
UIViewRoot.processRestoreState() was called.

Mojarra worked around this issue by inserting a call to FacesContext.setViewRoot() inside of
UIViewRoot.processRestoreState(), before initiating the PostRestoreStateEvent tree visit:

> public void processRestoreState(FacesContext context, Object state) {
>
> try {
> // hack to work around older state managers that may not set the
> // view root early enough
> if (context.getViewRoot() == null) { > context.setViewRoot(this); > }

Rather than forcing the setViewRoot() call early, I believe that a better solution would be to delay
PostRestoreStateEvent delivery until after the UIViewRoot has been attached to the FacesContext at the
usual time (ie. after the state manager has completed its work).


Ed Burns added a comment - 24/May/10 04:23 PM

This seems to have a behavior requirement, so I am moving it to 2.1.


aschwart added a comment - 28/May/10 07:59 AM

Couple of other notes that might be relevant...

There is another subtle different between between the full state saving case and the partial state saving
case - at least in the Mojarra implementation. In the full state saving case, PostRestoreStateEvents are
delivered after the full component tree has been restored. In the partial state saving case,
PostRestoreStateEvents are delivered during the same tree visit as calls to UIComponent.restoreState().
As a result, at the time when a PostRestoreStateEvent is delivered to a system event listener, the
component tree is in slightly different states. In one case (full) the component and all of its children
have been fully restored. In the other case (partial), only the component itself has been restored, but
state for children has not yet been restored.

Such subtle differences are worrisome as this could lead to code which works well in one case but fails
in another.

Also, based on the Mojarra implementation it looks like these events are delivered before dynamically
created subtrees are inserted into the tree. As a result, not all components will receive receive these
events.

Seems like the easiest way to avoid all of these problems would be to loosen up the spec to allow
implementations more flexibility in how/when they deliver PostRestoreStateEvents - ie. to allow
implementations to deliver PostRestoreStateEvents once, from the lifecycle, after the component tree
has been restored.


Ed Burns added a comment - 01/Jun/10 07:42 AM

Add adf keyword


Ed Burns added a comment - 01/Jun/10 08:52 AM

adf_high


Ed Burns added a comment - 01/Jun/10 12:59 PM

Created an attachment (id=238)
Fix for the implementation portion of this bug.


Ed Burns added a comment - 01/Jun/10 01:01 PM

Created an attachment (id=239)
binary jar so Doug can run the TCK


Ed Burns added a comment - 02/Jun/10 08:31 AM

Created an attachment (id=240)
Patch with only the changed files relevant to this fix


dougd added a comment - 02/Jun/10 12:02 PM

Ran it through the JSF 2.0 TCK and all tests passed.


Ed Burns added a comment - 02/Jun/10 12:52 PM

Move back to 2.0 Rev a.

The Mojarra portion of the fix for this bug has been committed, r8403.


Ed Burns added a comment - 03/Jun/10 08:46 AM

Created an attachment (id=241)
Fix for this bug, second iteration


Ed Burns added a comment - 03/Jun/10 12:42 PM

Fix checked in.


rogerk added a comment - 22/Jun/10 11:41 AM

changelog