Bugzilla – Full Text Bug Listing
| Summary: | [Viewers] Un-expended elements in a lazy loading view may contain invalid children. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Pawel Piech <pawel.1.piech> | ||||||||||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||||||||
| Status: | RESOLVED WORKSFORME | QA Contact: | |||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | bokowski, darin.eclipse, hsoliwal | ||||||||||||
| Version: | 3.6 | ||||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Whiteboard: | stalebug | ||||||||||||||
| Attachments: |
|
||||||||||||||
For use case see bug 305177. JDoc for void org.eclipse.jface.viewers.TreeViewer.replace(Object , int , Object ) :
For a TreeViewer with a tree with the VIRTUAL style bit set, replace the given parent's child at index with the given element. If the given parent is this viewer's input or an empty tree path, this will replace the root element at the given index.
This method should be called by implementers of ILazyTreeContentProvider to populate this viewer.
The last line clearly documents that the method be called from with a content provider during a callback for the item. This is either an enhancement request or invalid .
PS : An observation :- you materialize the item but forget to update the child count.
TreePath path = new TreePath(new Object[] {"Element"});
treeViewer.replace(path, 0, Integer.toString(0)); //enhancement req?
treeViewer.setChildCount(path, 1000); //enhancement req?
(In reply to bug 305177 comment #5) > Please feel free to explain the use case on bug 311442. And I recommended > using a snippet or explaining the use-case rather than pointing to the break > point view code . This makes it easy for other interested folks to follow. Sorry, I filed the bug in a rush. The use case is the following: The debugger views implement an extension and a specialized content provider for the TreeViewer. This content provider delegates to the model using an API that allows for content updates where the view requests data from the model as well as delta events where the model notifies the view of changes in the model. The delta notifications have several different types of changes that they can update the viewer with (see org.eclipse.debug.internal.ui.viewers.model.provisional.IModelDelta). The one that is a problem here is the ADDED flag. The exact sequence of events in a failing use case is as follows: 1) The model tells the viewer that a given element was added and it gives the viewer the correct index of the new element as well as the child count of the parent. Effectively this calls viewer.setChildCount(parent, count), then viewer.replace(parent, index, element). 2) Asynchronously to 1) the viewer requests an update on the parent element, so that some time after the call to setChildCount(parent), the content provider also calls setHasChildren(parent). setHasChildren(parent) zeroes out the children of the parent which were set in 1) because the element is collapsed. (BTW, maybe this can be the easy fix here). 3) Following 1) the viewer updates the children of the parent with the model, which eventually results in calls to replace(parent, index, element) for each child of the parent which was added using the ADDED flag. All the replace() calls get dropped except for the child at index==0, which replaces the dummy node. 4) When user expands the parent node, it has only one child in it. (In reply to comment #2) > The last line clearly documents that the method be called from with a content > provider during a callback for the item. This is either an enhancement request > or invalid . I don't want to get into bickering whether something is a bug or an enhancement, because I don't think it's productive. If you think that supporting this behavior is too much of the burden for the viewer, then feel free to mark as won't fix and clarify the javadoc to say that replace() should be called only in response to a viewer.updateElement() request. The implication for the debugger views is that the ADDED/REMOVED delta flags become a lot less useful, because this limitation means that the model will also have to always force the parent elements to be expanded. (In reply to comment #3) > > I don't want to get into bickering whether something is a bug or an > enhancement, because I don't think it's productive. If you think that > supporting this behavior is too much of the burden for the viewer, then feel > free to mark as won't fix and clarify the javadoc to say that replace() should > be called only in response to a viewer.updateElement() request. I am glad to hear that you find arguments unproductive, but lets not digress into irrelevant topic here. :) Thanks for clarifying your use case; the comments certainly help in arriving at a fix. > 2) Asynchronously to 1) the viewer requests an update on the parent element, so > that some time after the call to setChildCount(parent), the content provider > also calls setHasChildren(parent). setHasChildren(parent) zeroes out the > children of the parent which were set in 1) because the element is collapsed. > (BTW, maybe this can be the easy fix here). The call to setHasChildren(Object, boolean) in the content provider, can they not be avoided under these conditions (also suggested by JDoc). /** * For a TreeViewer with a tree with the VIRTUAL style bit set, inform the * viewer about whether the given element or tree path has children. Avoid * calling this method if the number of children has already been set. * * @param elementOrTreePath * the element, or tree path * @param hasChildren * * @since 3.3 */ > 3) Following 1) the viewer updates the children of the parent with the model, > which eventually results in calls to replace(parent, index, element) for each > child of the parent which was added using the ADDED flag. All the replace() > calls get dropped except for the child at index==0, which replaces the dummy > node. Quite naturally if (index < parentItem.getItemCount()) Note: this(suggested change) in conjunction with the recently suggested fix that defers materialization of unexpanded/leaf items may result in some stale element data problems. > > There's a whole bunch of nasty stuff going on here: > > > > 1) > > The use of a dummy node by the TreeViewer to indicate that an un-expanded > > element has children is the primary reason for the bug. I filed bug 311442 for > > this in the UI component but I doubt it'll get fixed in 3.6... if ever. > > If you see further problems with this, please do share with us? (In reply to comment #4) > If you see further problems with this, please do share with us? Could you clarify, what is the suggested fix? (In reply to comment #5) Quoting from Bug 305177 Comment #2 : > > > > > There's a whole bunch of nasty stuff going on here: > > > > > > 1) > > > The use of a dummy node by the TreeViewer to indicate that an un-expanded > > > element has children is the primary reason for the bug. I filed bug 311442 for > > > this in the UI component but I doubt it'll get fixed in 3.6... if ever. > > > > My Comment to the above: > If you see further problems with this, please do share with us? It seems to suggest that you have doubts about this bug ( 311442 ) ever get fixed due to some technical reasons. (In reply to comment #6) > It seems to suggest that you have doubts about this bug ( 311442 ) ever get > fixed due to some technical reasons. It was more of a comment at the conservative nature of the project and the low likelyhood of it getting addressed without a smoking gun problem in the JDT product. For example, I had opened bug 266189 in Feb 2009, but it wasn't until a month ago where a performance problem was found in JDT stack retrieval, that this problem got any attention. IMO there is a valid use case to support calling setChildCount/setHasChildren/replace even when the viewer hasn't requested an update on it. The current limitation is just a result of an implementation choice for tracking of hasChildren state of unexpanded nodes. (In reply to comment #7) OK. Skip. Moving on to bug at hand .. > IMO there is a valid use case to support calling > setChildCount/setHasChildren/replace even when the viewer hasn't requested an > update on it. The current limitation is just a result of an implementation > choice for tracking of hasChildren state of unexpanded nodes. Sure there might be a number of use cases, but I haven't seen any on the bug so far. As you mentioned correctly it could be about limitation of the client code. Clearly, the matter at hand appears to be about implementation : ordering of calls. > > 2) Asynchronously to 1) the viewer requests an update on the parent element, so > > that some time after the call to setChildCount(parent), the content provider > > also calls setHasChildren(parent). setHasChildren(parent) zeroes out the > > children of the parent which were set in 1) because the element is collapsed. > > (BTW, maybe this can be the easy fix here). > The call to setHasChildren(Object, boolean) in the content provider, can they not be avoided under these conditions - as suggested by JDoc(repeat). Yes, we could fix this here if (!item.getExpanded()) But that is an unjustified loss of optimization I am greatly reluctant about. > > /** > * For a TreeViewer with a tree with the VIRTUAL style bit set, inform the > * viewer about whether the given element or tree path has children. Avoid > * calling this method if the number of children has already been set. > * > * @param elementOrTreePath > * the element, or tree path > * @param hasChildren > * > * @since 3.3 > */ > > Note: this(suggested change) in conjunction with the recently suggested fix > that defers materialization of unexpanded/leaf items may result in some stale > element data problems. > Importantly, see Bug 305739. Created attachment 167201 [details]
API compliant snippet.
After your comments I thought that we could avoid this bug by restricting when we call replace() so that we don't call it unless prompted by the content provider. It took me a long time to reproduce the failure in a unit test, but then a fix wasn't too bad.
But then I though of another use case which actually is completely API compliant:
1) Element is expanded, children are realized.
2) Element is collapsed.
3) Element is refreshed, and children are refreshed at the same time.
4) Element is re-expanded.
This snippet reproduces the bug.
Created attachment 167202 [details]
Suggested fix.
Created attachment 167203 [details]
Debug viewers tests and attempted fix.
Created attachment 167205 [details]
Separate fix for debug viewers which override virtualRefreshExpandedItems()
I filed separate bug 311792 for the debugger views fixes related to this bug. IMO, this bug is not very severe, since we have a workaround for it in bug 305177 and in debugger we have a separate fix for it anyway. Hi Hitesh, Do you have an opinion on how risky is the proposed fix? (In reply to comment #9 & comment #14) > Created an attachment (id=167201) [details] > API compliant snippet. > This snippet reproduces the bug. Copy pasting from snippet .. public static class ContentProvider implements ILazyTreePathContentProvider ... public void updateHasChildren(final TreePath path) { fViewer.getControl().getDisplay().asyncExec(new Runnable() { ... public void updateChildCount(final TreePath path, final int currentChildCount) { fViewer.getControl().getDisplay().asyncExec(new Runnable() { public void run() { ... public void updateElement(final TreePath path, final int index) { fViewer.getControl().getDisplay().asyncExec(new Runnable() { ... (In reply to comment #13) > I filed separate bug 311792 for the debugger views fixes related to this bug. > IMO, this bug is not very severe, since we have a workaround for it in bug > 305177 and in debugger we have a separate fix for it anyway. Are you referring patch from Comment 10 ? I haven't seen the patch closely yet. (In reply to comment #15) > (In reply to comment #9 & comment #14) > Are you referring patch from Comment 10 ? I haven't seen the patch closely > yet. Yes, the fix basically does the same thing as setHasChildren(true) does when called on a collapsed element with children. This way, the viewer does not request an update on a child of an un-expanded element, therefore the replace on the first child of an un-expanded element is not called either... which is the direct cause of this bug. This in short addresses the bug in our breakpoints view use case. > Copy pasting from snippet .. Did you want to make a point about the snipped? I seem to have missed it. Thanks, Pawel (In reply to comment #16) > > > Copy pasting from snippet .. > Did you want to make a point about the snipped? I seem to have missed it. > > Thanks, > Pawel Use of Display.asyncExec(new Runnable(){}) is not legal there, the update calls still happen from outside that context. Please try to provide a snippet to demo usecase/problem without such schemes. Let me give a closer look at the patch though... PS: We cannot reverse fix everything, especially on a case by case basis... (In reply to comment #17) > Use of Display.asyncExec(new Runnable(){}) is not legal there, the update calls > still happen from outside that context. Please try to provide a snippet to demo > usecase/problem without such schemes. This is news to me. Both the structure of the API (i.e. the fact that updateElement() does not have a return value), and the comments indicate that it's legal for the content provider to respond to the update calls asynchronously. > Let me give a closer look at the patch though... Thanks! > PS: We cannot reverse fix everything, especially on a case by case basis... Could you elaborate. I don't quite understand. (In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #9 & comment #14) > > Are you referring patch from Comment 10 ? I haven't seen the patch closely > > yet. > Yes, the fix basically does the same thing as setHasChildren(true) does when > called on a collapsed element with children. This way, the viewer does not > request an update on a child of an un-expanded element, therefore the replace > on the first child of an un-expanded element is not called either... which is > the direct cause of this bug. This in short addresses the bug in our > breakpoints view use case. I think I can answer my own question... the patch is no good. While testing I discovered that this change (calling item.getItemCount() to be exact, causes the item to materialize. So on refresh, all items get matrialized. Is this bug any longer valid ? If not I am closing it. (In reply to comment #20) > Is this bug any longer valid ? If not I am closing it. Marking as WORKSFORME. (In reply to comment #20) > Is this bug any longer valid ? If not I am closing it. Yes, I do think it's still valid though based on comment #17 we seem to disagree on an important point. If I understand you correctly, then implementations of ILazyTreePathContentProvider should call fViewer.setHasChildren only in the implementation of updateHasChildrenI() and only in the same dispatch cycle. There seems to be nothing in the javadoc to indicate that such a restriction exists, in fact the following note in description of updateElement seems to say otherwise: * <strong>NOTE</strong> #updateElement(int index) can be used to determine * selection values. If TableViewer#replace(Object, int) is not called * before returning from this method, selections may have missing or stale * elements. In this situation it is suggested that the selection is asked * for again after replace() has been called. Unfortunately I don't currently have a fix for this bug, the fix that I tried and attached has bad side effects. On the upside, I do have a workaround for this bug, but the workaround is only for the narrow use case in which this bug was discovered. It's likely that we'll run into this bug again as we implement more features on top of the virtual tree viewer. *** Bug 311792 has been marked as a duplicate of this bug. *** (In reply to comment #22) Here it goes again all the way from top: As per comment #3: in step 1) the childCount has been set already , in step 2) which happens asynchronously you call setHasChildren In comment #4 : I quote the Java-doc for the method setHasChildren , which clearly says >Avoid calling this method if the number of children has already been set. Clearly your steps 1) and 2) are conflicting here. Please read the Java-doc for the method again. The bug is invalid unless you provide a Java-doc compliant snippet to demonstrate the same. Until then it is a WORKSFORME or a WONTFIX at most !!! PS: The call backs to the content provider happen in a certain order and yes they are meant to happen in the same dispatch. I am not going to bother explaining it again - the JavaDoc does that well enough already. Using the content provider in your 'complaint' snippet, could you please explain how an expand-to-nth-level will work, of course without all the strange listeners that one may wire up in his code. It'll be even more interesting to see how the API behaves when calls get mixed-up in order and are asynchronous in combination. (In reply to comment #24) Your last comment seems rather frustrated so let me try to cool down this discussion a bit. The debug views are some of the first to have used the lazy-loading viewers and have always called the set* methods asynchronously. So please let's not argue whether these methods can be called async. It's true that the async. callback makes the viewer implementation very complicated and sensitive to changes. Based on the snippet from comemnt #9 this bug is real even when the order of calls to setChildCount() and setHasChildren(). We've found many issues like this one and we've managed to find fixes and workarounds for them eventually. I don't expect you to run out and start trying to find a bullet-proof solution to this bug. I only ask of you that you keep this bug open to keep an accurate record of the technology (perhaps downgrade its severity without a active use case). Also, if I come across another use case that triggers this bug, I'll take another stab at fixing it. If I find a fix, I'd appreciate your support in reviewing it and applying it so that we may achieve our common goal of a better jface framework. (In reply to comment #25) > Based on the snippet from comemnt #9 this bug is real even when the order of > calls to setChildCount() and setHasChildren() ... is maintained correctly as specified in the api java doc. (In reply to comment #24) >Your last comment seems rather frustrated so let me try to cool down this >discussion a bit. Nope!! Not at all. I guess it may appear to you this way probably because the current usecase, for some unknown reason, has not been clearly communicated. As a matter of fact, some of my current bugs deal quite a bit with optimization, and require detailed work. What better time if you could suggest improving an area :). Please do carry on with your comments :). (In reply to comment #27) Great :-) In this case could you confirm whether you agree that the content provider can call viewer methods asynchronously. Please update the bug title appropriately, it sounds like an enhancement request to me. (In reply to comment #29) > Please update the bug title appropriately, it sounds like an enhancement > request to me. Thank you Hitesh for pointing it out. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. This bug has been marked with as "stalebug" for some time now. I mark this bug as "worksforme" to indicate that no work is planned here. If this bug is still relevant for the latest release, please reopen the bug and remote the "stalebug" whiteboard tag. |
Created attachment 166873 [details] Snippet. The virtual TreeViewer uses a dummy to node to indicate that an element has children while the element is not expanded. If the model happens to call replace() on this dummy child, which could be a result of an asynchronous update, then the viewer will think that the parent element really only has one child. The attached snippet reproduces the problem.