Comment 1 Andrey Loskutov 2020-05-20 07:47:52 EDT

Created attachment 282952 [details]
Second crash dump, now with SIGSEGV

I've managed to get two crashes in few minutes, but after an hour no other crashes. Looks like I don't do something is a key to crash.

Steps I did were:

1) Start fresh Eclipse I-Build with an existing workspace, that I always use to install new plugins into SDK.
2) "File -> Import -> Install Software Items from File".
3) Select in the combo box already existing entry for the file /data/4x_platform_workspace/eclipse.platform.ui/releng/org.eclipse.ui.releng/platformUiTools.p2f
4) After selection in the combo, the table below is filled with install entries
5) Now just click inside the table to select one of the entries
6) Eventually scroll down (I wanted confirm that see new entry I've just added to the end of the list) and crash.

Comment 2 Simeon Andreev 2020-05-20 08:29:33 EDT

Maybe a GTK3 debug build would help understand the crash?

https://wiki.eclipse.org/SWT/Devel/Gtk/Compiling_Gtk

https://wiki.eclipse.org/SWT/Devel/Gtk/Dev_guide#GTK_Versions_on_Fedora.2FRed_Hat

Comment 3 Alexander Kurtakov 2020-05-21 00:32:54 EDT

(In reply to Andrey Loskutov from comment #1)
> Created attachment 282952 [details]
> Second crash dump, now with SIGSEGV
> 
> I've managed to get two crashes in few minutes, but after an hour no other
> crashes. Looks like I don't do something is a key to crash.
> 
> Steps I did were:
> 
> 1) Start fresh Eclipse I-Build with an existing workspace, that I always use
> to install new plugins into SDK.
> 2) "File -> Import -> Install Software Items from File".
> 3) Select in the combo box already existing entry for the file
> /data/4x_platform_workspace/eclipse.platform.ui/releng/org.eclipse.ui.releng/
> platformUiTools.p2f
> 4) After selection in the combo, the table below is filled with install
> entries
> 5) Now just click inside the table to select one of the entries
> 6) Eventually scroll down (I wanted confirm that see new entry I've just
> added to the end of the list) and crash.

Does it crash for you everytime ? Did you miss a step? E.g. in in 6. you speak about added entry but there is nothing about adding one nor I can see a way to do it from this dialog.

Can you strip it down to pure SWT snippet? 
Quick try didn't crash for me (Fedora 32/Gtk 3.24.20)

Comment 4 Andrey Loskutov 2020-05-21 02:11:15 EDT

