(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
The hazard was complaining about: "unrooted '<returnvalue>' of type 'JSObject*' live across GC call at dom/base/Element.cpp:588".
But as far as I can tell, the Rooted destructor should run after the RefPtr destructor, so it should be safe, shouldn't it?
Nope, though it totally looks like that should work and we all start out assuming that it does.
The code used to have an extra scope which I removed, and then reintroduced in comment 3 to hopefully please the analysis, but it seems like a bug to me...
The analysis is correct, and I have observed actual crashes from this.
What happens is that
return ret extracts a JSObject* from the Rooted<> and places it on the stack (or puts it in a register). Then ~RefPtr runs a GC. The GC updates the pointer in the Rooted<>, but that's unused at this point so doesn't matter. It does not update the return value that is now sitting in a register or on the stack. Finally, ~Rooted runs and unlinks itself from the list of things to update, but that memory location is irrelevant at this point anyway. Control returns to the caller, and it sees a stale pointer as the return value.
This can be quite a nuisance when using RAII objects whose destructors can GC. We just happen to not use many of those within Spidermonkey, and until recently a bug in the analysis prevented it from noticing that those destructors could GC. Also, most of them can't -- for the most part, they'll only GC if the refcount reaches zero, and most of the time that's not going to happen. (Since usually you're assigning into a RefPtr or nsCOMPtr from something that already has a nonzero refcount, then the destructor will just drop it back to that refcount. Unless some code executes within the RAII scope that removes the original storage location.)
Depending on how much of a nuisance this turns out to be in practice, I may need to come up with more ergonomic ways of handling it. I suppose we could use Maybe<RefPtr<T>> and then reset() it explicitly before returning a value (I think the analysis is smart enough to not complain if you return a nullptr in error cases, so you'd only need it in the successful return path.) But that's anything but ergonomic.
When it works, switching to a MutableHandle<> outparam is a good solution (changing the return type to void, bool, or nsresult). But that's not always convenient, especially for heavily-used or idl-generated method signatures.