I added assertions of this form to the JS engine in bug 518633. This bug is for the rest of the tree. It's a relatively common programming error with RAII classes to forget to name the variable, in other words, to write: JSAutoTempValueRooter(cx, v); instead of: JSAutoTempValueRooter tvr(cx, v); which has entirely the wrong scope. In bug 518633, I figured out a technique to make such uses assert; it's a little ugly (but not too horrible, I think) at the implementation side, but requires no changes to the users and has no cost in non-DEBUG builds. I propose adding this to AutoRestore.h (added in bug 518756) and using these macros in the tree. (These classes should also all be annotated with the static analysis from bug 502775 once that static analysis lands.)

This has two additional macros in addition to the JS version (might want to port them back): * MOZILLA_GUARD_OBJECT_NOTIFIER_ONLY_PARAM, for RAII classes whose constructors currently take no parameters * MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL, for RAII classes whose constructors are not inline I didn't run into those cases in the JS version. I might hold off on the adding-uses patch until after bug 502775 lands, so then I can do this and annotate with NS_STACK_CLASS and NS_NONTEMPORARY_CLASS all at once. But I have work-in-progress in my tree: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/2257bfc9a7f0/guard-object-notifier-use

(And I know there are a bunch of additional classes in gfx/thebes/ that should be annotated, and probably many more.)

Comment on attachment 414879 [details] [diff] [review] patch 1: add the macros for the underlying mechanism Please add an in-code link to an MDC page giving examples for using these macros, since, even though I understand how they work, it would be a lot easier for me if there was a simple example of how to use them.

I've started the documentation at https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla

Mechanism patch landed: http://hg.mozilla.org/mozilla-central/rev/736c9ca89e99 I still need to attach a series of patches to add users of the macros.

Do you think the analysis in bug 502775 can obsolete the guard stuff here, or do you think it should complement it? If the former, I'll get it cleaned up and land next week. It's not a priority atm. :(

(In reply to comment #9) > Do you think the analysis in bug 502775 can obsolete the guard stuff here, or > do you think it should complement it? I think complement. Assertions are good for when the bug caused is easily noticeable and the developer wouldn't otherwise find the cause (and, e.g., might spend a while debugging the problem had the assert not immediately pointed out the problem); static analysis is good for the less noticeable variants.

Comment on attachment 435164 [details] [diff] [review] use the macros in xpcom I somewhat prefer a static analysis for this, but there's definitely value in having both types of checks.

Additional things to follow up on: * want to make sure that NS_STACK_CLASS applies both to inheritance and being a subobject... and then audit places I've added it here to remove unnecessary ones * follow up and add NS_NONTEMPORARY_CLASS when it exists... subject to the same constraints

Landed the above 3 patches: http://hg.mozilla.org/mozilla-central/rev/c6dcd6ea54af http://hg.mozilla.org/mozilla-central/rev/7ced71a5f405 http://hg.mozilla.org/mozilla-central/rev/3595abe885eb Still more to do here.

stack class definitely checks for both inheritance and member objects, and we even have tests for it!

Bastiaan Jacques just sent me email with what appears to be an even better idea: a way to prevent this pattern from compiling in the first place. In particular, given: class nsAutoLock { public: nsAutoLock(PRLock *aLock); }; the trick is to: #define nsAutoLock(lock) /* something that doesn't compile goes here */ (For the something-that-doesn't-compile, he suggested a template function containing a static assert, but I suspect something simpler would do just fine.) I haven't tested it yet, though.