[no subject]

Bug reports to:  http://bugzilla.gnome.org, product glib

Here are some bugs I found in glib-2.4.0 concerning the Main Loop
and file: glib-2.4.0/glib/gmain.c
Please examine and correct.

1----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Function: g_source_unref_internal()

The reference counting scheme :
===============================
New structures are created with a reference count=1.
The unref() functions decrease ref_count by 1. If the count drops to 0,
the finalization code is run and the memory is freed.
The destroy() function is invoked at this point, if the data structure is
not in a destroyed state.

For sources, the reference counting scheme seems buggy to me, in both the
unref() and destroy() functions:

- In 'g_source_unref_internal()', the test '{..g_warning()..}' does not
prevent a source marked NOT DESTROYED and attached to a context,
to be finalized and then freed.  Must be tested BEFORE deferencing.

- The call to 'g_source_unref_internal()' where the source is removed from
the context's linked list, should be moved to 'g_source_destroy_internal()'.


It should look like :

static void g_source_unref_internal (GSource * source, gboolean have_lock)
{
    GMainContext *context = source->context;

    /* First, the test */
    if (context && source->ref_count == 2 && !SOURCE_DESTROYED (source)) {
        g_warning (G_STRLOC
           ": Can't unref, the source is attached only to its context!");
        return;
    }

    if (!have_lock && context)
        LOCK_CONTEXT (context);

    source->ref_count--;

    if (source->ref_count <= 0) {

        /* Unref the corresponding callback object */
        if (source->callback_funcs)
            source->callback_funcs->unref (source->callback_data);

        /* Call source's finalize function */
        if (source->source_funcs->finalize)
            source->source_funcs->finalize (source);

        /* Frees the memory */
        g_slist_free (source->poll_fds);
        source->poll_fds = NULL;
        g_free (source);
    }

    if (!have_lock && context)
        UNLOCK_CONTEXT (context);
}

/**
 * g_source_unref:
 * @source: a #GSource
 *
 * Decreases the reference count of a source by one. If the
 * resulting reference count is zero the source and associated
 * memory will be destroyed.
 **/
void g_source_unref (GSource * source)
{
    g_return_if_fail (source != NULL);

    g_source_unref_internal (source, FALSE);
}

2----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Function: g_source_destroy_internal()

The destroy function marks source as destroyed and render it as 'unusable',
but as long there are references to it, it's allocated memory will not
be freed.

Here, the code unreferencing callback data is useless because done by
'g_source_unref_internal (source, context, TRUE);'.

It should look like:

static void g_source_destroy_internal (GSource * source, gboolean have_lock)
{
    GMainContext *context = source->context;

    /* Marks source as destroyed */
    source->flags &= ~G_HOOK_FLAG_ACTIVE;

    if (!have_lock)
        LOCK_CONTEXT (context);

    if (context) {  /* The source is attached to a context */

        GSList *tmp_list = source->poll_fds;
        while (tmp_list) {
            g_main_context_remove_poll_unlocked (context, tmp_list->data);
            tmp_list = tmp_list->next;
        }

        /* Removes source from the linked list of context */
        g_source_list_remove (source, context);

        /* Detaches explicitely source from context */
        source->context = NULL;

     /* Decreases ref counter and frees memory if needed */
     g_source_unref_internal (source, context, TRUE);
    }

    if (!have_lock)
        UNLOCK_CONTEXT (context);
}

void g_source_destroy (GSource * source)
{
    g_return_if_fail (source != NULL);

    g_source_destroy_internal (source, FALSE);
}

3----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Function: g_source_set_callback_indirect (),

When setting a new callback objet to a source, referencing the new object
must be done.

void
g_source_set_callback_indirect (GSource * source,
                                gpointer callback_data,
                                GSourceCallbackFuncs * callback_funcs)
{
    GMainContext *context;

    g_return_if_fail (source != NULL);
    g_return_if_fail (callback_funcs != NULL || callback_data == NULL);

    context = source->context;

    if (context)
        LOCK_CONTEXT (context);

    /* Unref the old object */
    if (source->callback_funcs)
        source->callback_funcs->unref (source->callback_data);

    source->callback_data = callback_data;
    source->callback_funcs = callback_funcs;

==> /* Ref the new object */
==> if (callback_funcs)
==>     callback_funcs->ref (callback_data);

    if (context)
        UNLOCK_CONTEXT (context);
}

4----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Functions: g_source_callback_ref (),
           g_source_callback_unref ()
           g_source_callback_get ()

