Bug 359859

Comment 1 Dorin Ciuca 2011-10-04 11:28:12 EDT

Created attachment 204518 [details]
java test to reproduce the bug

java test to reproduce the bug

Comment 2 Dorin Ciuca 2011-10-04 12:10:42 EDT

Debugging the code, it looks like a race between thread doing  fViewerStates.get() in ModelContentProvider and the thread doing fViewerStates.put().

Comment 3 Pawel Piech 2011-10-04 12:33:41 EDT

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.

Comment 4 Dorin Ciuca 2011-10-05 08:27:20 EDT

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.

Comment 5 Pawel Piech 2011-10-05 09:14:12 EDT

(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.

Comment 6 Dorin Ciuca 2011-10-05 11:37:18 EDT

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.

Comment 7 Dorin Ciuca 2011-10-05 12:15:52 EDT

Created attachment 204610 [details]
unit test save restore order

Comment 8 Curtis Windatt 2011-10-05 12:36:38 EDT

(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

Comment 9 Dorin Ciuca 2011-10-06 09:46:12 EDT

Created attachment 204666 [details]
proposed fix

Added ITreeModelContentProvider.postInputChanged(IInternalTreeModelViewer, Object, Object) to guarantee that RESTORE is called after SAVE.

Comment 10 Dorin Ciuca 2011-10-06 10:16:50 EDT

> (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?

Comment 11 Dorin Ciuca 2011-10-06 10:45:41 EDT

(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.

Comment 12 Pawel Piech 2011-10-06 14:37:40 EDT

(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.

Comment 13 Pawel Piech 2011-10-17 23:53:01 EDT

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 14 Pawel Piech 2011-10-18 00:27:57 EDT

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

Comment 15 Pawel Piech 2011-12-05 17:28:23 EST

Fixed along with commit for bug 161435.