Closed Bug 1134253 Opened 5 years ago Closed 4 years ago

Improve TypeError messages when they refer to objects


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox45 --- fixed


(Reporter: jorendorff, Assigned: mrrrgn, Mentored)




(2 files, 2 obsolete files)

For example, say you do

    Object.defineProperty(obj, "x", {...})

but it turns out obj has been frozen. The error message right now is:

    TypeError: x is not extensible

This is wrong and stupid. Ideally, it should say

    TypeError: can't define obj.x: obj is not extensible

We have all the tools to make this happen, we just need to hook them up. This will be more work than it looks like, but it's still a fairly quick way of making Firefox much more friendly to developers.
Depends on: 1113369
Hi, Jason.                                                                       
I'm interesting in JavaScript engine but have no experience. Is it possible for me to fix this bug?
Depends on: 1067942
Mentor: jorendorff
Whiteboard: [mentor=jorendorff]
Assignee: nobody → winter2718
Attached file scratch-work.diff (obsolete) —
Got some time to look at this bug, the error message is okay except my property string comes back with some ugly quotes right now. Just posting this version as a record and to ensure that I'm on the right track. The only real change I made was instructing the decompile function to search the stack. I realize this is messy and can fail in some situations (I'm curious to see if I can trick it into using the wrong variable name), but my other attempts at decompiling have turned up the actual object content.

Tests are all still passing after this change. If it seems good I'll look at fixing other value error messages as well (that seems to be in scope for this bug).
Morgans-MacBook-Pro:src mrrrgn$ _DBG.OBJ/dist/bin/js
js> let MmmBop = {};
js> Object.freeze(MmmBop);
js> Object.defineProperty(MmmBop, "dooWop", {"value": 99});
typein:3:1 TypeError: Can't define MmmBop."dooWop": MmmBop is not extensible
This works, but I'm not asking for review since it makes use of JSDVG_SEARCH_STACK. I'm going to think over the edge cases and try adding 1.) tests and 2.) replacing the stack search with an actual spindex to the value decompiler. Thanks for your patience on this. >.<
Attachment #8691806 - Attachment is obsolete: true
This limits the change in error message to Object.defineProperty only. A call like: Object.defineProperty(frozenObj, "a", {value: 99}) will now produce an error message like: "TypeError: can't define property "a": frozenObj is not extensible" which is a considerable improvement.

For other cases a slight change in the message was added to help clarify what's happening. For example, produces an error message: " object is not extensible" which I believe is slightly more accurate/clear than " is not extensible"

Setting JSDVG_SEARCH_STACK helps with Object.defineProperty but breaks badly when extra arguments are added to constructor functions, that is, undefined) will produce garbage like "Intl.Collator is not extensible" instead of "frozenObj is not extensible" (the message produced when there's no second argument). I looked at fixing this, but it will require changes in decompiler functions which may not be worthwhile here.
Attachment #8693814 - Flags: review?(jorendorff)
Comment on attachment 8693814 [details] [diff] [review]

Review of attachment 8693814 [details] [diff] [review]:

r=me after much confusion.

Homework: Please file a follow-up bug to fix JSDVG_IGNORE_STACK. Find a place where it's used and figure out how to trigger that error; I bet the error message will be wrong, because JSDVG_IGNORE_STACK is wrong.

::: js/src/jsapi.cpp
@@ +173,5 @@
> +            RootedValue val(cx, ObjectValue(*obj));
> +            return ReportValueErrorFlags(cx, flags, code_, JSDVG_SEARCH_STACK, val,
> +                                     nullptr, propName.ptr(), nullptr);
> +        }

Let's drop this special case and do without decompilation for this particular error message.

It would have been correct to allow the caller to ask for decompilation in cases where we know it's correct, like in js::obj_DefineProperty, but for now let's go for the minimum effort to get the error message reasonable...
Attachment #8693814 - Flags: review?(jorendorff) → review+
Attachment #8693814 - Attachment is obsolete: true
Pushing the latest patch after tests.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Duplicate of this bug: 1183241
You need to log in before you can comment on or make changes to this bug.