Using Dehydra to Detect Problematic Code
In XPCOMGC, the behavior of nsCOMPtr is very different than currently:
nsCOMPtrshould only be used as a class member, never on the stack. Taras is working on a rewriting script that will replacensCOMPtr<nsIFoo>on the stack with a rawnsIFoo*(more on that later).- the purpose of
nsCOMPtris not to ensure correct reference counting (there is no reference-counting!); instead it serves to enforce write barriers, so that MMgc can properly perform incremental GC.
I was able to rewrite nsCOMPtr so that existing code code mostly use the existing API: there is however one major difference: getter_AddRefs cannot return a pointer directly to the nsCOMPtr instance. Instead, it must save the value in a local variable and call nsCOMPtr.set() to preserve the write-barrier semantics. It does this using a temporary class:
/**
* nsGetterAddRefs is used for XPCOM out parameters that need to be assigned
* to nsCOMPtr members. We can't pass the address of nsCOMPtr.mRawPtr directly
* because of the need to set the write barrier.
*/
template <class T>
class nsGetterAddRefs
{
public:
explicit
nsGetterAddRefs(nsCOMPtr<T> &aSmartPtr) :
mTempPtr(aSmartPtr),
mTargetSmartPtr(aSmartPtr)
{
// nothing else to do
}
~nsGetterAddRefs()
{
mTargetSmartPtr = mTempPtr;
}
operator T**()
{
return &mTempPtr;
}
private:
T* mTempPtr;
nsCOMPtr<T> &mTargetSmartPtr;
};
template <class T>
inline
nsGetterAddRefs<T>
getter_AddRefs(nsCOMPtr<T> &aSmartPtr)
{
return nsGetterAddRefs<T>(aSmartPtr);
}
For the vast majority of cases where code makes a simple getter call, this works fine:
nsresult rv = something->GetAFoo(getter_AddRefs(mFoo));
However, if you test or use the returned value after you get it in the same statement, the value won’t be assigned yet:
Bad:
if (NS_SUCCEEDED(something->GetAFoo(getter_AddRefs(mFoo))) && mFoo)
Also bad:
NS_SUCCEEDED(something->GetAFoo(getter_AddRefs(mFoo))) && mFoo->DoSomething();
In the XPCOMGC world, both of these cases will fail because the ~nsGetterAddRefs destructor runs after the dereference of mFoo. Once we remove stack comptrs, this is not a common occurrence, but it does happen occasionally.
Checking for this kind of pattern is the perfect job for dehydra: check it out.
October 16th, 2007 at 6:02 pm
since nsCOMPtr is going to change so drastically, do you want to consider changing its name?
October 16th, 2007 at 9:52 pm
Doug: I brought up name polishing today. It’s worth doing, but later. You’re right that we should rename simply because the new beast is so different from the old. Then there are the ns and COM name-parts to hate. ;-)
Benjamin: don’t you wish C++ let you force temporary dtors to run by some super-parentheses? I do. You didn’t say how many offenders that dehydra script caught. If lots, would it be better (in the spirit of new names for new things; getter_AddRefs makes no sense in a GC system as a name) to simply rewrite away from getter_AddRefs altogether? IIRC an MMgc DWB member can have its address taken and be passed as an out param pointer. Do we really need all this if there’s a more direct MMgc method for putting out param address-of-members through the write barrier? We’d want our own XPCOMGC veneer on top, but it wouldn’t be getter_AddRefs and it would not have that form and fit.
/be
October 17th, 2007 at 1:19 am
[…] Firefox 4, we’re moving towards GC for all but the most performance-sensitive pieces of code, using high performance code donated to […]
October 17th, 2007 at 3:24 pm
Brendan, AFAICT from http://mxr-test.landfill.bugzilla.org/tamarin-central/source/MMgc/WriteBarrier.h you are not allowed to take the address of a DWB to pass to a getter. I don’t see how you could make it safe from incremental GC.
October 17th, 2007 at 11:27 pm
The alternative would be to modify both caller and callee: caller passes a WriteBarrier by explicit (&, address-of) reference, callee takes WriteBarrier*. Is that the long-term goal, with a layer of XPCOMGC naming to hide the MMgc names?
/be
October 17th, 2007 at 11:31 pm
Sorry, I whiffed that last comment completely. What I meant was caller passes WriteBarrier to a callee taking WriteBarrier& aResult, and assignments to aResult run the right operator=.
/be
October 18th, 2007 at 12:42 am
Or maybe I’m about to strike three ;-). High-order bit is to change callees as well as callers. More rewriting work, but it seems tool-able — is it?
Also (or again), do you know how many instances of the pattern your dehydra script finds here are in the tree? For Mozilla 2 we want to remove Mozilla and XPCOM idioms in favor of standard C++. This restriction makes for yet another idiomatic pattern, if-if instead of if(-&&-) or the like. Avoid if possible.
/be
October 18th, 2007 at 10:18 am
We can’t change the signature to take a WriteBarrier& because most callers are passing a stack param. I think, if we were going to rewrite this, we should remove the getter_AddRefs muck altogether and always pass a stack param:
– if (NS_SUCCEEDED(something->GetAFoo(getter_AddRefs(mFoo))) && mFoo)
+ nsIFoo* foo = NULL;
+ nsresult rv = something->GetAFoo(&foo);
+ mFoo = foo;
+ if (NS_SUCCEEDED(rv) && mFoo)
October 19th, 2007 at 3:45 am
Yeah, that’s better. I mentioned privately that this still forces us into overstating XPCOMGC in current MMgc terms: conservative stack scanning with incremental mark/sweep. And the natural C++ style of passing &mFoo is banned, but Taras points out that outparamdel awaits. I hope that really reduces the remaining new XPCOMGC idioms infrequent.
/be