[Kde-bindings] Problem condition in Qyoto MethodCall

[Richard Dale]

On Saturday 10 February 2007, Arno Rehn wrote:
> Am Samstag, 10. Februar 2007 schrieb Richard Dale:
> > On Saturday 10 February 2007, Arno Rehn wrote:
> > > Am Samstag, 10. Februar 2007 schrieb Richard Dale:
> > > > On Saturday 10 February 2007, Arno Rehn wrote:
> > > > > Am Samstag, 10. Februar 2007 schrieb Arno Rehn:
> > > > > > Am Samstag, 10. Februar 2007 schrieb Richard Dale:
> > > > > > > This is wrong, but I still haven't worked out what the correct
> > > > > > > version should be:
> > > > > > >
> > > > > > > // We have to check here, if our target does still exists.
> > > > > > > // If there is no entry in the weakRef Dictionary, the instance
> > > > > > > doesn't exist anymore.
> > > > > > > // There's also no entry, if the method is a constructor or the
> > > > > > > method is static.
> > > > > > > // If the target doesn't exist anymore, set _called to true so
> > > > > > > the method won't be invoked.
> > > > > > > // The other possibility is that the qApp was just destroyed
> > > > > > > and we want to call a destructor.
> > > > > > > // This could lead to a crash when we interfere with the
> > > > > > > destroying mechanism of Q(Core)Application.
> > > > > > > if ( ((getPointerObject(_current_object) == 0) && !_ctor &&
> > > > > > > !(_tmp.flags & Smoke::mf_static))
> > > > > > >
> > > > > > >         || ((_tmp.flags & Smoke::mf_dtor) && (qApp == 0)) )
> > > > > > >
> > > > > > >         _called = true;
> > > > > > >
> > > > > > > Not every instance has an entry in the weak reference map. For
> > > > > > > example, I had a QVariant which was created in C++ code and it
> > > > > > > failed the condition and '_called' was set to true. Try the
> > > > > > > qdbus/pingpong example - it crashes when trying to access to
> > > > > > > QVariant returned.
> > > > > >
> > > > > > Oops, didn't know that not every instance is in the weakRef map.
> > > > > >
> > > > > > > We should only be looking at the 'o' and 'o->ptr' values and
> > > > > > > not call the method if they are zero and it's not a static
> > > > > > > method or constructor. I don't think we need to test for
> > > > > > > destructors at all because 'o->ptr' is set to 0 once something
> > > > > > > has been deleted (or it should be anyway).
> > > > > >
> > > > > > Uhm, actually if we just test for o->ptr == 0 and the method not
> > > > > > being a constructor it doesn't work. My code:
> > > > > > 		if ((_current_object == 0) && !_ctor)
> > > > > > 			_called = true;
> > > > > > Result:
> > > > > > *** glibc detected *** mono: free(): invalid pointer: 0xb6b94c9c
> > > > > > *** at startup.
> > > > > > As the problem with destructors just happens when the qApp is
> > > > > > about to quit I think we shouldn't test for something being null
> > > > > > at all, just if it is a destructor and if the qApp is about to
> > > > > > quit. If so, don't call the destructor. If it still crashes
> > > > > > somewhen else because the underlying C++ instance doesn't exist
> > > > > > anymore, the problem will probably have another cause.
> > > > >
> > > > > Ok, that won't work, I didn't think about that it is possible to
> > > > > not have called QCoreApplication::exec and still can do something
> > > > > with Qt. But just checking for o->ptr being 0 won't work either, as
> > > > > I've already stated.
> > > >
> > > > Ah, ok. This is the thing I love about systems programming - you can
> > > > spend about two weeks thinking about the correct version of a single
> > > > line of code. I think o->ptr is only set to zero for something which
> > > > was created in the C# side, but for an instance created in the C++
> > > > side I think we need to not call the delete method when the
> > > > destructor is called, and we do need to set o->ptr to zero.
> > >
> > > Ok, but how do we check whether it was created in C++ or in C#? Should
> > > we add another field to the smokeqyoto_object struct? What purpose does
> > > the "allocated" field in that structure have?
> >
> > It indicates whether the instance was constructed in the C++ world, or
> > in the C# world, when 'allocated' is true it means the instance was
> > created via a Qyoto constructor. We don't want to call the destructor
> > on any instances which have been constructed in the C++ world. Also we
> > want to call destructors for instances constructed in the C# world
> > only once, and so we must set the o->ptr to 0 once an instance has
> > been deleted.
> >
> > The reason that we don't want to keep C++ instance to C# instance
> > mappings for instances in the weak reference pointer map that were
> > allocated in the C++ world, is that C++ can delete an instance and
> > reallocate another one with the same address without the Qyoto runtime
> > knowing anything about it. So we could get a QEvent in an event
> > handler that was allocated by the Qt runtime in C++ with address
> > 0xfoobar, keep a mapping to the corresponding C# QEvent instance with
> > address 0xfoobar. But in the C++ world the QEvent could be deleted,
> > and a QBrush allocated on the heap with the same address as the former
> > QEvent. Subsequently, when we call a method in C# that returns a
> > QBrush with a C++ address 0xfoobar, we look it up in the pointerMap,
> > and still think it is a QEvent and return it as a C# QEvent instance.
>
> Ok, I have tried it out. That's my code so far (only the relevant part):
>
> // the instance doesn't exist anymore, the method is not static and it
> isn't a // constructor -> skip call
> if (!_current_object && !(_tmp.flags & Smoke::mf_static) && !_ctor)
> 	_called = true;
> // it is a destructor but the instance wasn't created in C# -> skip call
> if (_dtor && !o->allocated)
> 	_called = true;
> if (_dtor) {
> 	o->ptr = 0;
> }
I've just changed it to this:

		if (_target != 0) {
	    	_o = value_obj_info(_target);
			if (_o != 0 && _o->ptr != 0) {
				if (!_o->allocated && isDestructor()) {
					_called = true;
				}
			} else if (!isConstructor() && !isStatic()) {
				_called = true;
			}
		}

Then, once the method has been called:

		// A constructor
		if (isConstructor()) {
			_o = alloc_smokeqyoto_object(true, _smoke, method().classId, 
_stack[0].s_voidp);
			(*SetSmokeObject)(_target, _o);
		    mapPointer(_target, _o, _o->classId, 0);
			
			// create a signal spy to catch the "aboutToQuit()" signal
			if (strcmp("QApplication", _smoke->className(method().classId)) == 0
				|| strcmp("QCoreApplication", _smoke->className(method().classId)) == 0) {
				
				qapp_spy = new QSignalSpy(qApp, SIGNAL(aboutToQuit()));
			}
		} else if (isDestructor()) {
			_o->ptr = 0;
			_o->allocated = false;
		}



> That works nicely and well with t1 and the pingpong example, no problems
> here. But with t14 the whole thing still crashes when quitting the app.
> After some debugging code I found out that a certain QLabel-destructor is
> called two times, the pointer is exactly the same. Only difference:
> 'allocated' is one time 'false', the other time it is 'true'. Output from
> my debugging:
>
> class name: QLabel, method name: ~QLabel, allocated: 0, _current_object:
> 140796736
> class name: QLabel, method name: ~QLabel, allocated: 1, _current_object:
> 140796736
>
> Normally _current_object should be 0 in the second call, but it isn't. And
> I have no clue why allocated is false first time, and true second time. Any
> ideas?
I don't know if my latest changes have fixed this, but it seems strange to go 
from 'allocated == false' to 'allocated == true' rather than the other way 
round.

-- Richard