From 12749f54c97153f52a6175d93d2669dcc0e23ea8 Mon Sep 17 00:00:00 2001 From: Hugo Parente Lima Date: Wed, 12 Jun 2024 16:57:34 -0300 Subject: [PATCH] Implement a memory safe way to connect signals. Fixes #150 --- ecr/signal.ecr | 3 ++ spec/safe_signal_spec.cr | 68 +++++++++++++++++++++++++++++++ src/bindings/g_object/binding.yml | 1 + src/bindings/g_object/connect.cr | 58 ++++++++++++++++++++++++++ src/bindings/g_object/object.cr | 7 ++++ src/bindings/g_object/signal.cr | 4 +- src/generator/signal_gen.cr | 15 +++++++ 7 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 spec/safe_signal_spec.cr create mode 100644 src/bindings/g_object/connect.cr diff --git a/ecr/signal.ecr b/ecr/signal.ecr index 0be5f9f..864ec89 100644 --- a/ecr/signal.ecr +++ b/ecr/signal.ecr @@ -4,6 +4,9 @@ struct <%= signal_type %> < GObject::Signal @detail ? "<%= signal.name %>::#{@detail}" : "<%= signal.name %>" end + def validate_params(<%= validation_args %>) + end + def connect(*, after : Bool = false, &block : Proc(<%= lean_proc_params %>)) : GObject::SignalConnection connect(block, after: after) end diff --git a/spec/safe_signal_spec.cr b/spec/safe_signal_spec.cr new file mode 100644 index 0000000..834dd2e --- /dev/null +++ b/spec/safe_signal_spec.cr @@ -0,0 +1,68 @@ +require "./spec_helper" + +private class Obj < GObject::Object + getter int32 = 0 + getter bool = false + + signal int32_bool(int32 : Int32, bool : Bool) + + def initialize(int32_value : Int32, bool_value : Bool) + super() + # Uncomment this and the test will fail, because a reference is left on ClosureManager + # + # int32_bool_signal.connect do |int, bool| + # int32_bool_slot(int, bool) + # end + + GObject.connect(int32_bool_signal, int32_bool_slot(Int32, Bool)) + int32_bool_signal.emit(int32_value, bool_value) + end + + def int32_bool_slot(@int32, @bool) + end +end + +@[NoInline] +private def create_obj(int32_value : Int32, bool_value : Bool) + obj = Obj.new(int32_value, bool_value) + {WeakRef.new(obj), obj.int32, obj.bool} +end + +private class Subject < Test::Subject + getter enum : Test::RegularEnum = Test::RegularEnum::Value1 + + def initialize(enum_value : Test::RegularEnum) + super() + GObject.connect(enum_signal, enum_slot(Test::RegularEnum)) + enum_signal.emit(enum_value) + end + + def enum_slot(@enum) + end +end + +@[NoInline] +private def create_subject(enum_value) + obj = Subject.new(enum_value) + {WeakRef.new(obj), obj.enum} +end + +describe "Save signals" do + it "does not leak memory" do + obj_ref, int32, bool = create_obj(int32_value: 42_u32, bool_value: true) + int32.should eq(42) + bool.should eq(true) + # Need to call some obj_ref method to avoid LLVM optimizations that break this test. + obj_ref.inspect + GC.collect + obj_ref.value.should eq(nil) + end + + it "works for non-user signals the same way" do + obj_ref, enum_ = create_subject(enum_value: Test::RegularEnum::Value2) + enum_.should eq(Test::RegularEnum::Value2) + obj_ref.inspect + GC.collect + obj_ref.value.should eq(nil) + end +end diff --git a/src/bindings/g_object/binding.yml b/src/bindings/g_object/binding.yml index 37ab978..b53c781 100644 --- a/src/bindings/g_object/binding.yml +++ b/src/bindings/g_object/binding.yml @@ -14,6 +14,7 @@ require_after: - value.cr - signal_connection.cr - gi_crystal.cr + - connect.cr ignore_constants: - PARAM_MASK diff --git a/src/bindings/g_object/connect.cr b/src/bindings/g_object/connect.cr new file mode 100644 index 0000000..dab8f6b --- /dev/null +++ b/src/bindings/g_object/connect.cr @@ -0,0 +1,58 @@ +require "weak_ref" + +module GObject + @[Experimental("Join discussion about this topic at https://github.com/hugopl/gi-crystal/issues/150")] + macro connect(signal, handler) + begin + %box = ::Box.box(WeakRef.new(self)) + %sig = {{ signal }} + %sig.validate_params({{ handler.args.splat }}) + + # Declare "C" slot + %c_slot = ->(sender : Void*, + {% i = 0 %} + {% for arg in handler.args %} + {% + resolved_type = arg.resolve + if resolved_type == String || resolved_type == Path + type = ::Pointer(UInt8) + elsif resolved_type == Bool + type = ::Int32 + else + type = resolved_type + end + %} + c_arg_{{ i }} : {{ type }}, + {% i += 1 %} + {% end %} + box : Void*) { + + ref = ::Box(WeakRef(typeof(self))).unbox(box).value || raise GICrystal::ObjectCollectedError.new + + # Convert from C + {% i = 0 %} + {% for arg in handler.args %} + {% resolved_type = arg.resolve %} + {% if resolved_type == String %} + arg_{{ i }} = String.new(c_arg_{{ i }}) + {% elsif resolved_type == Path %} + arg_{{ i }} = Path.new(String.new(c_arg_{{ i }}) + {% elsif resolved_type == Bool %} + arg_{{ i }} = GICrystal.to_bool(c_arg_{{ i }}) + {% else %} + arg_{{ i }} = c_arg_{{ i }} + {% end %} + {% i += 1 %} + {% end %} + + ref.{{ handler.name }}( {{ (0...handler.args.size).map { |i| "arg_#{i}".id }.splat }} ) + nil + } + sig_handler_id = LibGObject.g_signal_connect_data(%sig.source, %sig.name, %c_slot.pointer, + GICrystal::ClosureDataManager.register(%box), + ->GICrystal::ClosureDataManager.deregister, 0_u32) + + GObject::SignalConnection.new(%sig.source, sig_handler_id) + end + end +end diff --git a/src/bindings/g_object/object.cr b/src/bindings/g_object/object.cr index 4a47838..b9a6670 100644 --- a/src/bindings/g_object/object.cr +++ b/src/bindings/g_object/object.cr @@ -532,6 +532,13 @@ module GObject alias LeanProc = Proc({{ (signature.args.map(&.type) << Nil).splat }}) + def validate_params( + {% for arg in signature.args %} + {{ arg }}.class, + {% end %} + ) + end + def connect(*, after : Bool = false, &block : LeanProc) : GObject::SignalConnection connect(block, after: after) end diff --git a/src/bindings/g_object/signal.cr b/src/bindings/g_object/signal.cr index e697dca..fb210aa 100644 --- a/src/bindings/g_object/signal.cr +++ b/src/bindings/g_object/signal.cr @@ -2,8 +2,8 @@ require "./signal_connection" module GObject abstract struct Signal - @source : GObject::Object - @detail : String? + getter source : GObject::Object + getter detail : String? def initialize(@source, @detail = nil) end diff --git a/src/generator/signal_gen.cr b/src/generator/signal_gen.cr index 2f5968d..72bf3e6 100644 --- a/src/generator/signal_gen.cr +++ b/src/generator/signal_gen.cr @@ -41,6 +41,21 @@ module Generator end end + private def validation_args : String + String.build do |s| + @args_strategies.each do |arg_strategy| + next if arg_strategy.remove_from_declaration? + + arg = arg_strategy.arg + arg_type_info = arg.type_info + nullmark = " | Nil" if arg.nullable? + + s << to_identifier(arg.name) << " : " + s << to_crystal_type(arg_type_info, include_namespace: true) << ".class" << nullmark << ',' + end + end + end + private def lean_proc_params : String String.build do |s| arg_strategies_to_proc_param_string(s, @signal, @args_strategies)