public inbox for guile-gtk@sourceware.org
 help / color / mirror / Atom feed
* Fixed memory leaks in gdk-1.2.defs
@ 2003-05-09  6:33 Marko Rauhamaa
  2003-05-12  1:05 ` Kevin Ryde
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Marko Rauhamaa @ 2003-05-09  6:33 UTC (permalink / raw)
  To: guile-gtk

Summary: Fixed memory leaks in gdk-1.2.defs.

Details:

 - build-guile-gtk-1.2, guile-gtk.c: GDK and GTK differ in the reference
   count policy. While GTK returns the widgets with a reference count 0,
   GDK returns them with a reference count 1. That means that
   gdk-1.2.defs should not increment the reference count -- but it was
   doing that.

   It turns out that the different GDK types don't need a copy option (=
   "method") at all whether they have reference counts or not. I made
   the copy option optional (previously the omission of the copy option
   caused a runtime crash).

 - gdk-1.2.defs: Added GdkFontType. Added it to GdkFont.

 - gdk-1.2.defs: Moved GdkFillRule among other enums.

 - gdk-1.2.defs: Removed copy options (previously: gdk_..._ref,
   gtk_no_copy, gtk_fake_copy, gdk_..._copy).

 - gdk-1.2.defs: Removed now redundant (copy #f) options from return
   values.

 - gdk-1.2.defs: Removed redundant conversion option from GdkFont.

 - gdk-1.2.defs: Changed GdkWChar from uint to uint32 (since it is now
   available).

 - gdk-1.2.defs: Removed useless size options -- but left it with
   gtimer, since define-struct still requires a dummy size.

 - gdk-1.2.defs: Indicated that gdk_font_intern and gdk_color_intern are
   now deprecated. (I don't think anybody used them anyway.)

 - gdk-1.2.defs, gdk-support.c, guile-gtk.h: Changed the signature of
   pixel-from-rgb from (visual red green blue) to (visual rgb) for
   consistency's sake.

 - gdk-support.c, gtk-compat.c: Now that copying is no longer done,
   needed to explicitly copy the return values of gdk-color-new,
   gdk-color-parse and gtk-style-get-white.

 - gdk-support.c, guile-gtk.h: Removed sgtk_gdk_drag_context_ref as
   no longer needed.


Marko

-- 
Marko Rauhamaa      mailto:marko@pacujo.net     http://pacujo.net/marko/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixed memory leaks in gdk-1.2.defs
  2003-05-09  6:33 Fixed memory leaks in gdk-1.2.defs Marko Rauhamaa
@ 2003-05-12  1:05 ` Kevin Ryde
  2003-05-14  1:18   ` Marko Rauhamaa
  2003-05-12  1:08 ` Kevin Ryde
  2003-06-18 23:31 ` GdkEvent copy under signal handler (was: Fixed memory leaks in gdk-1.2.defs) Kevin Ryde
  2 siblings, 1 reply; 9+ messages in thread
From: Kevin Ryde @ 2003-05-12  1:05 UTC (permalink / raw)
  To: Marko Rauhamaa; +Cc: guile-gtk

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

Marko Rauhamaa <marko@pacujo.net> writes:
>
>  - build-guile-gtk-1.2, guile-gtk.c: GDK and GTK differ in the reference
>    count policy. While GTK returns the widgets with a reference count 0,
>    GDK returns them with a reference count 1. That means that
>    gdk-1.2.defs should not increment the reference count -- but it was
>    doing that.

I don't know if it's this change or something else, but I seem to now
be losing references to GdkFont.  The program below prints something
like for me (i386 debian with current cvs guile),

    #<GdkFont 8058b28>
    id 2400002
    id 2400002
    id 2400002
    id 0
    id 0
    id 0
    ...

It seems the font is freed, despite having an "f" variable referring
to it.  I put a printf in boxed_free and saw it getting released at
the point "id" changes.  Dunno where the fault lies though.

The symptom in my program was a gdk-draw-string failing on account of
a bad font type (it was looking at freed memory I think).


[-- Attachment #2: foo.scm --]
[-- Type: text/plain, Size: 349 bytes --]

(use-modules (ice-9 format)
	     (gtk gdk)
	     (gtk gtk))

(define f (gdk-font-intern "fixed"))
(format #t "~a\n" f)
(format #t "id ~x\n" (gdk-font-id f))

(define wid (gtk-window-new 'toplevel))

(gtk-idle-add (lambda ()
		(let ((id (gdk-font-id f)))
		  (format #t "id ~x\n" id)
		  (gc))))

(gtk-widget-show-all wid)
(gtk-standalone-main wid)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixed memory leaks in gdk-1.2.defs
  2003-05-09  6:33 Fixed memory leaks in gdk-1.2.defs Marko Rauhamaa
  2003-05-12  1:05 ` Kevin Ryde
