Closed Bug 1415076 Opened 2 years ago Closed 2 years ago

Improve error message mistaking type of descriptor when using defineProperty

Categories

(Core :: JavaScript Engine, defect, P3, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: kikuchi1024r, Assigned: kikuchi1024r)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.89 Safari/537.36

Steps to reproduce:

Improve this error message.

js> Object.defineProperty({},'key',1);


Actual results:

TypeError: ({}) is not a non-null object


Expected results:

descriptor must be object, but got [type] [what you write]
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Blocks: 1353067
Attached patch bug1353067.patch (obsolete) — Splinter Review
Changed error message. 

In this patch, the error message is below.
```
js> Object.defineProperty({},'key',1);
typein:1:1 TypeError: descriptor must be an object, got number '1'

```
 the grammer is "{0} must be an object, got {1} '{2}'", and when you use defineProperty, {0} is "descriptor" ,{1} is the type of descriptor, and {2} is the descripter which you write down literely.
Attachment #8925812 - Flags: review?(arai.unmht)
Comment on attachment 8925812 [details] [diff] [review]
bug1353067.patch

Review of attachment 8925812 [details] [diff] [review]:
-----------------------------------------------------------------

the following case doesn't look correct.
it's because ThrowTypeError tried to decompile the parameter when it's neither int32 or string.
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/vm/SelfHosting.cpp#291-296

js> Object.defineProperty({}, 1, true)
typein:4:1 TypeError: descriptor must be an object, got boolean 'attributes'
Stack:
  @typein:4:1
js> Object.defineProperty({}, 1, Symbol.iterator)
typein:5:1 TypeError: descriptor must be an object, got symbol 'attributes'
Stack:
  @typein:5:1
js> Object.defineProperty({}, 1, null) 
typein:6:1 TypeError: descriptor must be an object, got object 'attributes'
Stack:
  @typein:6:1
js> Object.defineProperty({}, 1, undefined)
typein:7:1 TypeError: descriptor must be an object, got undefined 'attributes'
Stack:
  @typein:7:1
js> Object.defineProperty({}, 1, 1.1)       
typein:8:1 TypeError: descriptor must be an object, got number 'attributes'
Stack:
  @typein:8:1

You need to call ToSource, and remove single quote from the message template, like this:
https://hg.mozilla.org/mozilla-central/rev/d8176df5f67e#l5.13
Attachment #8925812 - Flags: review?(arai.unmht) → feedback+
Attached patch bug1415076.patch (obsolete) — Splinter Review
I improved almost all of what you pointed.

but I couldn't understand  about type "undefined".
Attachment #8925812 - Attachment is obsolete: true
Attachment #8925832 - Flags: review?(arai.unmht)
Comment on attachment 8925832 [details] [diff] [review]
bug1415076.patch

Review of attachment 8925832 [details] [diff] [review]:
-----------------------------------------------------------------

I've written the review comments for the patch, but I think it's better using the different way than that, reusing existing functions.

there's ReportNotObjectWithName function defined in jsobj.cpp.
that is used by WeakMap's case (bug 774744)
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/jsobj.cpp#103-113

So, exposing it to self-hosted environment (the environment for Object.js etc) and calling it will solve the "undefined" handling and also that will provide the better error message ("the number something" etc).

