531460 - assert when classes not meant to be are used as temporaries

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: add the macros for the underlying mechanism

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on 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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: use the macros in xpcom

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: use the macros in xpconnect

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: use the macros in content

Comment 9

[[e-mail]]

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. :(

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Benjamin Smedberg]

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

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 16

[Andrew McCreight [:mccr8] (PTO until 12-2)]

A bunch of patches landed here in 2010, so I think it makes sense to close the bug as is.