818629 - widget/gtkxtbin/gtk2xtbin.c:140:3: warning: initialization from incompatible pointer type ... (near initialization for ‘xt_event_funcs.finalize’)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning:
{
widget/gtkxtbin/gtk2xtbin.c:140:3: warning: initialization from incompatible pointer type [enabled by default]

widget/gtkxtbin/gtk2xtbin.c:140:3: warning: (near initialization for ‘xt_event_funcs.finalize’) [enabled by default]
}

This is for this chunk of code:
> 136 static GSourceFuncs xt_event_funcs = {
> 137   xt_event_prepare,
> 138   xt_event_check,
> 139   xt_event_dispatch,
> 140   g_free,
> 141   (GSourceFunc)NULL,
> 142   (GSourceDummyMarshal)NULL
> 143 };
https://mxr.mozilla.org/mozilla-central/source/widget/gtkxtbin/gtk2xtbin.c#136

This code dates back to bug 121615 (its patch: attachment 97117 [details] [diff] [review])

A quick google search shows that GSourceFuncs is declared with its 4th function-ptr needing to have this signature:
>  void     (*finalize) (GSource    *source);
http://developer.gnome.org/glib/2.31/glib-The-Main-Event-Loop.html#GSourceFuncs

...whereas we're passing g_free, which has this signature:
> void g_free(gpointer mem);
http://developer.gnome.org/glib/2.31/glib-Memory-Allocation.html#g-free

(Note that gpointer is simply a typedef for void*.)


So: GSourceFuncs expects a finalize function that takes a GSource*, but we're giving it a function that takes a void*.  So we're passing the wrong type of function.

We can fix this by just adding a simple wrapper-function that accepts a GSource* and g_free()s it. (and then passing that function-pointer, in place of g_free, in the code quoted above)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 2

[Daniel Holbert [:dholbert]]

(Side note: It looks like the 2 NULL lines (at lines 141 and 142 in the code quoted above) don't map to anything in the gtk 2.3.1 GSourceFuncs struct-definition that I linked in comment 0... but that doesn't seem to be causing any harm, and I'm guessing it might be there for backwards-compat with an older GTK version)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> (Side note: It looks like the 2 NULL lines [...]

(Ah yes, those are included here:
http://developer.gnome.org/glib/2.28/glib-The-Main-Event-Loop.html#GSourceFuncs
...so, never mind about those)

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 688892 [details] [diff] [review]
fix

The compiler warning is actually picking up a bug here.

The intention of the finalize method is to allow a derived object to release its resources. e.g.
http://git.gnome.org/browse/glib/tree/glib/giounix.c?h=glib-2-34#n172

The memory allocated by g_source_new will be released when the last reference is removed.

The g_free here would currently result in a double free, but that is not happening because the GSource is never unref'ed.  That can be done immediately after attach, as at http://hg.mozilla.org/mozilla-central/annotate/277998cf11cd/widget/gtk2/nsAppShell.cpp#l114

Here there are no resources to release, and so finalize can be NULL.
(GDK doesn't bother to cast the NULL - don't know whether our compiler flags would produce a warning with plain NULL.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Ah, thanks for noticing that!

Changed the patch to pass NULL and call unref after attaching.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 688968 [details] [diff] [review]
fix v2

Thanks for reporting and fixing!

Comment 7

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a16704573ef7

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a16704573ef7