@ 2003-05-12  1:08 ` Kevin Ryde
  2003-05-12  7:55   ` Marko Rauhamaa
  2003-06-18 23:31 ` GdkEvent copy under signal handler (was: Fixed memory leaks in gdk-1.2.defs) Kevin Ryde
  2 siblings, 1 reply; 9+ messages in thread
From: Kevin Ryde @ 2003-05-12  1:08 UTC (permalink / raw)
  To: Marko Rauhamaa; +Cc: guile-gtk

Marko Rauhamaa <marko@pacujo.net> writes:
>
>  - gdk-1.2.defs: Indicated that gdk_font_intern and gdk_color_intern are
>    now deprecated. (I don't think anybody used them anyway.)

I've been using them for the purpose described in the README, namely
to get a conversion from a string just once.

I guess there's other ways to do that, but for upward compatibility
I'd suggest keeping those routines.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixed memory leaks in gdk-1.2.defs
  2003-05-12  1:08 ` Kevin Ryde
@ 2003-05-12  7:55   ` Marko Rauhamaa
  2003-05-15 23:03     ` Kevin Ryde
  0 siblings, 1 reply; 9+ messages in thread
From: Marko Rauhamaa @ 2003-05-12  7:55 UTC (permalink / raw)
  To: guile-gtk

Kevin Ryde <user42@zip.com.au>:

> Marko Rauhamaa <marko@pacujo.net> writes:
> >
> >  - gdk-1.2.defs: Indicated that gdk_font_intern and gdk_color_intern are
> >    now deprecated. (I don't think anybody used them anyway.)
> 
> I've been using them for the purpose described in the README, namely
> to get a conversion from a string just once.
> 
> I guess there's other ways to do that, but for upward compatibility
> I'd suggest keeping those routines.

Aha, didn't think of that.

I have submitted this generalization of the scheme: Whenever a
'conversion option is specified for a boxed or struct type, such an
"intern" function is generated automatically. However, I have chosen to
name the converter function ->type (for example: ->GdkFont, ->GdkColor)
for two reasons:

 - The arrow notation is more descriptive and idiomatic.

 - The "intern" suffix is actually misleading (no symbol table
   manipulation is going on).

For compatibility reasons I have retained the two "intern" functions but
still deprecate them.


Marko

-- 
Marko Rauhamaa      mailto:marko@pacujo.net     http://pacujo.net/marko/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixed memory leaks in gdk-1.2.defs
  2003-05-12  1:05 ` Kevin Ryde
@ 2003-05-14  1:18   ` Marko Rauhamaa
  0 siblings, 0 replies; 9+ messages in thread
From: Marko Rauhamaa @ 2003-05-14  1:18 UTC (permalink / raw)
  To: guile-gtk

Kevin Ryde <user42@zip.com.au>:

> Marko Rauhamaa <marko@pacujo.net> writes:
> >
> > - build-guile-gtk-1.2, guile-gtk.c: GDK and GTK differ in the
> >    reference count policy. While GTK returns the widgets with a
> >    reference count 0, GDK returns them with a reference count 1.
> >    That means that gdk-1.2.defs should not increment the reference
> >    count -- but it was doing that.
> 
> I don't know if it's this change or something else, but I seem to now
> be losing references to GdkFont.

Yeah, I was confused about the copying semantics. I have now submitted a
fix.

BTW, the copying rules are as follows:

  You MUST COPY the return value of a C function if the C function
  returns

  * a fresh object with reference count 0,

  * one of its arguments as is,

  * a field of one of its arguments as is or

  * a global object as is.


  You MUST NOT COPY the return value of a C function if the C function

  * returns a fresh object with a reference count 1,

  * returns one of its arguments after incrementing the reference count,

  * returns a field of one of its arguments after incrementing reference
    count or

  * dequeues an object and hands it over to the caller.


Marko

-- 
Marko Rauhamaa      mailto:marko@pacujo.net     http://pacujo.net/marko/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixed memory leaks in gdk-1.2.defs
  2003-05-12  7:55   ` Marko Rauhamaa
@ 2003-05-15 23:03     ` Kevin Ryde
  2003-05-16  1:02       ` Marko Rauhamaa
  0 siblings, 1 reply; 9+ messages in thread
From: Kevin Ryde @ 2003-05-15 23:03 UTC (permalink / raw)
  To: Marko Rauhamaa; +Cc: guile-gtk

Marko Rauhamaa <marko@pacujo.net> writes:
>
>  - The arrow notation is more descriptive and idiomatic.

The type names in mixed case don't appear in any function names
currently though do they?  Isn't it normally gtk-color-something?

>  - The "intern" suffix is actually misleading (no symbol table
>    manipulation is going on).

If you read it as short for "internalize" then it seems fairly clear.
Ie. a thing is brought to a canonical or efficient internal form.

Dunno who conceived these functions (was it Marius?) or if that was
what they were meant to stand for, but can always introduce a
retrospective justification :-).

> ->GdkFont, ->GdkColor

If it was up to me I'd leave the current arrangements alone.  They're
not really too terrible, and there's no need for too many departures
from plain vanilla gtk/gdk.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixed memory leaks in gdk-1.2.defs
  2003-05-15 23:03     ` Kevin Ryde
@ 2003-05-16  1:02       ` Marko Rauhamaa
  2003-05-17  8:08         ` Marko Rauhamaa
  0 siblings, 1 reply; 9+ messages in thread
From: Marko Rauhamaa @ 2003-05-16  1:02 UTC (permalink / raw)
  To: guile-gtk

Kevin Ryde <user42@zip.com.au>:

> Marko Rauhamaa <marko@pacujo.net> writes:
> >
> >  - The arrow notation is more descriptive and idiomatic.
> 
> The type names in mixed case don't appear in any function names
> currently though do they? Isn't it normally gtk-color-something?

I think the function naming scheme is an adaptation of the C function
names ("gtk_color_something") rather than the type names.

> >  - The "intern" suffix is actually misleading (no symbol table
> >    manipulation is going on).
> 
> If you read it as short for "internalize" then it seems fairly clear.
> Ie. a thing is brought to a canonical or efficient internal form.

"Intern" and "internalize" sounds to me like entering symbols into the
symbol table. "Coerce", "convert" and "cast" would sound like type
conversion. Scheme uses the arrow notation for coercion.


Marko

-- 
Marko Rauhamaa      mailto:marko@pacujo.net     http://pacujo.net/marko/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixed memory leaks in gdk-1.2.defs
  2003-05-16  1:02       ` Marko Rauhamaa
@ 2003-05-17  8:08         ` Marko Rauhamaa
  0 siblings, 0 replies; 9+ messages in thread
From: Marko Rauhamaa @ 2003-05-17  8:08 UTC (permalink / raw)
  To: guile-gtk

Marko Rauhamaa <marko@pacujo.net>:

> "Intern" and "internalize" sounds to me like entering symbols into the
> symbol table. "Coerce", "convert" and "cast" would sound like type
> conversion. Scheme uses the arrow notation for coercion.

I have reversed myself. The ->GdkType function is out, and
gdk_type_intern is in. The intern functions are generated automatically
when the conversion option is present.

The reason is a dual precedent:

  1. That's how it was before.

  2. GDK defines gdk_atom_intern, which clearly was the original model
     although it's not quite the same thing.


Marko

-- 
Marko Rauhamaa      mailto:marko@pacujo.net     http://pacujo.net/marko/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* GdkEvent copy under signal handler (was: Fixed memory leaks in gdk-1.2.defs)
  2003-05-09  6:33 Fixed memory leaks in gdk-1.2.defs Marko Rauhamaa
  2003-05-12  1:05 ` Kevin Ryde
  2003-05-12  1:08 ` Kevin Ryde
@ 2003-06-18 23:31 ` Kevin Ryde
  2 siblings, 0 replies; 9+ messages in thread
From: Kevin Ryde @ 2003-06-18 23:31 UTC (permalink / raw)
  To: guile-gtk

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

Marko Rauhamaa <marko@pacujo.net> writes:
>
>  - gdk-1.2.defs: Removed copy options (previously: gdk_..._ref,
>    gtk_no_copy, gtk_fake_copy, gdk_..._copy).

This included GdkEvent I take it.  It's best to give the full name of
each function and/or type, rather than "...", so they can be located
by a grep etc.

I believe this has introduced a bug in the handling of GdkEvent
objects.  An event passed to a signal handler is not copied, and can
end up being freed while still in use.

To observe this, add a fake version of gdk_event_free into guile-gtk.c

	void
	gdk_event_free (GdkEvent *e)
	{
	  printf ("gdk_event_free %p\n", e);
	}

And run a little program

	(use-modules (gtk gtk))
	(define w (gtk-window-new 'toplevel))
	(gtk-widget-add-events w '(pointer-motion-mask))
	(define e #f)
	(gtk-signal-connect w "motion_notify_event"
	                    (lambda (event)
	                      (format #t "prev event ~a\n" e)
	                      (set! e event)))
	(gtk-widget-show-all w)
	(gtk-main)

move the mouse across the window created.  The output is something
like

	gdk_event_free 0x80b1b7c
	prev event #<GdkEvent 80b1b7c>

0x80b1b7c has been freed while it's still in the "e" variable.


I propose to add back the copy option,

        * gdk-1.2.defs (GdkEvent): Reinstate copy option gdk_event_copy, and
        describe why it's there.

I believe all functions which return a GdkEvent (namely gdk_event_get,
gdk_event_peek, gdk_event_get_graphics_expose) have their own
copyingness specified and are therefore unaffected by this change.


[-- Attachment #2: gdk-1.2.defs.gdk-event-copy.diff --]
[-- Type: text/plain, Size: 714 bytes --]

--- gdk-1.2.defs.~1.32.~	2003-06-15 09:28:33.000000000 +1000
+++ gdk-1.2.defs	2003-06-15 15:33:29.000000000 +1000
@@ -407,7 +407,16 @@
 (define-type-alias GdkBitmap GdkWindow)
 (define-type-alias GdkDrawable GdkWindow)
 
+;; A GdkEvent passed to a signal handler is freed when the handler returns.
+;; The copy option here ensures the boxed object we create from it has an
+;; unlimited lifespan.
+;;
+;; For reference, the Gdk internal gdk_event_dispatch, which is a
+;; GSourceFunc in the main loop, is where the freeing happens.  An event is
+;; dequeued, the handler function called, and the event freed.
+;;
 (define-boxed GdkEvent
+  (copy gdk_event_copy)
   (free gdk_event_free))
 
 (define-boxed GdkColor

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2003-06-18 23:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-05-09  6:33 Fixed memory leaks in gdk-1.2.defs Marko Rauhamaa
2003-05-12  1:05 ` Kevin Ryde
2003-05-14  1:18   ` Marko Rauhamaa
2003-05-12  1:08 ` Kevin Ryde
2003-05-12  7:55   ` Marko Rauhamaa
2003-05-15 23:03     ` Kevin Ryde
2003-05-16  1:02       ` Marko Rauhamaa
2003-05-17  8:08         ` Marko Rauhamaa
2003-06-18 23:31 ` GdkEvent copy under signal handler (was: Fixed memory leaks in gdk-1.2.defs) Kevin Ryde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).