The problem happens in gi/pygi-marshal-from-py.c _pygi_marshal_from_py_array(), here:

                    } else if (!is_boxed || is_gvalue) {
                        memcpy (array_->data + (i * item_size), item.v_pointer, item_size);
                        if (from_py_cleanup)
                            from_py_cleanup (state, item_arg_cache, item.v_pointer, TRUE);

In the test case, the second argument is "42", and after the memcpy the value is correct:

(gdb) p (char*) ((GValue*) array_->data)[1]->data->v_pointer
$6 = 0xc07c50 "42"

but the from_py_cleanup kills it with g_value_unset ((GValue *) data), so after that call we have

(gdb) p (char*) ((GValue*) array_->data)[1]->data->v_pointer
$8 = 0xc07c50 ""

So the bug is that the memcpy() copies v_pointer by reference instead of by value, or we should not actually remove the GValue.

I have a patch for properly copying the GValues, and that gets me a little further. Now it crashes in:

+ Trace 229879

This code in _pygi_marshal_cleanup_from_py_array() is broken:

                cleanup_func (state,
                              sequence_cache->item_cache,
                              (array_ != NULL) ? g_array_index (array_, gpointer, i) : g_ptr_array_index (ptr_array_, i),
                              TRUE);

The array_ is an array of structs (GValue), not an array of struct pointers (GValue*), so it tries to interpret the first couple of bytes of the struct (the GType) as a pointer, which results in data=0x18 in pygi_marshal_cleanup_from_py_interface_struct_gvalue(). I need to check whether this is a special case for GValues, but judging how they are created in _pygi_marshal_from_py_array() I think not. I rather think that all the other cases we had so far were arrays of simple types (int etc.) which do not have a cleanup function, or arrays of actual pointers, which go through the g_ptr_array_index code path. I'll verify this.

Created attachment 209788 [details] [review]
Fix Python to C marshalling of GValue arrays

This fixes both problems. The two new test cases, as well as all existing ones run fine now. I believe after staring at and debugging this code for some two hours I now understand what it does, so please don't hesitate to ask me questions or challenge assumptions during patch review. I'm still by far not an expert in the intricacies of all that marshalling.

Thanks in advance for a review!

Note, the real hard-core test for this is in bug 671610 when using Gtk.ListStore.insert_with_valuesv() instead of the per-item set_value() methods in the overrides. With this crash, my preliminary patch for that bug already works a whole lot better, but it still causes a crash. This might or might not be a problem with the patch here, but it's getting too late now, I'll continue this tomorrow.

So the patch is still fine for review, and I believe it's reasonably correct, but I'd hold it back for a while until I get bug 671610 working as well.

Review of attachment 209788 [details] [review]:

Purely stylistic review, I need more attention for a real review...

::: gi/pygi-marshal-cleanup.c
@@ -337,3 +337,3 @@
                 sequence_cache->item_cache->from_py_cleanup;
 
             for(i = 0; i < len; i++) {

fix space in "for (" whoile touching this code

@@ -339,1 +339,5 @@
             for(i = 0; i < len; i++) {
+                gpointer item;
+
+                if (array_ != NULL)
+                    item = sequence_cache->item_cache->is_pointer 

this looks unreadable enough to warrant a real "if...else"

@@ -340,1 +349,1 @@
                 cleanup_func (state,

now that you shortened it, this can go all on a single line

::: gi/pygi-marshal-from-py.c
@@ +840,3 @@
                         g_assert (item_size == sizeof (item.v_pointer));
                         g_array_insert_val (array_, i, item.v_pointer);
                     } else if (!is_boxed || is_gvalue) {

this nested if looks not very readable: if these two cases share so little we may be better off with

else if (is_gvalue)
  ...
else if (!is_boxed)
  ...

Created attachment 209803 [details] [review]
Fix Python to C marshalling of GValue arrays

Thanks Paolo. This updated patch addresses your review comments and also fixes the crash I mentioned before -- it happened when having None entries in the passed array, which is now checked explicitly. This is not explicitly covered by the test case in this attached patch, but well covered in the test_{list,tree}_store test cases (see bug 671610). If you prefer testing this case explicitly here, we need to add a new method to the GIMarshallingTests in gobject-introspection.

Review of attachment 209803 [details] [review]:

Looks great to me.

Thanks for the review! Pushed.

For the record, this uncovered another bug which results in random crashes. Reported as bug 672224