Its hard to tell where the error originated from when a handler trap throws. __iterator__ should use this as well.

Rather, JS1.7+ __iterator__ uses special JSINVOKE_ITERATOR (aliased via JSFRAME_ and JSV2F_ prefixes too) to get better error reports for bogus __iterator__ return values. Same idea generalizes to all traps including the new iterate one. /be

A couple examples of the brave new world: var h = { getPropertyDescriptor: function() { } } var p = Proxy.create(h); p.x; x.js:7: TypeError: trap getPropertyDescriptor for p returned an invalid value For our legacy __iterator__ api (this is on objects directly, not on the handler) I am using the same error message: x.js:3: TypeError: trap __iterator__ for ({__iterator__:(function () {return 0;})}) returned an invalid value var o = {}; o.__iterator__ = function () { return 0; } for (i in o);

we know the value is on top of the stack, so we can report a bit better: x.js:3: TypeError: trap __iterator__ for o returned an invalid value

Comment on attachment 448154 [details] [diff] [review] patch >+ uintN error = (flags & JSV2F_CONSTRUCT) ? JSMSG_NOT_CONSTRUCTOR >+ : JSMSG_NOT_FUNCTION; Fits on one line, but if it didn't, avoid right-heavy layout by breaking before ? and putting : under ?. > LeaveTrace(cx); > FrameRegsIter i(cx); > while (!i.done() && !i.pc()) > ++i; > > ptrdiff_t spindex = > !i.done() && StackBase(i.fp()) <= vp && vp < i.sp() > ? vp - i.sp() Pre-existing, but please fix: the ? condition should be parenthesized and the ? should underhang the (. Check the : to underhang the ? too. >diff --git a/js/src/jsfun.h b/js/src/jsfun.h >--- a/js/src/jsfun.h >+++ b/js/src/jsfun.h >@@ -335,17 +335,16 @@ js_DefineFunction(JSContext *cx, JSObjec > uintN nargs, uintN flags); > > /* > * Flags for js_ValueToFunction and js_ReportIsNotFunction. We depend on the > * fact that JSINVOKE_CONSTRUCT (aka JSFRAME_CONSTRUCTING) is 1, and test that > * with #if/#error in jsfun.c. > */ > #define JSV2F_CONSTRUCT JSINVOKE_CONSTRUCT >-#define JSV2F_TRAP JSINVOKE_TRAP I didn't see JSINVOKE_TRAP go in -- is this patch based on another not-yet-committed patch? >@@ -358,20 +358,22 @@ GetCustomIterator(JSContext *cx, JSObjec > > /* If there is no custom __iterator__ method, we are done here. */ > if (*vp == JSVAL_VOID) > return true; > > /* Otherwise call it and return that object. */ > LeaveTrace(cx); > jsval arg = BOOLEAN_TO_JSVAL((flags & JSITER_FOREACH) == 0); >- if (!js_InternalInvoke(cx, obj, *vp, JSINVOKE_TRAP, 1, &arg, vp)) >+ if (!js_InternalCall(cx, obj, *vp, 1, &arg, vp)) > return false; > if (JSVAL_IS_PRIMITIVE(*vp)) { >- js_ReportValueError(cx, JSMSG_BAD_ITERATOR_RETURN, JSDVG_SEARCH_STACK, *vp, NULL); >+ js_ReportValueError2(cx, JSMSG_BAD_TRAP_RETURN_VALUE, >+ -1, OBJECT_TO_JSVAL(obj), NULL, Comment before the call why the -1 works. >+ ReturnedValueMustNotBePrimitive(cx, proxy, ATOM(iterate), *vp); The js.msg change uses "invalid value" which is imprecise -- the only problem is "primitive value", right? SO say that. /be

Comment on attachment 448155 [details] [diff] [review] patch >+ uintN error = (flags & JSV2F_CONSTRUCT) >+ ? JSMSG_NOT_CONSTRUCTOR >+ : JSMSG_NOT_FUNCTION; Fits on one line! Waldo and I would agree here, no complex control flow. It's an expression, like && and ||. > LeaveTrace(cx); > FrameRegsIter i(cx); > while (!i.done() && !i.pc()) > ++i; > > ptrdiff_t spindex = >- !i.done() && StackBase(i.fp()) <= vp && vp < i.sp() >- ? vp - i.sp() >- : flags & JSV2F_SEARCH_STACK ? JSDVG_SEARCH_STACK >- : JSDVG_IGNORE_STACK; >+ (!i.done() && StackBase(i.fp()) <= vp && vp < i.sp()) >+ ? vp - i.sp() >+ : ((flags & JSV2F_SEARCH_STACK) >+ ? JSDVG_SEARCH_STACK >+ : JSDVG_IGNORE_STACK); Could fit second ?: on one line, but if you chain then don't over parenthesize and over-indent! Just let the ? and : string run vertically down the page (analogous to if-elseif). >@@ -62,17 +62,16 @@ enum JSFrameFlags { > JSFRAME_COMPUTED_THIS = 0x02, /* frame.thisv was computed already and > JSVAL_IS_OBJECT(thisv) */ > JSFRAME_ASSIGNING = 0x04, /* a complex (not simplex JOF_ASSIGNING) op > is currently assigning to a property */ > JSFRAME_DEBUGGER = 0x08, /* frame for JS_EvaluateInStackFrame */ > JSFRAME_EVAL = 0x10, /* frame for obj_eval */ > JSFRAME_FLOATING_GENERATOR = 0x20, /* frame copy stored in a generator obj */ > JSFRAME_YIELDING = 0x40, /* js_Interpret dispatched JSOP_YIELD */ >- JSFRAME_ITERATOR = 0x80, /* trying to get an iterator for for-in */ > JSFRAME_GENERATOR = 0x200, /* frame belongs to generator-iterator */ > JSFRAME_OVERRIDE_ARGS = 0x400, /* overridden arguments local variable */ Yuck, gaps. Please rotate the last flag down (or in this case just move stuff up) to fill the gap. >@@ -5627,17 +5627,17 @@ js_Call(JSContext *cx, JSObject *obj, ui > } > if (JSVAL_IS_OBJECT(fval) && JSVAL_TO_OBJECT(fval) != callee) { > argv[-2] = fval; > ok = js_Call(cx, obj, argc, argv, rval); > argv[-2] = OBJECT_TO_JSVAL(callee); > return ok; > } > #endif >- js_ReportIsNotFunction(cx, &argv[-2], js_GetTopStackFrame(cx)->flags & JSFRAME_ITERATOR); >+ js_ReportIsNotFunction(cx, &argv[-2], js_GetTopStackFrame(cx)->flags); You don't even need the js_GetTopStackFrame(cx)->flags any longer. r+me with these fixed. /be

I think this is an existing bug. We strip of the CONSTRUCT flag which ReportIsNotFunction really wants. >- js_ReportIsNotFunction(cx, &argv[-2], js_GetTopStackFrame(cx)->flags & JSFRAME_ITERATOR); >+ js_ReportIsNotFunction(cx, &argv[-2], js_GetTopStackFrame(cx)->flags);

(In reply to comment #10) > http://hg.mozilla.org/tracemonkey/rev/79c2b94a31e5 This did not address comment 8. js_Call is never called with JSFRAME_CONSTRUCT among the top frame's flags. So the diff 6.7 - js_ReportIsNotFunction(cx, &argv[-2], js_GetTopStackFrame(cx)->flags & JSFRAME_ITERATOR); 6.8 + js_ReportIsNotFunction(cx, &argv[-2], js_GetTopStackFrame(cx)->flags); should show new code just passing 0. /be

Follow-up fix (comment 11): http://hg.mozilla.org/tracemonkey/rev/327f3abee3c8

There are some jstests failing due to the change in the error message. Why we're testing for an exact error message I don't know.

Error-message testing isn't quite happymaking, but it's perhaps the least evil when you want to make sure error message quality doesn't regress.