It is better testing if the pointer to the callback object is not
NULL before any operation on it.

static void g_source_callback_ref (gpointer cb_data)
{
==> if (cb_data) {
        GSourceCallback *callback = cb_data;

        callback->ref_count++;
    }
}


static void g_source_callback_unref (gpointer cb_data)
{
==> if (cb_data) {
        GSourceCallback *callback = cb_data;

        callback->ref_count--;
        if (callback->ref_count == 0) {
            if (callback->notify)
                callback->notify (callback->data);
            g_free (callback);
        }
    }
}

static void g_source_callback_get (gpointer cb_data, GSource * source,
                                   GSourceFunc * func, gpointer * data)
{
==> if (cb_data) {
        GSourceCallback *callback = cb_data;

        *func = callback->func;
        *data = callback->data;
    }
}

5----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Function: g_source_set_callback (),

That function does a call to 'g_source_callback_indirect()' with the
parameter '&g_source_callback_funcs', preventing from using other
callback functions than the predefined 'g_source_callback_ref',
 'g_source_callback_unref', and 'g_source_callback_get'.

Think it would be better to separate in 2 functions:
- a callback object creation function
- the linkage of a source with a callback_object, callback_funcs.

    GSourceCallback g_callback_new (GSourceFunc func,
                                    gpointer data,
                                    GDestroyNotify notify)

    void g_source_set_callback (GSource * source,
                                GSourceCallback * callback_object,
                                GSourceCallbackFuncs * callback_funcs)

6----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Function: g_main_context_find_source_by_id (),

If the source ID is NOT in the context list, the function badly returns
the last element.  Should be :

GSource *g_main_context_find_source_by_id (GMainContext * context,
                                           guint source_id)
{
    GSource *source;

    g_return_val_if_fail (source_id > 0, NULL);

    if (context == NULL)
        context = g_main_context_default ();

    LOCK_CONTEXT (context);
    source = context->source_list;
    while (source) {
        if (!SOURCE_DESTROYED (source) && source->source_id == source_id) {
==>         UNLOCK_CONTEXT (context);
==>         return (source);
        }
        source = source->next;
    }
    UNLOCK_CONTEXT (context);
==> return (NULL);
}

7----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Function: g_main_context_prepare ()

The timeout value (source_timeout) returned by prepare() is erased by
line 2080: for each source G_SOURCE_READY, context->timeout is zeroed.
Then   MIN (context->timeout, source_timeout)   always gives 0; and
the poll() returns immediately.

gboolean g_main_context_prepare (GMainContext * context, gint * priority)
{
    ..........................
    context->timeout = -1;

    source = next_valid_source (context, NULL);
    while (source) {
        ..........................
        gint source_timeout = -1;
        ..........................
        if (!(source->flags & G_SOURCE_READY)) {
            gboolean (*prepare) (GSource * source, gint * timeout);

            prepare = source->source_funcs->prepare;
==>         if (prepare) {
                context->in_check_or_prepare++;
                UNLOCK_CONTEXT (context);

==>             if ((*prepare) (source, &source_timeout)) {
                    source->flags |= G_SOURCE_READY;
                }

                LOCK_CONTEXT (context);
                context->in_check_or_prepare--;
            }
        }

        if (source->flags & G_SOURCE_READY) {
            n_ready++;
            current_priority = source->priority;
/* Line 2080 conflicts with MIN () */
==>         context->timeout = 0;
        }

        if (source_timeout >= 0) {
            if (context->timeout < 0)
                context->timeout = source_timeout;
            else
/* MIN always gives 0 */
                context->timeout = MIN (context->timeout, source_timeout);
        }

      next:
        source = next_valid_source (context, source);
    }
    ..........................
}

Think it should be :
        ..........................
        if (source->flags & G_SOURCE_READY) {
            n_ready++;
            current_priority = source->priority;

/* Determins context->timeout ONLY for ready sources */
            if (source_timeout >= 0) {
                if (context->timeout < 0)
                    context->timeout = source_timeout;
                else
                    context->timeout = MIN (context->timeout,
source_timeout);
            }
        }

      next:
        source = next_valid_source (context, source);
    }

8----------------------------------------------------------------------
File: glib-2.4.0/glib/gmain.c
Function: g_main_context_poll()

The parameter 'priority' is unused.

static void
g_main_context_poll (GMainContext * context, gint timeout, gint priority,
                     GPollFD * fds, gint n_fds)

-----------------------------------------------------------------------
- Philippe