[pywin32-bugs] [ pywin32-Bugs-1590399 ] Refcounting in ui_assoc_object::GetGoodRet

[SourceForge.net <no...@so...>]

Bugs item #1590399, was opened at 2006-11-04 11:12
Message generated for change (Comment added) made by kxroberto
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=551954&aid=1590399&group_id=78018

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: pythonwin
Group: None
Status: Open
Resolution: None
>Priority: 9
Private: No
Submitted By: kxroberto (kxroberto)
Assigned to: Nobody/Anonymous (nobody)
Summary: Refcounting in ui_assoc_object::GetGoodRet

Initial Comment:
I wonder if the below patch possibly addresses a
refcount/memory problem. Think its possible, that
DODECREF(this) frees <*this> from memory, another
released threads malloc uses the memory and
this->virtualInst is bogus?

( In preceding function  ui_assoc_object::DoKillAssoc
there is a simlar technique to do "PyObject *vi =
virtualInst;" - but for reverse reason )

( I'm still in search of memory leaks/crashes on dual
cores and fast CPUs when other threads are going on
parallel to win32ui GUI mainthread. )



--- win32assoc.cpp.orig	2005-04-12 17:01:00.000000000 +0200
+++ win32assoc.cpp	2006-11-04 10:59:46.326646400 +0100
@@ -252,9 +252,10 @@
 {
 	if (this==NULL) return NULL;
 	if (virtualInst) {
+		PyObject *vi = virtualInst;
+		DOINCREF(vi);
 		DODECREF(this);
-		DOINCREF(virtualInst);
-		return virtualInst;
+		return vi;
 	} else
 		return this;
 }


----------------------------------------------------------------------

>Comment By: kxroberto (kxroberto)
Date: 2006-11-04 16:49

Message:
Logged In: YES 
user_id=972995

Think I found other serious potential refcount/GIL problems
which cause crashes here in apps with threads - mainly on
dual core - and set priority to 9:

I looked over GUI_BGN_SAVE's
=> Py_XINC/DECREF's outside of the GIL. this causes probably
the special problems only on dual cores when multiple threads:


win32cmdui/Line120:
					GUI_BGN_SAVE;
					Python_delete_assoc(ob);
					GUI_END_SAVE;
win32uimodule/Line650:
void Python_delete_assoc( void *ob )
{
	// Notify Python object of my attached object removal.
	{
	CVirtualHelper helper ("OnAttachedObjectDeath", ob);
<================
	helper.call();                                      
<================
	}
	ui_assoc_object *pObj;
	if ((pObj=ui_assoc_object::GetPyObject(ob)) &&
!bInFatalShutdown) {
		CEnterLeavePython _celp; // KillAssoc requires it is held!
		pObj->KillAssoc();
	}
}

win32virt.cpp/Line22
CVirtualHelper::CVirtualHelper(const char *iname, const void
*iassoc, EnumVirtualErrorHandling veh/* = VEH_PRINT_ERROR */)
{
	handler=NULL;
	py_ob = NULL;
...
	py_ob = py_bob;
	Py_INCREF(py_ob);            <================
//	Py_XINCREF(handler);
}


The gross hack for speed in
CVirtualHelper::~CVirtualHelper() will not work for dual cores. 
A fast "ZDODECREF/ZDODECREF" independent of GIL (unless it
went to 0) would be possible on x86 with "__asm LOCK DEC
<(op)->ob_refcnt>" or so.
see:
http://groups.google.de/group/comp.lang.python/msg/ef530d4448814391


CVirtualHelper::~CVirtualHelper()
{
	// XXX - Gross hack for speed.  This is called for eachh
window message
	// so only grabs the Python lock if the objects need Python,
	if ((retVal && retVal->ob_refcnt==1) ||    
<================ DECREF not atomic on multi-core CPU's
		(handler && handler->ob_refcnt==1) || 
		(py_ob && py_ob->ob_refcnt==1)) {
		CEnterLeavePython _celp;
		XDODECREF(retVal);
		XDODECREF(handler);
		XDODECREF(py_ob);
	} else {
		XDODECREF(retVal);          <================ DECREF not
atomic on multi-core CPU's
		XDODECREF(handler);
		XDODECREF(py_ob);
	}
}


Maybe there are other loose INCREFS / DECREFS in win32 code?


And I found - probably most serious in this list - : 
Only CVirtualHelper::do_call  is CEnterLeavePython protected:

BOOL CVirtualHelper::do_call(PyObject *args)
{
	CEnterLeavePython _celp;
	XDODECREF(retVal);	// our old one.
	retVal = NULL;


but all entrace call functions are not - but they are
bulding up Python Values ! 
Think that is serious (even on single cores where I have
very rare crashes):
e.g.:

BOOL CVirtualHelper::call()
{
	if (!handler) return FALSE;
	PyObject *arglst = Py_BuildValue("()");      <================
	return do_call(arglst);
}


=>Probably the CEnterleave protection should be moved out of
do_call to all entrance call(...)'s & call_args()  ?



Looked in a rush over the 200 base DECREFs in Pythonwin.
Found so far no other smelling refcounting.
Only maybe this is invalid  :

--- win32app.cpp.orig   2006-01-10 15:38:00.000000000 +0100
+++ win32app.cpp        2006-11-04 16:44:24.009694400 +0100
@@ -412,6 +412,7 @@
 {
        PyCWinThread::cleanup();
        // total hack!
-       while (pExistingAppObject)
+       while (pExistingAppObject->ob_refcnt>1)
                DODECREF(pExistingAppObject); // this may
delete it.
+       DODECREF(pExistingAppObject);
 }



----------------------------------------------------------------------

Comment By: kxroberto (kxroberto)
Date: 2006-11-04 11:22

Message:
Logged In: YES 
user_id=972995

(repeated)

--- win32assoc.cpp.orig	2005-04-12 17:01:00.000000000 +0200
+++ win32assoc.cpp	2006-11-04 10:59:46.326646400 +0100
@@ -252,9 +252,10 @@
 {
 	if (this==NULL) return NULL;
 	if (virtualInst) {
+		PyObject *vi = virtualInst;
+		DOINCREF(vi);
 		DODECREF(this);
-		DOINCREF(virtualInst);
-		return virtualInst;
+		return vi;
 	} else
 		return this;
 }


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=551954&aid=1590399&group_id=78018