On 9/12/13 4:02 PM, Manfred Riem wrote:
> Hi Andy,
>
> I am currently looking at
> https://java.net/jira/browse/JAVASERVERFACES-3024
> and I can see the problem he is pointing out. Upon a better look I
> have a couple
> of questions.
>
> Note this is FSS.
>
> 1. We are processing events when we are restoring the component tree
> structure.
>
> Q. Should we be processing events during restoreTree?
JSF implementations should not deliver PostAddToView events while
restoring the component tree structure during full state saving, as the
component tree is in an intermediate, unusable state at this point.
This is mentioned in javadoc for PostAddToViewEvent:
http://docs.oracle.com/javaee/7/api/javax/faces/event/PostAddToViewEvent.html
In particular:
> The implementation must guarantee that
> Application.publishEvent(javax.faces.context.FacesContext,
> java.lang.Class<? extends javax.faces.event.SystemEvent>,
> java.lang.Object) is called, immediately after any UIComponent
> instance is added to the view hierarchy except in the case where
> ResponseStateManager.isPostback(javax.faces.context.FacesContext)
> returns true at the same time as FacesContext.getCurrentPhaseId()
> returns PhaseId.RESTORE_VIEW. When both of those conditions are met,
> Application.publishEvent(javax.faces.context.FacesContext,
> java.lang.Class<? extends javax.faces.event.SystemEvent>,
> java.lang.Object) must not be called.
The PostRestoreStateEvent is intended to provide a more appropriate hook
point for restore view-time processing.
Note that the above javadoc is unfortunately too general. We don't want
PostAddToViewEvents delivered when re-constructing the component tree
from full state. However, when re-constructing the component tree by
executing tags (ie. partial state saving), PostAddToViewEvents do need
to be fired. Without this, the component tree might not be successfully
restored to its previous state.
> 2. We are indeed caching the notion of a compositeComponent and the
> boolean
> used gets cached because the restoreState of that component has not
> happened
> yet.
>
> Q. Would you consider it safe to cache this?
Looking at the stack trace from the issue tracker:
> UIComponent.isCompositeComponent(UIComponent) : 2112
> UINamingContainer(UIComponent).pushComponentToEL(FacesContext,
> UIComponent) : 1985
> UIComponentBase.publishAfterViewEvents(FacesContext, Application,
> UIComponent) : 2244
> UINamingContainer(UIComponentBase).doPostAddProcessing(FacesContext,
> UIComponent) : 1927
> UINamingContainer(UIComponentBase).setParent(UIComponent) : 447
> UIComponentBase$ChildrenList.add(UIComponent) : 2679
> UIComponentBase$ChildrenList.add(Object) : 2651
> FaceletFullStateManagementStrategy.restoreTree(FacesContext, String,
> Object[]) : 534
> FaceletFullStateManagementStrategy.restoreView(FacesContext, String,
> String) : 571
> StateManagerImpl.restoreView(FacesContext, String, String) : 138
It is clear that isCompositeComponent() has no chance of returning the
correct result at this point. Since we have yet to restore the
component's attribute-related state, this attribute lookup in
UIComponent.isCompositeComponent():
> result = component.isCompositeComponent =
> (component.getAttributes().containsKey(
> Resource.COMPONENT_RESOURCE_KEY));
Will fail.
One way to avoid this initial invalid result would be to implement the
spec defined behavior - ie. do not attempt to publish
PostAddToViewEvents (and thus avoid the component pushing/popping).
However, this will only get us so far.
As seen in the second trace from the issue tracker,
FaceletFullStateManagementStrategy subsequently performs a tree visit
for the purpose of calling restoreState():
> UINamingContainer(UIComponentBase).restoreState(FacesContext, Object)
> : 1591
> FaceletFullStateManagementStrategy$2.visit(VisitContext, UIComponent)
> : 350
> FullVisitContext.invokeVisitCallback(UIComponent, VisitCallback) : 151
> UINamingContainer(UIComponent).visitTree(VisitContext, VisitCallback)
> : 1729
> UINamingContainer.visitTree(VisitContext, VisitCallback) : 174
> HtmlForm(UIComponent).visitTree(VisitContext, VisitCallback) : 1740
> HtmlForm(UIForm).visitTree(VisitContext, VisitCallback) : 371
> HtmlBody(UIComponent).visitTree(VisitContext, VisitCallback) : 1740
> UIViewRoot(UIComponent).visitTree(VisitContext, VisitCallback) : 1740
> FaceletFullStateManagementStrategy.restoreComponentState(FacesContext,
> HashMap<String,Object>) : 335
> FaceletFullStateManagementStrategy.restoreView(FacesContext, String,
> String) : 586
This tree visit suffers from the same problem as the PostAddToViewEvent
processing traversal: it calls pushComponentToEL() on each component
before UIComponent.restoreState() has been called on the component. As
such, isCompositeComponent() will again always return false.
The fact that this invalid result is cached isn't as much of a problem
as the fact that we get an invalid result in the first place.
The caching part of this can be easily fixed by nulling out the cached
value in the implementation of
UIComponent.restoreState().
Although we should implement this trivial fix, I don't believe that this
fix - or, actually, even removing caching altogether - will address the
problem that mysticfall is seeing.
As soon as pushComponentToEL() is called with a partially restored
composite component, we end up corrupting the composite component stack
maintained by UIComponent. That is, this call to push() in
pushCompoentToEL():
> // if the pushed component is a composite component, we need
> to update that
> // stack as well
> if (UIComponent.isCompositeComponent(component))
> {
> _getComponentELStack(_CURRENT_COMPOSITE_COMPONENT_STACK_KEY,
> contextAttributes).push(component);
Fails to execute, even if the component is a composite component.
As a result, calls to UIComponent.getCurrentCompositeComponent() will
return an incorrect result, with or without caching. Also, the matching
call to popComponentFromEL() may result in an unbalanced stack pop (if
we remove caching, or if we clear the cache in restoreState()).
So, yeah, we've got a bit of a mess here.
Looking back, I think that the root of our problems dates back to the
JSF 2.0 spec and the introduction of composite components and component
pushing/popping. Given that component pushing/popping can only be
correctly performed after component state has been restored, we should
have taken steps (in the spec) to ensure that no attempt to push/pop
components would be made until after component state has been restored.
In reality, the first safe point to push/pop components when performing
full state saving is during the PostRestoreStateEvent delivery
traversal. The spec should have recognized this reality and disallowed
pushing/popping before this point.
Although that would have been an easy adjustment to make before the 2.0
spec was finalized, I am not quite sure what to do about this now.
I do think that there are a couple of implementation choices that
Mojarra has made that might be exacerbating the issue. Two in particular:
1. visitTree vs. processRestoreState
Up until recently Mojarra leveraged good old processRestoreState() for
performing the restore state traversal for full state saving. At some
point in the last year or so this was changed over to visitTree(). I
cannot say for sure without looking at the PrimeFaces Tree code, but my
guess is that reverting back to processRestoreState() would avoid the
sort of short-circuiting that we are now seeing with visitTree().
2. Facelet component map
From the issue tracker:
> As the tree nodes are not visited by the
FaceletFullStateManagementStrategy, they are
> missing from ComponentSupport.getFaceletComponentMap() so it creates
another copy
> from the VDL template.
This is another case where the tagId->UIComonent map introduced with
this optimization:
https://java.net/jira/browse/JAVASERVERFACES-2494
Is leading to unexpected/undesirable behavior. I would highly recommend
that we revisit this fix, and instead consider implementing an
equivalent of the optimization that we implemented for JSP years ago, as
described here:
https://java.net/jira/browse/JAVASERVERFACES-1620
Doing #1 and #2 should have the nice side effect of resolving issue 3024.
Andy
> Regards,
> Manfred
>