Closed
Bug 531460
Opened 15 years ago
Closed 4 years ago
assert when classes not meant to be are used as temporaries
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
Attachments
(4 files)
5.37 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
9.79 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
11.82 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
4.82 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•15 years ago
|
||
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
Attachment #414879 -
Flags: review?(benjamin)
Assignee | ||
Comment 2•15 years ago
|
||
(And I know there are a bunch of additional classes in gfx/thebes/ that should be annotated, and probably many more.)
Updated•15 years ago
|
Attachment #414879 -
Flags: review?(benjamin) → review+
Comment 3•15 years ago
|
||
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.
Assignee | ||
Comment 4•15 years ago
|
||
I've started the documentation at https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla
Assignee | ||
Comment 5•15 years ago
|
||
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.
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #435164 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 7•15 years ago
|
||
Attachment #435166 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•15 years ago
|
||
Attachment #435167 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #435167 -
Flags: review? → review?(jst)
Comment 9•15 years ago
|
||
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. :(
Assignee | ||
Comment 10•15 years ago
|
||
(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.
Attachment #435164 -
Flags: review?(jones.chris.g) → review+
Updated•15 years ago
|
Attachment #435167 -
Flags: review?(jst) → review+
Updated•15 years ago
|
Attachment #435166 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•15 years ago
|
||
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
Assignee | ||
Comment 13•15 years ago
|
||
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•15 years ago
|
||
stack class definitely checks for both inheritance and member objects, and we even have tests for it!
Assignee | ||
Comment 15•15 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Assignee: dbaron → nobody
Status: ASSIGNED → NEW
Comment 16•4 years ago
|
||
A bunch of patches landed here in 2010, so I think it makes sense to close the bug as is.
Assignee: nobody → dbaron
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•