15433 - MOZ_TRACE_XPCOM_REFCNT is lethal now

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P2)

Description

[Daniel van der Zee]

When MOZ_TRACE_XPCOM_REFCNT is defined, nsISupportsUtil.h defines
NS_ADDREF as:

 #define NS_ADDREF(_ptr)                                       \
  ((nsrefcnt) nsTraceRefcnt::AddRef((_ptr), (_ptr)->AddRef(), \
                                    __FILE__, __LINE__))

In combination with code like this:

  NS_ADDREF(nsMenuFrame::mDismissalListener = new nsMenuDismissalListener());

(from layout/xul/base/src/nsMenuPopupFrame.cpp)

This is pretty lethal in VC++ on Windows because the statement will get executed
twice. VC++ evaluates the second argument to nsTraceRefcnt::AddRef first so the
actual object that gets assigned to nsMenuFrame::mDismissalListener has a
refcount of 0 and the object with a refcount of 1 will be leaked.

In other compilers it will do the same or just leak an object.

Result: Apprunner won't start

Maybe this should be fixed (enclose in {} and assign to temp first) or removed?

Comment 1

[Daniel van der Zee]

O, never mind the solution in the last line.

Comment 2

[Suresh Duddi (gone)]

Thanks. Good catch.

Comment 3

[Suresh Duddi (gone)]

Fixed this one code that you pointed. Still need to work on fixing TRACE_REFCNT
to not evaluate twice...

Comment 4

[Daniel van der Zee]

Here's a fix for the macro, but it uses templates. Maybe since this is an
optional feature, and there is no other way around it (I think) it's useable?

This might interfere with the stack tracing when the code is not inlined in
debug builds.

#include <stdio.h>

class tstClass {
	int	mRefCnt;
public:
	int AddRef(void) { printf("AddRef\n"); return ++mRefCnt; }
	int Release(void) { printf("Delete\n");  return --mRefCnt; }

	tstClass() { printf("ctor\n"); }
};


template <class X>
	inline  X * ADDREF(X *x)
	{
		x->AddRef();
		printf("addref logging goes here\n");
		return x;
	}

int main(int argc, char* argv[])
{
	tstClass *a = NULL;
	ADDREF(a=new tstClass());

	return 0;
}

Comment 5

[Suresh Duddi (gone)]

Warren since you are modifying these defines anyway, can you take care of this.

Comment 6

[Warren Harris]

We don't need templates for this. Just do the (_ptr)->AddRef() inside of the
nsTraceRefcnt::AddRef call. I'll fix it.

Comment 7

[Warren Harris]

Ok, I investigated fixing this, and it's not as easy as I thought. The problem
is that if you try to perform the AddRef/Release down inside of the
nsTraceRefcnt routine, you have to pass an nsISupports to do this. But many
classes use multiple interface inheritance which makes it impossible to
automatically cast to nsISupports whenever NS_ADDREF or NS_RELEASE are used. So
the user has to put in explicit casts (but only for NS_LOSING_ARCHITECTURE).

Since this is only for platforms that don't support stack walking
(NS_LOSING_ARCHITECTURE), and it seems improbable that the expression being
evaluated in the context of the NS_ADDREF or NS_RELEASE macros will have side
effects (which would be fatal to evaluate twice), I'm declaring this WONTFIX.

Comment 8

[Jonas Nagel]

Owner/reporter please verify if possible...