Using Dehydra to Detect Problematic Code

In XPCOMGC, the behavior of nsCOMPtr is very different than currently:

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.

Atom Feed for Comments 9 Responses to “Using Dehydra to Detect Problematic Code”

  1. Doug Turner Says:

    since nsCOMPtr is going to change so drastically, do you want to consider changing its name?

  2. Brendan Eich Says:

    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

  3. Rob Sayre’s Mozilla Blog » Blog Archive » Keeping An Eye On Things Says:

    [...] Firefox 4, we’re moving towards GC for all but the most performance-sensitive pieces of code, using high performance code donated to [...]

  4. Benjamin Smedberg Says:

    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.

  5. Brendan Eich Says:

    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

  6. Brendan Eich Says:

    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

  7. Brendan Eich Says:

    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

  8. Benjamin Smedberg Says:

    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)

  9. Brendan Eich Says:

    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

Leave a Reply