Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Attempt at getting Gdk events to get finalized and freed on the UI th… #254

Closed
wants to merge 3 commits into from

Conversation

Therzok
Copy link
Contributor

@Therzok Therzok commented Dec 4, 2018

…read

Possible fix for VSTS #739030 - Visual Studio Community 7.7 for Mac - crahses when holding down cursor key

…read

Possible fix for VSTS #739030 - Visual Studio Community 7.7 for Mac - crahses when holding down cursor key
@Therzok Therzok force-pushed the gdk-event-finalize branch from cf4c089 to 24a198d Compare December 4, 2018 01:23
gdk/Event.cs Outdated
if (!owned)
System.GC.SuppressFinalize (this);
}

static object lockObject = new object ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static readonly ?

gdk/Event.cs Outdated
{
if (owned) {
System.GC.SuppressFinalize (this);
gdk_event_free (Handle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zero out Handle to prevent crashes due to double free?

Copy link
Member

@alanmcgovern alanmcgovern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One double-Dispose bug!

gdk/Event.cs Outdated
static List<Event> PendingFrees = new List<Event> ();
static bool idleQueued;

static GLib.TimeoutHandler PerformQueuedFreesHandler = PerformQueuedFrees;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static readonly here too

@Therzok
Copy link
Contributor Author

Therzok commented Dec 9, 2018

Updated with PR feedback.

@slluis
Copy link
Member

slluis commented Dec 11, 2018

@alanmcgovern can you review?

Copy link

@bratsche bratsche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to test this, but 👍 lgtm.

@Therzok
Copy link
Contributor Author

Therzok commented Dec 11, 2018

using System;
using System.IO;
using Gdk;
using Gtk;

public partial class MainWindow : Gtk.Window
{
    public MainWindow() : base(Gtk.WindowType.Toplevel)
    {
        Build();
    }

	protected override bool OnExposeEvent(EventExpose evnt)
	{
		for (int i = 0; i < 1000000; ++i)
		{
			var ev = Gtk.Application.CurrentEvent;
			var copy = new Gdk.Event(ev.Handle, true);
		}

		System.GC.Collect();
		System.GC.Collect();
		System.GC.Collect();
		System.GC.WaitForPendingFinalizers();

		return base.OnExposeEvent(evnt);
	}
}

    public class MainClass
    {
        public static void Main(string[] args)
        {
            Application.Init();

			var mw = new MainWindow();
			mw.ShowAll();

			Application.Run();

		}
    }

@Therzok
Copy link
Contributor Author

Therzok commented Dec 11, 2018

Actually, this doesn't fix the issue...

The event goes out of scope and it gets freed by native gtk. I think our only solution is to make it a ref struct and avoid the value from escaping, otherwise it'll just free invalid memory.

@Therzok
Copy link
Contributor Author

Therzok commented Dec 11, 2018

I thought it was an UI thread bug, but it can still happen.

@Therzok
Copy link
Contributor Author

Therzok commented Dec 11, 2018

EventOwnerChange as it's not read-only, the rest of the events could be solved by just making a copy of the GdkEvent.

@Therzok
Copy link
Contributor Author

Therzok commented Apr 5, 2019

Closing. The problem with Gdk.Event is how it's bound and it requires breaking API to fix. We worked around it in VSMac by manually pinvoking

@Therzok Therzok closed this Apr 5, 2019
@Therzok Therzok deleted the gdk-event-finalize branch April 5, 2019 12:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants