Consider this code: JSAutoTempValueRooter(cx, v); Ruh-roh, Scooby! You've just protected v for the duration of that statement only, against no possible ill effects. People on stackoverflow have two suggestions on fixing this: use a macro, or force the ctor to take a pointer to itself with a runtime assertion. (I suppose a static analysis is feasible as well, but I want something that will fail more eagerly than that, since most of us don't do s-a-enabled builds.) The former doesn't forbid using the class directly, is less facially understandable, and involves macros. The latter means we'd have to do: JSAutoTempValueRooter::JSAutoTempValueRooter(const JSAutoTempValueRooter& tvr, JSContext* cx, jsval v) { JS_ASSERT(this == &tvr); } JSAutoTempValueRooter tvr(tvr, cx, v); ...but I think I can stomach it. Anyone object to the idea?

That's disgusting. :-/ I guess explicit would not do the trick? I'm not a C++ guru, but if it doesn't then what good is it? How likely is this kind of error? If we can't fix it in C++, we could certainly use a treehydra script and the static analysis tinderbox will turn red in the rare event of such a thinko. /be

dbaron may have solved this elsewhere. My "That's disgusting. :-/" was indeed an objection. We shouldn't try to solve non-problems or marginal problems that have yet to bite, at the price of API and code complexity (source and binary). I'll leave it at that. /be

This isn't a non-problem or a marginal problem; I've made this typo a number of different times. Sometimes I've caught it before posting a patch for review; other times reviewers have noticed the mistake. This is very much a real problem.

(In reply to comment #1) > I guess explicit would not do the trick? I'm not a C++ guru, but if it doesn't > then what good is it? It protects you against implicitly constructing an object, like when you pass a 1 to a function taking a vector<int>, you might prefer an error instead of a vector of 1 element. Unfortunately, waldo's code is being plenty explicit about constructing that object. I'm also in favor of static analysis over gross-API. It would be trivial to write. I don't think its a problem that it doesn't fail eagerly, since its an unlikely error: all that matters is that we catch it at all.

This same problem has happened for a number of content classes. It seems we could have some sort of "guard object" annotation we could make on various classes (JSTempValueRooter, nsAutoLock, nsCxPusher, nsAutoGCRoot, nsAutoScriptBlocker, etc., etc.), and verify some invariants about the use of such classes, such as that they're never constructed as temporaries. (If C++ had const constructors (like const methods) we could fix this quite easily, though, by making the const constructor inaccessible, since temporaries are implicitly const... I think.)

Bug 502775 has an analysis in-hand which can prevent certain classes from existing as a temporary, explicit or implied.

That said, I once had another idea for fixing this that *might* work but that I've never tried: add an additional parameter to the constructor, ifdef DEBUG, that's another class (this could be the same class for all guard objects), with a default value. Pass that object the address of a boolean member of the guard object. That other object sets the boolean member in its destructor; the guard object asserts in its destructor that the boolean has already been so set. If my memory of destruction order rules is correct, this might work, in that in the buggy case it would lead to an assert immediately followed by a likely-harmless write-after-destruction. Might be worth trying...

That approach does seem to get working runtime assertions, at least with gcc. Patch shortly.

This seems to work for me. When I put: jsval silly = JSVAL_NULL; JSAutoTempValueRooter(cx, 1, &silly); inside the main function in js.cpp, I get the expected assert when starting the JS shell. (But if I insert " tvr", it starts up.) I think the piece of the C++ standard that this is depending on is that the temporaries in a statement are destroyed in reverse order of creation. (I think it says that.) And the parameter to a temporary's constructor has to be constructed before it. I'm actually not sure that my secondary assertion that the user initialized the boolean correctly is valid, though, since I think it may be allowed for the compiler to introduce extra temporaries in a construct like this.

(In reply to comment #7) Inventive! For fun, I whipped up what you said. It would need to be macroized so that way all guard classes weren't super gross, but, yeah, you're right. Also, from what I understand about sub-object lifetimes, this is well-defined to work across compilers (if they follow the rules...) I'm still not so convinced that a nice little static analysis wouldn't be the way to go, though. It would keep our guards crisp and clean, so that we could (as we should) apply RAII liberally.

That said, I think we should *also* definitely do a static analysis here; there are some existing problems in code that we may or may not execute all that often.

(In reply to comment #10) On second thought, I access 'dead' after its object was destroyed, but that can be fixed... comment 9 obsoletes this anyhow.

I consider this version reviewable, although I'm expecting at the very least some quibbling about the names and the location of the code. I expect we might want to duplicate this somewhere in XPCOM. I think having assertions for this is useful (in addition to a static analysis) since it will help people catch these problems while they're developing the code (and potentially avoid a lot of wasted time trying to debug why a guard object wasn't working) rather than only catching problems after commit to mozilla-central. Who ought to review?

Actually, I can avoid the potentially-bogus assertion by initializing to false earlier. (I didn't start off that way because I started off thinking I was going to use a class and a boolean rather than two classes.) And this fixes a whole bunch of deviations from local style as well.

Clearly I was wrong about this being a hypothetical problem! I stand corrected. /be

Dbaron, can you refresh based on the commit for bug 518675. /be

(In reply to comment #19) > Dbaron, can you refresh based on the commit for bug 518675. Is a new patch needed? Just ignore the json.cpp and jsarray.cpp changes now.

So, it actually turns out this doesn't work on MSVC; assertions don't fire since the temporary argument gets destroyed before the temporary JSAutoTempValueRooter. It doesn't do anything harmful on MSVC; it just doesn't give any useful feedback.

And here's a patch that also works on MSVC, by using a const reference rather than passing by value.

And, for the record, I think what MSVC was doing was valid; it was constructing the implicit temporary for pass-by-value that compilers are allowed to omit if they want to (and gcc does), and that one had a shorter lifetime.

Painful -- Luke, anyone: is there c++[01]x hope for a "explicit named" qualifier pair, or something like that? /be

There are more classes than the patch covers. JSAutoRequest from jsapi.h, JSAutoTempIdRooter and JSAutoEnumStateRooter from jscntxt.h. I will r+ the patch with this adderssed.

(In reply to comment #25) > Painful -- Luke, anyone: is there c++[01]x hope for a "explicit named" > qualifier pair, or something like that? No; this is actually the first time I've seen this error get such attention. I'm still thinking a non-intrusive static analysis (which, as comment 6 explains, is already mostly done) would achieve the same bug-finding effect without the code mutilation. Also, as a hypothetical custom-guard writer, I'm more likely to annotate my class with a simple "JS_nontemporary" than insert the three JS_GUARD_OBJECT_* macros in just the right places.

Igor, I mentioned the same to dbaron yesterday (although I was thinking more broadly of JSAuto* in general), and he said he wanted to get it working on just the one to start. (I also did a grep for JSAuto, and none of the other classes had mistakes.) I'd personally do it all at once, but it seems reasonable to split it up as well.

(In reply to comment #27) > I'm still thinking a non-intrusive static analysis (which, as comment 6 > explains, is already mostly done) would achieve the same bug-finding effect > without the code mutilation. It would, for the bugs that are subtle enough to get checked in. But it wouldn't for the more easily observed bugs that cause people to spend hours debugging something and then discover this tiny little typo that caused it.

Defense in depth: we want the assertion (ugly boilerplate as a pragmatic move; we are not purists) *and* the static analysis: * Dbaron's point about prompt developer feedback via the assertion is good. * The static analysis is justified because all-paths coverage is an exponential problem and we can't rely only on the assertion. /be

Comment on attachment 402909 [details] [diff] [review] patch for JSAuto* >+class JSGuardObjectNotifier >+{ >+private: >+ JSBool* mStatementDone; ... >+ ~JSGuardObjectNotifier() { >+ *mStatementDone = JS_TRUE; >+ } ... >+ JSGuardObjectNotificationReceiver() : mStatementDone(JS_FALSE) {} Nit: replace JSBool JS_TRUE JS_FALSE with bool true false as this is C++ code.

http://hg.mozilla.org/tracemonkey/rev/d4f8378c0d6e Perhaps we should mark this fixed-in-tracemonkey and spin off a second bug for the static analysis bit, if there isn't one already?

(In reply to comment #33) > Perhaps we should mark this fixed-in-tracemonkey and spin off a second bug for > the static analysis bit, if there isn't one already? bug 519171