Closed Bug 568966 Opened 14 years ago Closed 14 years ago

TM: improve error reporting for proxy handler traps triggered by the engine

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gal)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

Its hard to tell where the error originated from when a handler trap throws. __iterator__ should use this as well.
Blocks: 568867
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);
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Attached patch patch (obsolete) — Splinter Review
Attachment #448153 - Attachment is obsolete: true
Attachment #448154 - Flags: review?(brendan)
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
Attached patch patchSplinter Review
Attachment #448154 - Attachment is obsolete: true
Attachment #448155 - Flags: review?(brendan)
Attachment #448154 - Flags: review?(brendan)
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
Attachment #448155 - Flags: review?(brendan) → review+
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);
http://hg.mozilla.org/tracemonkey/rev/79c2b94a31e5
Whiteboard: fixed-in-tracemonkey
(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
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.
http://hg.mozilla.org/mozilla-central/rev/79c2b94a31e5
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: