argv[-2] looks either magic or random - inline helpers are prettier.

Comment on attachment 396340 [details] [diff] [review] patch This is all great, but would calleeFunction as well be even better? Not if it drags in jsfun.h, but jsinterp.h already includes that header. Also, callee now seems misnamed -- calleeOrNull or maybeCallee? Want to get things right as we can while patching this area of argv[-2] confusion. So one more patch and I will stamp. Thanks, /be

(In reply to comment #1) > This is all great, but would calleeFunction as well be even better? Not if it > drags in jsfun.h, but jsinterp.h already includes that header. It doesn't strike me as either better or worse taken on its own, but if I saw a method named calleeFunction I'd expect it to return a JSFunction*. > Also, callee now seems misnamed -- calleeOrNull or maybeCallee? Want to get > things right as we can while patching this area of argv[-2] confusion. The only case in which argv[-2] is not a non-null object is when a native function indiscriminately steals argv[-2] as a root -- correct?

(In reply to comment #2) > (In reply to comment #1) > > This is all great, but would calleeFunction as well be even better? Not if it > > drags in jsfun.h, but jsinterp.h already includes that header. > > It doesn't strike me as either better or worse taken on its own, but if I saw a > method named calleeFunction I'd expect it to return a JSFunction*. Yes, sorry to be unclear -- that's exactly what I meant. calleeFunction would be in addition to what's in the current patch, to handle stuff like this: + JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, fp->calleeObject())); > > Also, callee now seems misnamed -- calleeOrNull or maybeCallee? Want to get > > things right as we can while patching this area of argv[-2] confusion. > > The only case in which argv[-2] is not a non-null object is when a native > function indiscriminately steals argv[-2] as a root -- correct? No, I'm talking about how JSStackFrame::callee() null-tests argv and returns null. It's a maybe-callee or callee-or-null-if-global-code method. /be

(In reply to comment #3) > Yes, sorry to be unclear -- that's exactly what I meant. calleeFunction would > be in addition to what's in the current patch, to handle stuff like this: > > + JS_ASSERT(fp->fun == GET_FUNCTION_PRIVATE(cx, fp->calleeObject())); Sounds good to me, I was missing the "in addition" part, all cool with that noted. > No, I'm talking about how JSStackFrame::callee() null-tests argv and returns > null. It's a maybe-callee or callee-or-null-if-global-code method. Oh, duh. Rookie mistake... Maybe-null is a pretty little mistake to make. Rather than requiring sites to null-test, I think I'd rather add a not-null assertion to |calleeObject| (JSVAL_TO_OBJECT doesn't contain such a check, and probably can't). If a site is aware of maybe-null, it should use |calleeValue| and check |JSVAL_IS_NULL|, would rather not make every object-needing place pay that price in line noise.

(In reply to comment #4) > > No, I'm talking about how JSStackFrame::callee() null-tests argv and returns > > null. It's a maybe-callee or callee-or-null-if-global-code method. > > Oh, duh. Rookie mistake... I fear we're still talking about two different things being null. I'm talking about the return value of callee being null to signify a global frame (one whose argv is null). Are you talking about a JSVAL_NULL in argv[-2] insane embedding case where the returned JSObject* would be 0? /be

Check the NoSuchMethod code path carefully, there may be a JSVAL_NULL in argv[-2] for a little bit. Don't want to burn that case. It should get replaced with a proxy thingie. /be

(In reply to comment #5) > I fear we're still talking about two different things being null. No, I got you, just perhaps being overly terse.

New patch soon? /be

Comment on attachment 408128 [details] [diff] [review] refreshed >+++ b/js/src/jstracer.cpp >@@ -3363,9 +3363,7 @@ FlushNativeStackFrame(JSContext* cx, uns > * has to be the same JSFunction (FIXME: bug 471425, eliminate fp->callee). > */ > JS_ASSERT(JSVAL_IS_OBJECT(fp->argv[-1])); >- JS_ASSERT(HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(fp->argv[-2]))); >- JS_ASSERT(GET_FUNCTION_PRIVATE(cx, JSVAL_TO_OBJECT(fp->argv[-2])) == >- GET_FUNCTION_PRIVATE(cx, fp->callee())); >+ JS_ASSERT(HAS_FUNCTION_CLASS(fp->calleeObject())); > JS_ASSERT(GET_FUNCTION_PRIVATE(cx, fp->callee()) == fp->fun); You cut out the second assertion, this one: JS_ASSERT(GET_FUNCTION_PRIVATE(cx, JSVAL_TO_OBJECT(fp->argv[-2])) == GET_FUNCTION_PRIVATE(cx, fp->callee())); but it is usefully different from the subsequent one. >@@ -5557,7 +5555,7 @@ SynthesizeSlowNativeFrame(InterpState& s > fp->thisv = state.nativeVp[1]; > fp->argc = state.nativeVpLen - 2; > fp->argv = state.nativeVp + 2; >- fp->fun = GET_FUNCTION_PRIVATE(cx, JSVAL_TO_OBJECT(fp->argv[-2])); >+ fp->fun = GET_FUNCTION_PRIVATE(cx, GET_FUNCTION_PRIVATE(cx, fp->calleeObject())); What's that inner GET_FUNCTION_PRIVATE doing there? Do not want :-P. /be

(In reply to comment #10) > You cut out the second assertion, this one: > > JS_ASSERT(GET_FUNCTION_PRIVATE(cx, > JSVAL_TO_OBJECT(fp->argv[-2])) == > GET_FUNCTION_PRIVATE(cx, fp->callee())); > > but it is usefully different from the subsequent one. Is it useful? It seems to reduce to: GET_FUNCTION_PRIVATE(cx->fp->callee()) == GET_FUNCTION_PRIVATE(cx->fp->callee()) > What's that inner GET_FUNCTION_PRIVATE doing there? Do not want :-P. Yuck, not sure how that got there. Thanks.

(In reply to comment #11) > (In reply to comment #10) > > You cut out the second assertion, this one: > > > > JS_ASSERT(GET_FUNCTION_PRIVATE(cx, > > JSVAL_TO_OBJECT(fp->argv[-2])) == > > GET_FUNCTION_PRIVATE(cx, fp->callee())); > > > > but it is usefully different from the subsequent one. > > Is it useful? It seems to reduce to: > > GET_FUNCTION_PRIVATE(cx->fp->callee()) == > GET_FUNCTION_PRIVATE(cx->fp->callee()) Duh, nm. > > What's that inner GET_FUNCTION_PRIVATE doing there? Do not want :-P. > > Yuck, not sure how that got there. Thanks. New patch and I'll stamp. Thanks, /be