[GTK4] Fix segmentation fault when creating multiple displays #1832

ptziegler · 2025-02-14T22:37:41Z

Subclasses are not created in GTK4, leading to the assignment of NULL pointers when disposing the display. If a second display is created afterwards, this leads to a segmentation fault.

Contributes to
#1663

ptziegler · 2025-02-14T22:40:01Z

See

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java

Lines 3774 to 3806 in 857ac0b

    
           void initializeSubclasses () { 
        
           	if (!GTK.GTK4) { 
        
           		long pangoLayoutType = OS.PANGO_TYPE_LAYOUT (); 
        
           		long pangoLayoutClass = OS.g_type_class_ref (pangoLayoutType); 
        
           		pangoLayoutNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (pangoLayoutClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (pangoLayoutClass, OS.pangoLayoutNewProc_CALLBACK(pangoLayoutNewProc)); 
        
           		OS.g_type_class_unref (pangoLayoutClass); 
        
           		long imContextType = GTK.GTK_TYPE_IM_MULTICONTEXT (); 
        
           		long imContextClass = OS.g_type_class_ref (imContextType); 
        
           		imContextNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (imContextClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (imContextClass, OS.imContextNewProc_CALLBACK(imContextNewProc)); 
        
           		OS.g_type_class_unref (imContextClass); 
        
           		long pangoFontFamilyType = OS.PANGO_TYPE_FONT_FAMILY (); 
        
           		long pangoFontFamilyClass = OS.g_type_class_ref (pangoFontFamilyType); 
        
           		pangoFontFamilyNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (pangoFontFamilyClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (pangoFontFamilyClass, OS.pangoFontFamilyNewProc_CALLBACK(pangoFontFamilyNewProc)); 
        
           		OS.g_type_class_unref (pangoFontFamilyClass); 
        
           		long pangoFontFaceType = OS.PANGO_TYPE_FONT_FACE (); 
        
           		long pangoFontFaceClass = OS.g_type_class_ref (pangoFontFaceType); 
        
           		pangoFontFaceNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (pangoFontFaceClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (pangoFontFaceClass, OS.pangoFontFaceNewProc_CALLBACK(pangoFontFaceNewProc)); 
        
           		OS.g_type_class_unref (pangoFontFaceClass); 
        
           		long printerOptionWidgetType = GTK.gtk_printer_option_widget_get_type(); 
        
           		long printerOptionWidgetClass = OS.g_type_class_ref (printerOptionWidgetType); 
        
           		printerOptionWidgetNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (printerOptionWidgetClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (printerOptionWidgetClass, OS.printerOptionWidgetNewProc_CALLBACK(printerOptionWidgetNewProc)); 
        
           		OS.g_type_class_unref (printerOptionWidgetClass); 
        
           	} 
        
           }

Two questions though:

On GTK3, the printerOptionWidgetNewProc is allocated but never freed, when disposed. I don't think this is deliberate, but rather a resource leak?
Would this still be fine for the RC1 or is it safer to postpone it until after the 2025-03 release?

iloveeclipse · 2025-02-14T22:49:27Z

On GTK3, the printerOptionWidgetNewProc is allocated but never freed, when disposed. I don't think this is deliberate, but rather a resource leak?

Probably leak, but can wait for 4.36 as not new.

Would this still be fine for the RC1 or is it safer to postpone it until after the 2025-03 release?

This patch seem to be safe for GTK3, and only improves (anyway bad state of) GTK4, so would be OK for RC1. On the other side, there is no reason to rush for exact same reason, so I would propose to wait for 4.36 too. I know at least one company with patches for SWT Display class, and last minute changes are always disruptive.

github-actions · 2025-02-14T22:50:47Z

Test Results

381 files - 129 381 suites - 129 6m 18s ⏱️ - 2m 9s
4 326 tests - 19 4 313 ✅ - 18 13 💤 - 1 0 ❌ ±0
12 499 runs - 4 114 12 401 ✅ - 4 101 98 💤 - 13 0 ❌ ±0

Results for commit f95cb6b. ± Comparison against base commit cc4c178.

This pull request removes 19 tests.

org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_emptyString
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letterA
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letters
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16LE_null
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letter
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letters
…

♻️ This comment has been updated with latest results.

ptziegler · 2025-02-14T22:52:41Z

This patch seem to be safe for GTK3, and only improves (anyway bad state of) GTK4, so would be OK for RC1. On the other side, there is no reason to rush for exact same reason, so I would propose to wait for 4.36 too. I know at least one company with patches for SWT Display class, and last minute changes are always disruptive.

That's a fair point. Then I'll wait for the start of the next release cycle.

laeubi · 2025-02-15T04:56:25Z

I know at least one company with patches for SWT Display class, and last minute changes are always disruptive.

Then this company should at best contribute the patches so it becomes less disruptive for them.

This patch seem to be safe for GTK3, and only improves (anyway bad state of) GTK4, so would be OK for RC1.

I agree and would vote for merging this ASAP, it is not an "improvement" if the process crash with a core dump but a server bug.

laeubi · 2025-02-15T07:37:33Z

Lets see if it already improves the situation, I added the gtk4 label so it should run the tests with gtk4 enabled.

laeubi · 2025-02-15T09:10:26Z

The segmentation fault is gone, good job, now there are new error that popup but I think that's kind of expected.

ptziegler · 2025-02-15T11:07:11Z

At least from my side, the main motivation was to fix the early crash of the test suite. So this is not something I need to have in the RC1. And if the only benefit or merging this is to disrupt someone elses workflow, then I'm also leaning towards postponing this for the next release.

I agree and would vote for merging this ASAP, it is not an "improvement" if the process crash with a core dump but a server bug.

I don't think (or rather hope) there is anyone who uses the GTK4 productively. Especially since e.g. the UnsatisfiedLinkError when handling Expose events made the port effectively unusable, outside of the Snippets. So whether this is merged or not would hardly make a difference.

The segmentation fault is gone, good job, now there are new error that popup but I think that's kind of expected.

That is true, though now it seems like the run gets stuck while executing the Browser tests. 😦

laeubi · 2025-02-15T11:29:06Z

And if the only benefit or merging this is to disrupt someone elses workflow

Every change breaks someones workflow :-P

https://xkcd.com/1172/

laeubi · 2025-02-16T11:13:09Z

That is true, though now it seems like the run gets stuck while executing the Browser tests. 😦

I think it would be fine to skip the browsertest with an assumeThat(Version!=GTK4)

akurtakov · 2025-03-05T07:11:29Z

@ptziegler What do you mean by "Subclasses are not created in GTK4" ?

ptziegler · 2025-03-05T07:45:52Z

@ptziegler What do you mean by "Subclasses are not created in GTK4" ?

This method is effectively skipped in GTK4. It's not the Java inheritance I'm talking about, but e.g. the Pango Layout subclasses that are set when the Display is created. Perhaps I can rephrase it to make it less confusing.

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java

Lines 3774 to 3806 in 32032fb

    
           void initializeSubclasses () { 
        
           	if (!GTK.GTK4) { 
        
           		long pangoLayoutType = OS.PANGO_TYPE_LAYOUT (); 
        
           		long pangoLayoutClass = OS.g_type_class_ref (pangoLayoutType); 
        
           		pangoLayoutNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (pangoLayoutClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (pangoLayoutClass, OS.pangoLayoutNewProc_CALLBACK(pangoLayoutNewProc)); 
        
           		OS.g_type_class_unref (pangoLayoutClass); 
        
           		long imContextType = GTK.GTK_TYPE_IM_MULTICONTEXT (); 
        
           		long imContextClass = OS.g_type_class_ref (imContextType); 
        
           		imContextNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (imContextClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (imContextClass, OS.imContextNewProc_CALLBACK(imContextNewProc)); 
        
           		OS.g_type_class_unref (imContextClass); 
        
           		long pangoFontFamilyType = OS.PANGO_TYPE_FONT_FAMILY (); 
        
           		long pangoFontFamilyClass = OS.g_type_class_ref (pangoFontFamilyType); 
        
           		pangoFontFamilyNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (pangoFontFamilyClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (pangoFontFamilyClass, OS.pangoFontFamilyNewProc_CALLBACK(pangoFontFamilyNewProc)); 
        
           		OS.g_type_class_unref (pangoFontFamilyClass); 
        
           		long pangoFontFaceType = OS.PANGO_TYPE_FONT_FACE (); 
        
           		long pangoFontFaceClass = OS.g_type_class_ref (pangoFontFaceType); 
        
           		pangoFontFaceNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (pangoFontFaceClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (pangoFontFaceClass, OS.pangoFontFaceNewProc_CALLBACK(pangoFontFaceNewProc)); 
        
           		OS.g_type_class_unref (pangoFontFaceClass); 
        
           		long printerOptionWidgetType = GTK.gtk_printer_option_widget_get_type(); 
        
           		long printerOptionWidgetClass = OS.g_type_class_ref (printerOptionWidgetType); 
        
           		printerOptionWidgetNewProc = OS.G_OBJECT_CLASS_CONSTRUCTOR (printerOptionWidgetClass); 
        
           		OS.G_OBJECT_CLASS_SET_CONSTRUCTOR (printerOptionWidgetClass, OS.printerOptionWidgetNewProc_CALLBACK(printerOptionWidgetNewProc)); 
        
           		OS.g_type_class_unref (printerOptionWidgetClass); 
        
           	} 
        
           }

Subclasses are not created in GTK4, leading to the assignment of NULL pointers when disposing the display. If a second display is created afterwards, this leads to a segmentation fault. Contributes to eclipse-platform#1663

akurtakov · 2025-03-19T11:12:29Z

Jenkins succeeds. Merging.

ptziegler mentioned this pull request Feb 14, 2025

Add workflow to run with GTK4 enabled #1663

Closed

laeubi approved these changes Feb 15, 2025

View reviewed changes

laeubi requested a review from akurtakov February 15, 2025 04:57

laeubi added the gtk4 GTK4 issues label Feb 15, 2025

laeubi force-pushed the segfault-in-display branch from e50069d to c3cccad Compare February 15, 2025 07:36

ptziegler mentioned this pull request Feb 15, 2025

GtkPrinterOptionWidget GType not released after Display destruction #1838

Closed

akurtakov force-pushed the segfault-in-display branch from c3cccad to 061b6c1 Compare March 5, 2025 07:09

[GTK4] Fix segmentation fault when creating multiple displays

f95cb6b

Subclasses are not created in GTK4, leading to the assignment of NULL pointers when disposing the display. If a second display is created afterwards, this leads to a segmentation fault. Contributes to eclipse-platform#1663

akurtakov force-pushed the segfault-in-display branch from 061b6c1 to f95cb6b Compare March 19, 2025 09:39

akurtakov approved these changes Mar 19, 2025

View reviewed changes

akurtakov merged commit 9440140 into eclipse-platform:master Mar 19, 2025
13 of 14 checks passed

[GTK4] Fix segmentation fault when creating multiple displays #1832

[GTK4] Fix segmentation fault when creating multiple displays #1832

Uh oh!

Conversation

ptziegler commented Feb 14, 2025

Uh oh!

ptziegler commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse commented Feb 14, 2025

Uh oh!

github-actions bot commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

ptziegler commented Feb 14, 2025

Uh oh!

laeubi commented Feb 15, 2025

Uh oh!

laeubi commented Feb 15, 2025

Uh oh!

laeubi commented Feb 15, 2025

Uh oh!

ptziegler commented Feb 15, 2025

Uh oh!

laeubi commented Feb 15, 2025

Uh oh!

laeubi commented Feb 16, 2025

Uh oh!

akurtakov commented Mar 5, 2025

Uh oh!

ptziegler commented Mar 5, 2025

Uh oh!

akurtakov commented Mar 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ptziegler commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading