1415076 - Improve error message mistaking type of descriptor when using defineProperty

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[kikuchi1024r]

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]

Comment 1

[kikuchi1024r]

Attachment: bug1353067.patch

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.

Comment 2

[Tooru Fujisawa [:arai]]

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

Comment 3

[kikuchi1024r]

Attachment: bug1415076.patch

I improved almost all of what you pointed.

but I couldn't understand  about type "undefined".

Comment 4

[Tooru Fujisawa [:arai]]

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.

Comment 5

[kikuchi1024r]

Attachment: bug1415076.patch

thanks for your review!

I changed the code in the former way.

Comment 6

[Tooru Fujisawa [:arai]]

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.

Comment 7

[kikuchi1024r]

Attachment: bug1415076.patch

Thank you for your review again!

I changed what you meant.

Comment 8

[Tooru Fujisawa [:arai]]

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

Comment 9

[kikuchi1024r]

Attachment: bug1415076.patch

Thank you for checking my patch.
I changed what you said.

Comment 10

[Tooru Fujisawa [:arai]]

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.

Comment 11

[kikuchi1024r]

Attachment: bug1415076.patch

I'm sorry for overlooking the spaces.

Now I read and checked the patch file.

Comment 12

[Tooru Fujisawa [:arai]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52625c644b7c
Improve error message about defineProperty r=arai

Comment 14

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52625c644b7cb10f125df9968d7e9dcc16cd86ff
Bug 1415076 - Improve error message about defineProperty r=arai

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/52625c644b7c