Created attachment 11796 [details]
test code

With all three comment lines enabled in the test case, here's a dump
of the seg fault I get (on the first connect() line to execute):

+ Trace 29306

The actual cause of the bug was a large PEBKAC, however pygtk failed 
to throw the exception that I would have expected to see. 

My gtk.Entry(*a,**kw) should have been gtk.Entry.__init__(self,*a,**kw).
Running connect() on an uninitialized gtk.Entry shouldn't cause a
segfault, though.

Considering marking this bug WONTFIX.  This would essentially involve
adding calls to every single wrapper in the binding, and doesn't
really have that much of a benefit.

Isn't it just another check in all functions, something like:

_wrap_foo_bar(self, args)
{
   if (!PYGTK_OBJECT_IS_INITALIZED(self))
       return NULL;
   
   if (!PyArg_ParseTuple .....

   .....

====
It can easily be done for all autogenerated functions and i can
volunteer to add it to all manually generated.

If it prevents a segfault it's good.
  

mass reassign of open pygtk and gnome-python bugs.

I guess i changed my mind over this one, it's not really worth it to
add checks everywhere. 
I'm suprised that it worked at all.

Marking as WONTFIX, since it's really to much work too fix it, with as
james said, to little benefit. Just use it in the correct way instead.

class Foo(gtk.Foo):
   def __init__(self):
      Foo.__init__(self)

  This really should be fixed. In my opinion, no python should ever
crash. For programming errors, exceptions were invented.
  I also don't see why this would be too much work. As Johan said,
checks can be done automatically by the code generator. For overridden
functions, a simple macro could be defined and placed in each function
at the beginning. Example:
(in pygobject.h)
#define pygtk_object_init_check() \
  if (!PYGTK_OBJECT_IS_INITALIZED(self)) { \
    PyErr_SetString(PyExc_RuntimeError, "The underlying GObject isn't
initialized"); \
    return NULL; \
}

  Then:

_wrap_foo_bar(self, args)
{
    pygtk_object_init_check();
     /* ... */

  Is this too much work?
  You know, it really gives a bad image to pygtk when it segfaults, no
matter whose fault it is...

Just for the record: I disagree on "too little benefit."  drewp's
comment, "pygtk failed to throw the exception that I would have
expected to see" says much.  Judging from timestamps, it took him half
an hour to track down this very simple mistake, and that's *after* he
boiled it down to a minimal test case.  Errors that cannot be reported
sanely (i.e. segfaults) must be avoided at all costs.  I've come to
expect nothing less from Python, and that's one of the primary factors
making it my development tool of choice.

This should really be fixed on the grounds that it's a bug any time Python 
segfaults.  Is this a case of not checking if a GObject pointer is NULL?

john: I consider the goal of making pygtk crash proof impossible (the simple
fact that it can call abort() should prove that).  There are other places where
I've had to reduce the safety in order to just make things work

For example, try holding onto a pointer to the requisition object in
size_request.  If you try to modify the requisition after the signal handler has
completed you may corrupt the stack.  Previously, pygtk handled this safely, but
that made it impossible to implement the size_request signal handler.

If it was possible to make pygtk crash proof, then it would be obvious what to
do.  Given that that is not the case, its also worth considering maintainability
and bloat.

Yes, pygtk can never be crash proof.  The core python interpreter is not crash
proof and will probably never be.  That doesn't mean that we shouldn't try to
eliminate crashes where we can and even to go to some lengths to do so.  For
example the support for the size request handler is implemented so that if the
object pointing to the statically allocated structure outlives the function
call, the GValue will be copied so that access to the Python object will not
corrupt the stack.

I'm not asking anyone to go and fix this, but rather I think it should be left
as an open issue.  The question in my previous comment was intended to find out
whether the fix is simply adding a NULL pointer check or if there's more involved.

Why cannot the base class every wrapper inheritated from check the
initialization first? Isn't all widget based on gobject? 

A fix for this would involve adding a check to every single method/function in
PyGTK (not just checking self->obj -- you'd also need to make sure the user
didn't pass one of these half initialised GObject wrappers to some other function).

This would be doable, if rather ugly, for generated code, but I agree it would
be a burden for handwritten code in the .override files.  On the other hand, the
Python core code does this level of checking in many cases.  

Could we use the tp_new slot to get around this (as was suggested on another
bug)?  The idea would be to have PyGObject's tp_new create the object using
g_object_new using the most derived subclass and passing in a list of
properties.  The tp_new slots for the descendant classes would then be
implemented as functions that took positional arguments and bound them to
property names.

For example (in python pseudo code):
  class Label(Widget):
    def __new__(cls, text = '', **kw):
      return Widget.__new__(text = text, **kw)

Descendants implemented in C could muck this up, but it's pretty hard (if not
impossible) to do from Python.  Could all the constructors be implemented this
way?  If there were a few exceptions, they could have tp_new slots which would
do something special if cls exactly matched a particular class.

This also allows arbitrary properties to be specified as arguments to the
constuctor, which I think is a feature worth adding.

I don't get it. If there are some way to force every gobject instance to be
initialized at the creation time, then there should be no chance that an
uninitialized object derived from GObject being passed around.

Maybe the question is can python do that? force a function (a Mixin one?) to be
called at the creation time. 

OK, if it is about unbounded method, then how python itself does unbounded
method checking?

Is this fixed by the constructing widgets through g_object_new() patch?  See
http://bugzilla.gnome.org/show_bug.cgi?id=96658  

I do think this should be reopened if it's not already fixed because anything
that cause a segfault in fairly typical usage and can be fixed should be listed
as a bug, even if there aren't plans to immediately fix it.

Reopening for evaluation.

Nope, g_object_new() patch doesn't force the user to call the parent
constructor, therefore we still get this segfault.
I'm no longer sure we should fix this.  I tend to agree with James.  One thing
is to fix constructors, but this would require checks in _all_ pygtk methods
(this isn't just a problem with connect(), it is global).  Adding these checks
all over pygtk will make it marginally slower and its code larger.

Unless we agree to have conditional compiling in pygtk and a --enable-debug
configure option, so that we could only activate these extra checks for
developer packages.

Created attachment 29656 [details] [review]
Preliminary patch to use __new__ instead of __init__

Here a preliminary patch to create the GObject in __new__ rather than __init__
which means that the obj pointer is not NULL until __init__ or __gobject_init__
is invoked.  This change will affect user code because GObject subclasses will
now need to override __new__ instead of __init__ if the constructor requires a
different set of arguments than the base class.  I thought existing __init__
methods that didn't pass arguments to __gobject_init__ could be supported, but
positional arguments pass to __init__ are also passed to __new__ and the
__new__ of the superclass will interpret them as arguments for itself.

If a class used the following:
  class A(gtk.Window):
    def __init__(self, title):
      self.__gobject_init__()
      self.title = title

it would need to be changed to:
  class A(gtk.Window):
    def __new__(cls, title):
      self = gtk.Window.__new__(cls)
      self.title = title

So, the upside of this is it reduces the risk of segfaults and may allow for
better support for construct only properties, but the downside is code that
subclasses a GObject will need to change.

The patch is incomplete, but pygtk-demo seems to work.

In principle I agree with the patch, but the timing is less than perfect.

We cannot really break the API, existing code needs to continue to work.

I committed a patch which adds checks for all GObject method and will raise an
exception if the object is not initialized.

*** Bug 338760 has been marked as a duplicate of this bug. ***

Created attachment 63705 [details] [review]
pygobject API to check if object is initialized

Created attachment 63706 [details] [review]
make pygtk codegen call pygobject_check_init for methods and getters

It would be nice to evaluate the impact of this change in terms of code size; I think it's small, but...

OK, here's a test, compiled both times with gcc 4.0.3 with CFLAGS=-Os on amd64

_gtk.so before: 2631166 bytes
_gtk.so after:  2685523 bytes (+2%)

Same experiment, with stripped library:
_gtk.so before: 2159848 bytes
_gtk.so after: 2214184 bytes (+2.5%)

I think that we should get this ball rolling.

It should probably go into PyGObject where the code generator should live no (the pygtk copy is not quite removed).

*** Bug 554064 has been marked as a duplicate of this bug. ***

codegen and the static bindings are obsolete since pygobject 3.0, which is GI only.

Thanks!