(In reply to Simeon Andreev from comment #2)
> Maybe a GTK3 debug build would help understand the crash?
> 
> https://wiki.eclipse.org/SWT/Devel/Gtk/Compiling_Gtk
> 
> https://wiki.eclipse.org/SWT/Devel/Gtk/Dev_guide#GTK_Versions_on_Fedora.
> 2FRed_Hat

Simeon, can you please try if you can reproduce?

Alex: as said, it happened few times and I believe I can reproduce if I do it fast enough and repeat opening of the wizard again and again. Looks like selection of rows happens before or while table is "created" by GTK, but I have no idea how is this possible if I see all the rows?

May be running stressng could slow down everything so it is reproducible often?

Comment 5 Simeon Andreev 2020-05-21 03:33:19 EDT

(In reply to Andrey Loskutov from comment #4)
> Simeon, can you please try if you can reproduce?

I reproduced it once already, quite fast too. Do you mean, try to create an SWT-only snippet? That will probably be difficult; I assume something jface does is involved, and jface does complicated things very often. Its likely easier for us to debug the issue. Alexander, do you have suggestions for us?

Comment 6 Simeon Andreev 2020-05-21 04:00:53 EDT

Created attachment 282971 [details]
JDK crash log with GTK3 debug build.

Stack: [0x00007ffff7ecd000,0x00007ffff7fce000],  sp=0x00007ffff7fc5e10,  free space=995k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
C  [libgtk-3.so.0+0x4a9048]  _gtk_tree_path_new_from_rbtree+0xf1
C  [libgtk-3.so.0+0xb572d]  cell_info_get_path+0x27
C  [libgtk-3.so.0+0xb5da9]  set_cell_data+0xc2
C  [libgtk-3.so.0+0xb8a1a]  _gtk_tree_view_accessible_changed+0xa0
C  [libgtk-3.so.0+0x4a753a]  gtk_tree_view_row_changed+0x180
C  [libgobject-2.0.so.0+0xf988]  g_closure_invoke+0x138

Comment 7 Simeon Andreev 2020-05-21 04:20:43 EDT

I think clicking on the combo (maybe after scrolling a lot) and then clicking on a tree item is how to reproduce the crash.

Comment 8 Simeon Andreev 2020-05-21 07:45:49 EDT

Created attachment 282974 [details]
Installation file to use when reproducing.

The best sequence I got so far is:

1. Start in a new workspace.
2. Import Projects -> Install SW from file.
3. Choose the attached "ui_egit_tools_Photon.p2f".
4. Double the pop-up dialogs height.
5. Select 1 line in the tree.
6. Click on the combo text area.
7. Scroll down with mouse-wheel in the tree.
8. Click on one of the checkboxes.

Comment 9 Simeon Andreev 2020-05-21 11:07:42 EDT

From added g_warning() outputs, as far as I can tell this pointer becomes NULL within the if block at the end of _gtk_tree_path_new_from_rbtree (in gtktreeview.c):

      if (last)
	{
	  count = 1 + last->left->count;
	  tmp_node = last->parent;
	}

When I change the code as follows:

      if (last)
	{
	  g_warning("tag 4.55\n");
	  if (last != NULL){
	    g_warning("last is not null\n");
	  } else {
	    g_warning ("last is null\n");
	  }
	  g_warning("tag 4.56\n");
	  if (last != NULL && last->left != NULL) {
	    g_warning ("last left child is not null\n");
	  } else {
	    g_warning ("last or its left child is null\n");
	  }
	  g_warning("tag 4.57\n");
	  gint llc = last->left->count;
	  g_warning("tag 4.6\n");
	  count = 1 + llc;
	  g_warning("tag 4.7\n");
	  tmp_node = last->parent;
	}
    }

When the JDK crashes, I only see output:

(Eclipse:14457): Gtk-WARNING **: 17:06:09.807: tag 4.55

(Eclipse:14457): Gtk-WARNING **: 17:06:09.807: last is not null

(Eclipse:14457): Gtk-WARNING **: 17:06:09.807: tag 4.56

So something is deleting a tree node in parallel. Any ideas Alexander?

Comment 10 Alexander Kurtakov 2020-05-21 11:11:27 EDT

Sorry, you seem to better informed on the topic than me right now.

Comment 11 Simeon Andreev 2020-05-22 02:37:29 EDT

Created attachment 282987 [details]
Video showing the crash with a jface TreeViewer snippet, which sets input on focus lost.

JFace snippet for reproduction:

public class TestTreeViewer {

	public static void main(String[] args) {
		Display display = new Display();
		Shell shell = new Shell(display);
		shell.setSize(600, 400);
		shell.setLayout(new FillLayout());
		Composite composite = new Composite(shell, SWT.NONE);
		composite.setLayout(new FillLayout(SWT.VERTICAL));

		Combo c = new Combo(composite, SWT.NONE);
		c.add("1");
		c.add("2");
		
		int n = 20;
		
		TreeViewer viewer = new TreeViewer(composite, SWT.NONE);
		viewer.setContentProvider(new ITreeContentProvider() {

			@Override
			public Object[] getElements(Object inputElement) {
				if (inputElement instanceof Object[]) {
					return (Object[]) inputElement;
				}
				return null;
			}

			@Override
			public Object[] getChildren(Object parentElement) {
				return null;
			}

			@Override
			public Object getParent(Object element) {
				return null;
			}

			@Override
			public boolean hasChildren(Object element) {
				return false;
			}
		});
		viewer.getTree().addFocusListener(new FocusAdapter() {
			@Override
			public void focusLost(FocusEvent e) {
				viewer.setInput(createStrings(n));
			}
		});
		viewer.setInput(createStrings(n));

		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}

	private static String[] createStrings(int n) {
		String[] strings = new String[n];
		for (int i = 0; i < n; ++i) {
			strings[i] = "String " + i;
		}
		return strings;
	}
}

Alternate clicking on the combo (top), clicking and scrolling in the tree viewer (bottom).

I'll see if I can reproduce with pure SWT, it should be possible.

Comment 12 Simeon Andreev 2020-05-22 02:54:07 EDT

Created attachment 282988 [details]
SWT snippet to reproduce the problem with. Alternate clicks on the combo, tree and scrolling in the tree.

Comment 13 Simeon Andreev 2020-05-22 05:22:46 EDT

Next steps will likely be either or both:

1. Disable the focus lost listener in org.eclipse.equinox.internal.p2.importexport.internal.wizard.AbstractPage, or try doing something else in ImportPage.handleDestinationChanged() (maybe check if something actually changed...).
2. Try to create a GTK3 snippet and report a bug at the GTK+ tracker, then wait 1 year and maybe someone will comment on the bug. I'm not sure how to go about creating the snippet though; we would have to add and remove elements to the tree dynamically, not something I've done so far with GTK3 API.

Andrey?

Comment 14 Simeon Andreev 2020-05-22 05:29:23 EDT

Note that my snippets are somewhat different than the actual code in PDE. The focus lost listener is on the combo, not on the tree. I just noticed this while checking ImportPage/AbstractPage for potential changes.

Comment 15 Andrey Loskutov 2020-05-22 05:49:47 EDT

(In reply to Simeon Andreev from comment #13)
> Next steps will likely be either or both:
> 
> 1. Disable the focus lost listener in
> org.eclipse.equinox.internal.p2.importexport.internal.wizard.AbstractPage,
> or try doing something else in ImportPage.handleDestinationChanged() (maybe
> check if something actually changed...).
> 2. Try to create a GTK3 snippet and report a bug at the GTK+ tracker, then
> wait 1 year and maybe someone will comment on the bug. I'm not sure how to
> go about creating the snippet though; we would have to add and remove
> elements to the tree dynamically, not something I've done so far with GTK3
> API.
> 
> Andrey?

I'm not in favor of fixing p2 UI, I would better check if we can fix SWT to avoid the crash in general.

The question is: can we automate the snippet to crash fully automated, via code (produce same sequence that leads to manual crash)? If yes, I assume we have some states in SWT that allow API users to produce the crash. May be we aren't allowed to operate on trees during focus lost events, or something like that. If so, may be we can add some guards in SWT and use async exec in such cases, just as an idea.

Comment 16 Andrey Loskutov 2020-05-22 05:51:27 EDT

*** Bug 553073 has been marked as a duplicate of this bug. ***

Comment 17 Simeon Andreev 2020-05-23 01:57:37 EDT

From debug outputs, in _gtk_tree_path_new_from_rbtree() the passed RB tree has a member pointer "parent_node" with a weird address during the crash. I've seen 0x20, 0x40 and 0x60 addresses, but also an OK looking address *once* (no idea if that address was pointing to an RB tree node object though). I've tried to add more debug outputs to see when this broken address is set to no avail.

Next week I'll try to reproduce with a GTK3 snippet, hopefully with that we can create a GTK+ bug and we receive some feedback.

With the SWT only snippet the tree elements can be reduced to 3 and scrolling is not required to reproduce the crash. Just alternate clicking on tree items and on the combo.

I was unable to create an automated snippet that reproduces the issue. I also don't see how that is necessary, other than to add a regression test. The PDE/JFace already uses SWT API to produce a crash and IMO the API usage itself looks fine. If a combo loses focus, repopulate the tree (I guess this could be smarter to trigger repopulate only on actually changed combo value...). Not sure what more can be proven by another snippet.

I don't know about fixing the issue in SWT. Maybe we can offload the focus lost event in a Display.asyncExec() and that doesn't run into the crash. But that would change current behaviour quite a lot. I don't see what else we can change, without doing something awkward like looking at the call stack. Also probably there are other crash scenarios, and the focus lost event is only 1 occurrence.

I guess we'll have to find out why GTK3 code crashes. Alexander, is there some mailing list we can use to ask GTK+ developers for hints? I've never been that knowledgeable about C pointers, its hard for me to track when a pointer changes its address. Also any idea if an SWT snippet can be ran with valgrind?

Comment 18 Simeon Andreev 2020-05-25 04:51:50 EDT

When writing a GTK3 snippet, I'm having trouble including the code for the fix of bug 411452:

(TreeCrash:56905): GLib-GObject-WARNING **: 10:37:05.878: invalid unclassed pointer in cast to 'GtkTreeModel'

(TreeCrash:56905): Gtk-CRITICAL **: 10:37:05.878: gtk_tree_view_set_model: assertion 'model == NULL || GTK_IS_TREE_MODEL (model)' failed

(TreeCrash:56905): GLib-GObject-CRITICAL **: 10:37:05.878: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

When uncomment lines like this, I don't think I can reproduce the problem any more:

diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java
index 7adccf7cef..a28e98b520 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java 
+++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java 
@@ -2927,9 +2927,9 @@ public void removeAll () {
         * and this causes AAIOB in getId as items are already cleared but the model is not yet.
         * By disconnecting the model from the handle while clearing no intermediate signals are emitted.
         */
-       GTK.gtk_tree_view_set_model(handle, 0);
+       //GTK.gtk_tree_view_set_model(handle, 0);
        GTK.gtk_tree_store_clear (modelHandle);
-       GTK.gtk_tree_view_set_model(handle, modelHandle);
+       //GTK.gtk_tree_view_set_model(handle, modelHandle);
        if (fixAccessibility ()) {
                ignoreAccessibility = false;
                OS.g_object_notify (handle, OS.model);

I'll see if I can include this in my GTK3 snippet somehow, maybe its the reason for the crash.

Comment 19 Andrey Loskutov 2020-05-25 05:01:29 EDT

(In reply to Simeon Andreev from comment #18)
> When writing a GTK3 snippet, I'm having trouble including the code for the
> fix of bug 411452
> [...]
> I'll see if I can include this in my GTK3 snippet somehow, maybe its the
> reason for the crash.

See https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b2711372e321f1d46148c930a5bd50d663263ba4

@Alexander: any insights? You were involved in the bug 411452 fix, and the commit message starts with "Hack for GTK3" :-).

Comment 20 Simeon Andreev 2020-05-25 05:24:26 EDT

Created attachment 283005 [details]
GTK3 snippet to reproduce the crash with. Alternate clicking on the button, scrolling in the tree and clicking in the tree.

I can reproduce with this GTK3 snippet. I'll record a video and open a ticket in the GTK+ tracker. Setting the tree model to NULL before doing big changes to the model seems to be advertised e.g. here:

https://en.wikibooks.org/wiki/GTK%2B_By_Example/Tree_View/Tree_Models#Speed_Issues_when_Adding_a_Lot_of_Rows

Its supposed to speed up the process. Though I'm not sure how official this wiki is.

Comment 21 Simeon Andreev 2020-05-25 06:33:40 EDT

I've reported: https://gitlab.gnome.org/GNOME/gtk/-/issues/2786

Hopefully we get some feedback. I have no idea why the GTK+ code is crashing, other than that there is a pointer that points to nothing meaningful.

Comment 22 Simeon Andreev 2020-05-25 10:53:41 EDT

We got a relatively quick response from GTK+ developers, that the RHEL 7 GTK version (3.22.30) is ancient. I've reproduced the issue on Fedora 32 (64 bit) with GTK 3.24.20, which is considered recent.

This makes me believe that any fix coming from GTK+ developers will not be added to GTK 3.22.x. This likely means we need a workaround of some sort in SWT/JFace. 

If we do get a patch relatively quickly (its a crash after all), the patch might shed some light on a potential workaround. But I doubt it; so the sooner we start looking into a workaround, I guess the sooner this is resolved for our case. Or of course we can ignore it and hope users don't run into it, at least until we upgrade to whatever RHEL version has the fix?

Andrey WDYT?

Comment 23 Soraphol (Paul) Damrongpiriyapong 2020-05-28 16:48:29 EDT

Hello, I have been trying to reproduce this bug to see if I can work a patch out to fix the crash. From my experience of trying to reproduce, it seems like some sort of race condition, between removing the items from the tree & adding them back. 

I tested the race condition by having it print every time it creates a new tree item & when the whole tree refreshes. The item in which the crash occurs varies that's why I think its some sort of race.

I also tried to reproduce the bug according to the "install project from software" and haven't gotten any luck yet (makes it seem even more like a race).

However, this is the first time I am delving into this code so I am not sure if multiple threads even have access to the same tree here. This is just my ideas I will continue investigating.

Comment 24 Andrey Loskutov 2020-05-28 16:56:52 EDT

(In reply to Soraphol (Paul) Damrongpiriyapong from comment #23)
> I am not sure
> if multiple threads even have access to the same tree here. This is just my
> ideas I will continue investigating.

If we talk about JVM process, the SWT is single threaded so any calls into GTK from JVM should be thread safe, from one dedicated thread. I don't know anything about GTK3, if there can be multiple native threads.

Comment 25 Soraphol (Paul) Damrongpiriyapong 2020-06-01 15:29:08 EDT

From testing in both Java & pure C, the problem seems to stem from the 2  gtk_tree_view_set_model() calls, making it NULL and then reassigning. 

Although the comment says that this fixes Bug 411452, from my tests (by doing the reproduction steps of the bug), there seems to be no problem.

This was the "fix":
//	GTK.gtk_tree_view_set_model(handle, 0);
	GTK.gtk_tree_store_clear (modelHandle);
//	GTK.gtk_tree_view_set_model(handle, modelHandle);

Comment 26 Andrey Loskutov 2020-06-01 15:36:44 EDT

(In reply to Soraphol (Paul) Damrongpiriyapong from comment #25)
> From testing in both Java & pure C, the problem seems to stem from the 2 
> gtk_tree_view_set_model() calls, making it NULL and then reassigning. 
> 
> Although the comment says that this fixes Bug 411452, from my tests (by
> doing the reproduction steps of the bug), there seems to be no problem.
> 
> This was the "fix":
> //	GTK.gtk_tree_view_set_model(handle, 0);
> 	GTK.gtk_tree_store_clear (modelHandle);
> //	GTK.gtk_tree_view_set_model(handle, modelHandle);

If by reverting the fix for bug 411452 the crashes disappear, and the problem described in bug 411452 do not appear, I'm for reverting that. The patch was made 7 years ago, and GTK3 itself along with SWT support for GTK3 at that time was full of bugs. Now we still have lot of GTK3 issues, but I guess the particular one could be fixed meanwhile.

Sorephol, do you want to provide a Gerrit?

Comment 27 Soraphol (Paul) Damrongpiriyapong 2020-06-01 15:40:42 EDT

Yep, I'll create a Gerrit change. I would love if you or someone else could also test the fix as well. Just want to make sure my tests are consistent.

Comment 28 Eclipse Genie 2020-06-01 15:55:06 EDT

New Gerrit change created: https://git.eclipse.org/r/163949

Comment 29 Eclipse Genie 2020-06-09 15:34:29 EDT

Gerrit change https://git.eclipse.org/r/163949 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=f0e5443edc48b0e62dc70d93b75a95c62a611e9b

Comment 30 Soraphol (Paul) Damrongpiriyapong 2020-06-10 16:36:17 EDT

Verified:  

Eclipse SDK
Version: 2020-09 (4.17)
Build id: I20200609-1800
OS: Linux, v.5.6.14-300.fc32.x86_64, x86_64 / gtk 3.24.20
Java version: 1.8.0_252