To add self-hosting intrinsic (global function in Self-Hosted environment), you need to add the entry to intrinsic_functions array in js/src/vm/SelfHosting.js,
with JS_FN.  (JS_INLINABLE_FN is for function that can be inlined while JIT compiling, but in this case it's not necessary).
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/vm/SelfHosting.cpp#2204

WarnDeprecatedStringMethod there would be a good example.
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/vm/SelfHosting.cpp#2590
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/vm/SelfHosting.cpp#1843-1875
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/builtin/String.js#61

So, just like that, pass a string "descriptor" and the attributes value to the intrinsic, and the intrinsic needs to pass them to ReportNotObjectWithName.

::: js/src/builtin/Object.js
@@ +204,4 @@
>      // Step 3 (Call to 6.2.4.5 ToPropertyDescriptor).
>  
>      // 6.2.4.5 ToPropertyDescriptor, step 1.
> +    if (!IsObject(attributes)){

please put a space before "{"

@@ +204,5 @@
>      // Step 3 (Call to 6.2.4.5 ToPropertyDescriptor).
>  
>      // 6.2.4.5 ToPropertyDescriptor, step 1.
> +    if (!IsObject(attributes)){
> +        if(typeof(attributes) === undefined){

please put a space after "if".
also, `attributes === undefined`,
also, please remove braces around single line then-clause and else-clause.

@@ +205,5 @@
>  
>      // 6.2.4.5 ToPropertyDescriptor, step 1.
> +    if (!IsObject(attributes)){
> +        if(typeof(attributes) === undefined){
> +            ThrowTypeError(JSMSG_NOT_NONNULL_OBJECT_NAME, "descriptor", typeof(attributes));

"undefined", instead of typeof(attributes)

@@ +207,5 @@
> +    if (!IsObject(attributes)){
> +        if(typeof(attributes) === undefined){
> +            ThrowTypeError(JSMSG_NOT_NONNULL_OBJECT_NAME, "descriptor", typeof(attributes));
> +        }else{
> +            ThrowTypeError(JSMSG_NOT_NONNULL_OBJECT_TYPE, "descriptor", typeof(attributes), ToSource(attributes));

`typeof attributes`
(no parentheses)

::: js/src/js.msg
@@ +84,4 @@
>  MSG_DEF(JSMSG_MISSING_FUN_ARG,         2, JSEXN_TYPEERR, "missing argument {0} when calling function {1}")
>  MSG_DEF(JSMSG_NOT_NONNULL_OBJECT,      1, JSEXN_TYPEERR, "{0} is not a non-null object")
>  MSG_DEF(JSMSG_NOT_NONNULL_OBJECT_NAME, 2, JSEXN_TYPEERR, "{0} must be an object, got {1}")
> +MSG_DEF(JSMSG_NOT_NONNULL_OBJECT_TYPE, 3, JSEXN_TYPEERR, "{0} must be an object, got {1} '{2}'")

single quotes are not necessary.
please remove them.
Attachment #8925832 - Flags: review?(arai.unmht) → feedback+
Assignee: nobody → kikuchi1024r
Severity: normal → minor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug1415076.patch (obsolete) — Splinter Review
thanks for your review!

I changed the code in the former way.
Attachment #8925832 - Attachment is obsolete: true
Attachment #8926741 - Flags: review?(arai.unmht)
Comment on attachment 8926741 [details] [diff] [review]
bug1415076.patch

Review of attachment 8926741 [details] [diff] [review]:
-----------------------------------------------------------------

thanks :)

the overall approach looks good.
please address the following comments.

::: js/src/builtin/Object.js
@@ +205,4 @@
>  
>      // 6.2.4.5 ToPropertyDescriptor, step 1.
>      if (!IsObject(attributes))
> +        WarnDescriptorNotObject("descriptor", attributes);

ThrowNotObjectWithName maybe?

because, this is throwing error instead of warning.
and also "descriptor" is passed as argument, so this function is supposed to be more generic.

same for other places with same name

::: js/src/vm/SelfHosting.cpp
@@ +1936,5 @@
> +intrinsic_WarnDescriptorNotObject(JSContext* cx, unsigned argc ,Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc,vp);
> +    if (!args[1].isObject()) {
> +        ReportNotObjectWithName(cx, "descriptor", args[1]);

"descriptor" should be a string corresponds to args[0].
you can extract const char* from string value, or use enum value (see js/src/builtin/SelfHostingDefines.h) for each case (currently only "descriptor")

also, `!args[1].isObject()` should always be true. so put `MOZ_ASSERT(!args[1].isObject())` instead of branch.

also, please put `MOZ_ASSERT(args.length() == 2);` like others, to make sure there's always 2 arguments.

also, please put `return false` at the end of this function.

@@ +2668,4 @@
>                      IntrinsicStringSplitString),
>      JS_FN("StringSplitStringLimit", intrinsic_StringSplitStringLimit, 3, 0),
>      JS_FN("WarnDeprecatedStringMethod", intrinsic_WarnDeprecatedStringMethod, 2, 0),
> +    JS_FN("WarnDescriptorNotObject", intrinsic_WarnDescriptorNotObject, 3, 0),

this function receives only 2 arguments, so please replace 3 with 2.
Attachment #8926741 - Flags: review?(arai.unmht) → feedback+
Attached patch bug1415076.patch (obsolete) — Splinter Review
Thank you for your review again!

I changed what you meant.
Attachment #8926741 - Attachment is obsolete: true
Attachment #8926767 - Flags: review?(arai.unmht)
Comment on attachment 8926767 [details] [diff] [review]
bug1415076.patch

Review of attachment 8926767 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again!

::: js/src/builtin/Object.js
@@ +205,4 @@
>  
>      // 6.2.4.5 ToPropertyDescriptor, step 1.
>      if (!IsObject(attributes))
> +        ArgTypeNotObject(NOT_OBJECT_KIND_DESCRIPTOR, attributes);

Please prepend "Throw" to function name, to clarify this function throws.
here and other places.

::: js/src/vm/SelfHosting.cpp
@@ +1937,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc,vp);
> +    MOZ_ASSERT(!args[1].isObject());
> +    MOZ_ASSERT(args[0].isNumber());
> +    MOZ_ASSERT(args.length() == 2);

please reorder like:
	
    MOZ_ASSERT(args.length() == 2);
    MOZ_ASSERT(args[0].isNumber());
    MOZ_ASSERT(!args[1].isObject());

so the length is checked before accessing elements, and elements are checked in order.

@@ +1942,5 @@
> +    if (args[0].toNumber() == NOT_OBJECT_KIND_DESCRIPTOR)
> +        ReportNotObjectWithName(cx, "descriptor", args[1]);
> +    else
> +        MOZ_CRASH("unexpected kind");
> +    

please remove white spaces in this line
Attachment #8926767 - Flags: review?(arai.unmht) → feedback+
Attached patch bug1415076.patch (obsolete) — Splinter Review
Thank you for checking my patch.
I changed what you said.
Attachment #8926767 - Attachment is obsolete: true
Attachment #8927192 - Flags: review?(arai.unmht)
Comment on attachment 8927192 [details] [diff] [review]
bug1415076.patch

Review of attachment 8927192 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, almost there!

::: js/src/builtin/Object.js
@@ +204,4 @@
>      // Step 3 (Call to 6.2.4.5 ToPropertyDescriptor).
>  
>      // 6.2.4.5 ToPropertyDescriptor, step 1.
> +    //

please remove this line.

@@ +210,5 @@
>  
>      // 6.2.4.5 ToPropertyDescriptor, step 2.
>      var attrs = 0, hasValue = false;
>      var value, getter = null, setter = null;
> +    

please remove white spaces here.

it would be nice to check the patch file to see if there's no unnecessary changes, after exporting it.
Attachment #8927192 - Flags: review?(arai.unmht) → feedback+
Attached patch bug1415076.patchSplinter Review
I'm sorry for overlooking the spaces.

Now I read and checked the patch file.
Attachment #8927192 - Attachment is obsolete: true
Attachment #8927197 - Flags: review?(arai.unmht)
Comment on attachment 8927197 [details] [diff] [review]
bug1415076.patch

Review of attachment 8927197 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Great work :)

Here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dd9c371b38e06bbb29447382c886aacbbb524de
Attachment #8927197 - Flags: review?(arai.unmht) → review+
Priority: -- → P3
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52625c644b7c
Improve error message about defineProperty r=arai
https://hg.mozilla.org/mozilla-central/rev/52625c644b7c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.