Community
Participate
Working Groups
Build Identifier: 3.7.1 M20110909-1335 It seems that the “state restore” code doesn’t get called anymore after previous state was saved. Reproducible: Always Steps to Reproduce: 1. Start debug session with the breakpoint set at line 14 (i--;) 2. Expand ii variable from variables view. You can expand also first range [0...99] 3. Press Resume button 4. Repeat step 3 ii variable collapses after resuming the second time.
Created attachment 204518 [details] java test to reproduce the bug java test to reproduce the bug
Debugging the code, it looks like a race between thread doing fViewerStates.get() in ModelContentProvider and the thread doing fViewerStates.put().
Previous description: Array variable collapses after pressing “Resume” button when setting a breakpoint in a recursive method. I remember discussing this issue recently in context of a different user-facing bug, but I can't find the record at the moment. The fix may be as simple as delaying state restore until previous state save is completed, but there may be other side effects. We should at least try the simple fix in 3.8. If you code up a patch (which would be very welcome) please do so off the flex_viewer_refactor branch. I'm hoping to merge that branch into master soon.
Pawel, I’ve checked your branch flex_viewer_refactor. The race is fixed there. You post “save” and “restore” on UI thread, so they are coming one after another. The problem is that they are coming in the wrong order. “Restore” comes first and then “save”. TreeModelContentProvider.inputChanged() is hit first and after that is called TreeModelContentProvider.inputAboutToChange(). That’s why the logged bug is still reproducible on your branch.
(In reply to comment #4) Unfortunately it's not a matter of simple ordering. The save sequence is asynchronous with several calls to the model. So the restore would have to be chained to the end of the save.
I’ve made a unit test for this, but I don’t know to create the patch for you. I got the sources from git with egit installed. It seems that “Create patch” option is no longer available under the “Team” menu. Only “Apply patch” is present.
Created attachment 204610 [details] unit test save restore order
(In reply to comment #6) > I’ve made a unit test for this, but I don’t know to create the patch for you. I > got the sources from git with egit installed. It seems that “Create patch” > option is no longer available under the “Team” menu. Only “Apply patch” is > present. To create a git patch you have to commit your changes to your local repository, open the history view and create a patch from your latest commit. See: http://wiki.eclipse.org/Platform-releng/Git_Workflows#Create_a_patch
Created attachment 204666 [details] proposed fix Added ITreeModelContentProvider.postInputChanged(IInternalTreeModelViewer, Object, Object) to guarantee that RESTORE is called after SAVE.
> (In reply to comment #4) > Unfortunately it's not a matter of simple ordering. The save sequence is > asynchronous with several calls to the model. So the restore would have to be > chained to the end of the save. Yes, but apparently this was not the issue here. “Restore” sequence checks first if there is a “save” sequence in progress and postpone itself after “save” finished. The issue is that “Restore” is started before “Save” sequence get a chance to start. This happens only for JFace viewer. The order of events is done by org.eclipse.jface.viewers.ContentViewer.setInput(Object) and it looks like this: 1) JFace viewer calls first contentProvider.inputChanget() -> start “restore” sequance 2) JFace view calls its hook for derived class inputChanged(this.input, oldInput) -> calls contentProvider.inputAboutToChange() -> starts “save ” sequance The cleanest way I found to guarantee the order of starting these sequences was to add ITreeModelContentProvider.postInputChanged() near ITreeModelContentProvider.inputAboutToChanged() and make sure they are called in the right order. What do you think?
(In reply to comment #8) > To create a git patch you have to commit your changes to your local repository, > open the history view and create a patch from your latest commit. See: > http://wiki.eclipse.org/Platform-releng/Git_Workflows#Create_a_patch Thank you. It worked after all, but I encountered some problems. See bug 354800.
(In reply to comment #10) > > (In reply to comment #4) > > Unfortunately it's not a matter of simple ordering. The save sequence is > > asynchronous with several calls to the model. So the restore would have to be > > chained to the end of the save. > Yes, but apparently this was not the issue here. “Restore” sequence checks > first if there is a “save” sequence in progress and postpone itself after > “save” finished. Interesting. I guess my memory is failing me. I'll take a look at it after sorting out the debug toolbar thing.
Thank you for the patch contribution :-) I committed the fix to the flex_viewer_refactor branch. Once it's merged into master, this bug can be marked as fixed. I wrote a couple of unit tests and found that the supplied fix didn't work in case the new input was instance equal to the old input. So I adjusted the patch to address that and to clean up the interface methods. http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?h=flex_viewer_refactor&id=ad3a8af79e06aba4ceffb5afacbb26680b070e6f (I screwed up the comment but as I tried to fix it, seems that I was making a mess of the history, so decided to leave it as is).
Comment on attachment 204610 [details] unit test save restore order I noticed this contributed test only after the fact. I added it as well: http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?h=flex_viewer_refactor&id=7d27f120d4a2a31754b27300b4804ef8eab95b93
Fixed along with commit for bug 161435.
