1180290 - Builtin accessor functions must be named 'get foo' or 'set foo' (where 'foo' is the name of the property used to access the accessor)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Leo Balter]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Steps to reproduce:

Object.getOwnPropertyDescriptor(Map.prototype, "size").get.name

// expected to bet "get size" 


Actual results:

current value is "size" only.


Expected results:

Object.getOwnPropertyDescriptor(Map.prototype, "size").get.name === "get size"

From the ES6 Spec:

Functions that are specified as get or set accessor functions of built-in properties have "get " or "set " prepended to the property name string.

Comment 1

[Leo Balter]

Updating this issue to extend it to Set.prototype.size, the same issue is present there.

Comment 2

[Jeff Walden [:Waldo]]

This generalizes to all built-in accessor properties:

* the size accessors here.
* Object.prototype.__proto__,
* %TypedArray%.prototype.{buffer,byteLength,byteOffset,length},
* %TypedArray%.prototype[@@toStringTag],
* %TypedArray%[@@species],
* Array[@@species],
* RegExp.prototype.{flags,global,ignoreCase,multiline,source,sticky,unicode},
* RegExp[@@species],

...and then I lost interest enumerating them all.  The full list is in the spec.

Fairly simple changes to jsapi.cpp to fix this appear not supremely difficult, as regards any accessor that's defined in C++.  Mostly.

But some accessor functions are self-hosted.  A top-level implementation of such a function may require special treatment to get the proper name.  Intl code has a few of these, for example Intl_DateTimeFormat_format_get -- but they're defined ad-hoc and so require ad-hoc treatment.
If there are top-level accessors, defined systematically in the same way self-hosted builtin member functions are defined (I can't find any at a skim, but I might have missed something), they could require additional adjustment beyond that required to deal with C++-implemented accessors.

And what if the self-hosted accessor *isn't* implemented at top level?  ES6 also gives this "get"/"set"-plus-space prefixing to accessor function names for accessors defined in object/class literal syntax, i.e. |assertEq(Object.getOwnPropertyDescriptor({ get foo(){} }, "foo").name, "get foo");|.  We might or might not need to fix this, to fix this bug.

Anyway.  For someone looking to do a non-trivial first bug that's more than just mechanical changes, this is probably a pretty good one.  But it'll probably take some tracking down of all the places needing changing in the code to be absolutely sure it's fully fixt.

Comment 3

[Till Schneidereit [:till]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> This generalizes to all built-in accessor properties:
> But some accessor functions are self-hosted.  A top-level implementation of
> such a function may require special treatment to get the proper name.  Intl
> code has a few of these, for example Intl_DateTimeFormat_format_get -- but
> they're defined ad-hoc and so require ad-hoc treatment.
> If there are top-level accessors, defined systematically in the same way
> self-hosted builtin member functions are defined (I can't find any at a
> skim, but I might have missed something), they could require additional
> adjustment beyond that required to deal with C++-implemented accessors.

I don't think self-hosted properties are much of a problem. They're installed on the target object in C++ just as native properties, in JS_DefineProperties[1]. The function actually doing the definition is DefineSelfHostedProperty, but that takes an id just as DefinePropertyById. It'll be slightly annoying to atomize two instead of one ids, but that's about it.

Mostly, I'd imagine lots of test breakage caused by this.

> And what if the self-hosted accessor *isn't* implemented at top level?  ES6
> also gives this "get"/"set"-plus-space prefixing to accessor function names
> for accessors defined in object/class literal syntax, i.e.
> |assertEq(Object.getOwnPropertyDescriptor({ get foo(){} }, "foo").name, "get
> foo");|.  We might or might not need to fix this, to fix this bug.

This isn't really about self-hosted properties so much as about content code properties, right? I guess we have to change what we do for these in the bytecode emitter.

> Anyway.  For someone looking to do a non-trivial first bug that's more than
> just mechanical changes, this is probably a pretty good one.  But it'll
> probably take some tracking down of all the places needing changing in the
> code to be absolutely sure it's fully fixt.

Yeah, that's going to be the main thing taking time here.

Comment 4

[Leo Balter]

> But it'll probably take some tracking down of all the places needing changing
> in the code to be absolutely sure it's fully fixt.

For reference, these are the specific topics from the ES6 spec with accessor properties to have get or set prepended to it's name:

21.2.4.2 get RegExp [ @@species ]
21.2.5.3 get RegExp.prototype.flags
21.2.5.4 get RegExp.prototype.global
21.2.5.5 get RegExp.prototype.ignoreCase
21.2.5.7 get RegExp.prototype.multiline
21.2.5.10 get RegExp.prototype.source
21.2.5.12 get RegExp.prototype.sticky
21.2.5.15 get RegExp.prototype.unicode
22.1.2.5 get Array [ @@species ]
22.2.2.4 get %TypedArray% [ @@species ]
22.2.3.1 get  %TypedArray%.prototype.buffer
22.2.3.2 get  %TypedArray%.prototype.byteLength
22.2.3.3 get  %TypedArray%.prototype.byteOffset
22.2.3.17 get  %TypedArray%.prototype.length
22.2.3.31 get %TypedArray%.prototype [ @@toStringTag ]
23.1.2.2 get Map [ @@species ]
23.1.3.10 get Map.prototype.size
23.2.2.2 get Set [ @@species ]
23.2.3.9 get Set.prototype.size
24.1.3.3 get ArrayBuffer [ @@species ]
24.1.4.1 get ArrayBuffer.prototype.byteLength
24.2.4.1 get DataView.prototype.buffer
24.2.4.2 get DataView.prototype.byteLength
24.2.4.3 get DataView.prototype.byteOffset
25.4.4.6 get Promise [ @@species ]
B.2.2.1.1 get Object.prototype.__proto__
B.2.2.1.2 set Object.prototype.__proto__

Comment 5

[Boris Zbarsky [:bzbarsky]]

Should we also have this "get"/"set" thing on WebIDL getters setters?  Seems like we should.

Comment 6

[Leo Balter]

Maybe not, based on Function instances name (19.2.4.2 http://www.ecma-international.org/ecma-262/6.0/index.html#sec-function-instances-name) and all the calls to the abstract setFunctionName(9.2.11 http://www.ecma-international.org/ecma-262/6.0/index.html#sec-setfunctionname).

The chapter 17 specify this prefix rule is for *accessor functions of built-in properties*.

> Functions that are specified as get or set accessor functions of built-in properties have "get " or "set " prepended to the property name string."

Comment 7

[Boris Zbarsky [:bzbarsky]]

Here's a question.  What do class getter/setter names default to in ES6?

Comment 8

[Leo Balter]

> What do class getter/setter names default to in ES6?

If I read well the spec, it's only the function name.

Ref: http://www.ecma-international.org/ecma-262/6.0/index.html#sec-property-accessors-runtime-semantics-evaluation

A `class C { get fn() {} }` will have a `Object.getOwnPropertyDescriptor(C.prototype, 'fn').get.name === 'fn'`

And SpiderMonkey is still returning an empty string.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  So once again we have to decide whether web idl should act more like builtins or more like classes, yay.

Comment 10

[Leo Balter]

I would say prefixing every accessor property in JS would be good.

With these definitions I wouldn't prefix or change anything else unless it's specific.

Comment 11

[Tooru Fujisawa [:arai]]

Any progress on Web IDL?

I'm have draft patches but it affects accessors on DOM, like documentElement and innerHTML, which is tested in following:
https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/dom/bindings/test/test_exception_messages.html#21
>       [ 'Object.getOwnPropertyDescriptor(Document.prototype, "documentElement").get.call({})',
>         "'documentElement' getter called on an object that does not implement interface Document.",
>         "bogus getter this object" ],
>       [ 'Object.getOwnPropertyDescriptor(Element.prototype, "innerHTML").set.call({})',
>         "'innerHTML' setter called on an object that does not implement interface Element.",

Can they be 'get documentElement' and 'set innerHTML' ?

Comment 12

[Tooru Fujisawa [:arai]]

here's overview:
  1. add prefix parameter to IdToFunctionName
  2. change DefinePropertyById to use function name generetad by IdToFunctionName for native getter/setter
     (self-hosted builtin getters are fixed in bug 1235656)
  3. fix some specific cases for ArrayBuffer, DataView, and TypedArrays to use prefixed name directly when creating function

Then, with 2, DefineProperties is also affected and CreateInterfacePrototypeObject generates getter/setter with "get"/"set" prefix for bindings.
https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/dom/bindings/BindingUtils.cpp#768

If DOM bindings should not have "get"/"set" prefix, we should have new JSPROP_ flag to enable or disable prefixing (is there any free bit?)

Comment 13

[Boris Zbarsky [:bzbarsky]]

In terms of the Web IDL behavior, here is the current situation:

1)  The Web IDL spec does not define .name for the functions it defines.  See also https://www.w3.org/Bugs/Public/show_bug.cgi?id=22392
2)  Current behavior in browsers for accessor names for IDL attributes is as follows:

  Firefox: The attribute name
  Chrome: Empty string
  Safari: Claims to have an undefined getter (as in, getOwnPropertyDescriptor just lies)
  MS Edge: The attribute name
  MS IE 11: undefined (getter is present, but has .name === undefined)

Doesn't seem to me like web compat is a strong issue here, and given the discussion in that w3.org bug report I think we should probably just change Web IDL to define the .name and have it be "get whatever".

Comment 14

[Tooru Fujisawa [:arai]]

Thank you :D
I'll go with current patch that changes all names for getter/setter.
Will post it after some more test.

Comment 15

[Cameron McCormack (:heycam)]

Thanks for testing, Boris.  I've made the change in the spec:

https://github.com/heycam/webidl/commit/a275e567f07c23cde20d1c8b1dd50574355c0d74

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add prefix parameter to IdToFunctionName.

Prepared 5 patches (comment #12 + 2 tests).

Part 1 adds prefix parameter to IdToFunctionName, that could be nullptr, "get", or "set" for now.
That was a part of bug 1235656 patch.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Handle prefix in DefinePropertyById.

Part 2 changes DefinePropertyById to use function name generated by IdToFunctionName, with "get" or "set" for native accessors.

It's handled only for native accessors now, as self-hosted builtin accessor already have "get" prefix by _SetCanonicalName.  might it be better applying same change to DefineSelfHostedProperty as well and remove _SetCanonicalName call from self-hosted accessors?

With this change, accessor name for DOM bindings are also changed, and the testcase for it is fixed in Part 5.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use canonical name in native getter.

Part 3 fixes getters for ArrayBuffer, DataView, and TypedArray.  They calls NewNativeFunction directly, so added Atomize with "get FOO" string literal.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add tests for builtin getter name.

Part 4 adds testcase for builtin getter properties, some of them are fixed in bug 1235656.
Some testcases are commented out, as they're not available.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Fix testcase for DOM accessors.

Part 5 fixes testcase for DOM accessors, to check that "get" or "set" prefixed name in error message.


Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=025d051bbe3f

Comment 21

[Till Schneidereit [:till]]

Comment on attachment 8703886 [details] [diff] [review]
Part 1: Add prefix parameter to IdToFunctionName.

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

Looks great, r=me with nits addressed.

::: js/src/jsfun.cpp
@@ +2253,4 @@
>   * property id.
>   *
>   * Function names are always strings. If id is the well-known @@iterator
>   * symbol, this returns "[Symbol.iterator]".

Add a paragraph below this:

If a prefix is supplied the final name is |prefix + " " + name|.

@@ +2254,5 @@
>   *
>   * Function names are always strings. If id is the well-known @@iterator
>   * symbol, this returns "[Symbol.iterator]".
>   *
>   * Implements step 4 of SetFunctionName in ES6 draft rev 27 (24 Aug 2014).

Please update this comment to say that it implements steps 4 and 5 of SetFunctionName, and that it's based on the final spec.

@@ +2257,5 @@
>   *
>   * Implements step 4 of SetFunctionName in ES6 draft rev 27 (24 Aug 2014).
>   */
>  JSAtom*
> +js::IdToFunctionName(JSContext* cx, HandleId id, const char* prefix)

Nit: add " /* = nullptr */" to the prefix parameter.

@@ +2262,2 @@
>  {
> +    if (JSID_IS_ATOM(id)) {

I think you should be able to simplify this to just

if (JSID_IS_ATOM(id) && !prefix)
   return JSID_TO_ATOM(id);

And remove the handling of the prefix from this entirely. The general case at the end of the function should work just fine for that.

::: js/src/jsfun.h
@@ +663,5 @@
>                       NewObjectKind newKind = GenericObject,
>                       NewFunctionProtoHandling protoHandling = NewFunctionClassProto);
>  
>  extern JSAtom*
> +IdToFunctionName(JSContext* cx, HandleId id, const char* prefix=nullptr);

Nit: spaces around the "=".

Comment 22

[Till Schneidereit [:till]]

Comment on attachment 8703898 [details] [diff] [review]
Part 2: Handle prefix in DefinePropertyById.

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

I think you need to fold part 5 into this, otherwise the test might fail during bisecting.

Comment 23

[Till Schneidereit [:till]]

Comment on attachment 8703899 [details] [diff] [review]
Part 3: Use canonical name in native getter.

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

r=me with feedback addressed.

::: js/src/vm/ArrayBufferObject.cpp
@@ +1444,1 @@
>      RootedId byteLengthId(cx, NameToId(cx->names().byteLength));

Use IdToFunctionName here instead of manually creating the atom.

::: js/src/vm/TypedArrayObject.cpp
@@ +2094,5 @@
>  }
>  
>  template<Value ValueGetter(DataViewObject* view)>
>  bool
> +DataViewObject::defineGetter(JSContext* cx, PropertyName* name, HandleAtom atom, HandleNativeObject proto)

Can't you create the atom inside this function by using IdToFunctionName(id) after line 2100? Then all the callsites below can stay unchanged.

Comment 24

[Till Schneidereit [:till]]

Comment on attachment 8703900 [details] [diff] [review]
Part 4: Add tests for builtin getter name.

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

Looks great. I really hope this is web-compatible, but it seems somewhat likely.

Comment 25

[Boris Zbarsky [:bzbarsky]]

So the new error message ends up looking like:

  ""'get documentElement' getter ..."

which is a little silly.  It really would be better if it just said "'documentElement' getter ..." as it used to.  Right now this string is coming from JS_GetFunctionDisplayId on the getter function.  Is there some other API that will return the original thing?  If not, then it might make sense to use funcNameStr.get()+4 in the call to JS_ReportErrorNumberUC in ThrowInvalidThis in the getter/setter case (the caller would need to indicate when we're in it), with a comment about how we're skipping over the "get " or "set " prefix.  That's assuming that we're really 100% guaranteed the prefix is there in all the cases we care about, including aliases, Xrays, etc (would want to add tests for these if possible).  And probably a MOZ_CRASH if funcNameStr.Length() < 4...

Comment 26

[Tooru Fujisawa [:arai]]

With current implementation, only "get documentElement" string is stored on the JSFunction, and no API to return "documentElement".

I'll try implementing the '+4' patch.

btw, do that error message have to say "getter" ?
how about always using MSG_METHOD_THIS_DOES_NOT_IMPLEMENT_INTERFACE instead of MSG_GETTER_THIS_DOES_NOT_IMPLEMENT_INTERFACE ?

https://dxr.mozilla.org/mozilla-central/rev/0771c5eab32f0cee4f7d12bc382298a81e0eabb2/dom/bindings/Errors.msg#30
> MSG_DEF(MSG_METHOD_THIS_DOES_NOT_IMPLEMENT_INTERFACE, 2, JSEXN_TYPEERR, "'{0}' called on an object that does not implement interface {1}.")
> MSG_DEF(MSG_METHOD_THIS_UNWRAPPING_DENIED, 1, JSEXN_TYPEERR, "Permission to call '{0}' denied.")
> MSG_DEF(MSG_GETTER_THIS_DOES_NOT_IMPLEMENT_INTERFACE, 2, JSEXN_TYPEERR, "'{0}' getter called on an object that does not implement interface {1}.")
> MSG_DEF(MSG_GETTER_THIS_UNWRAPPING_DENIED, 1, JSEXN_TYPEERR, "Permission to call '{0}' getter denied.")
> MSG_DEF(MSG_SETTER_THIS_DOES_NOT_IMPLEMENT_INTERFACE, 2, JSEXN_TYPEERR, "'{0}' setter called on an object that does not implement interface {1}.")
> MSG_DEF(MSG_SETTER_THIS_UNWRAPPING_DENIED, 1, JSEXN_TYPEERR, "Permission to call '{0}' setter denied.")

I think that error message is not common, as the testcase uses Object.getOwnPropertyDescriptor to get raw function of the getter property.  in that case saying '"get documentElement" called on an object ...' will also make sense.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> btw, do that error message have to say "getter" ?

It doesn't, really.  It can say whatever it wants, as long as a web developer will understand what it means.  Which is a pretty significant constraint, actually.

It's possible that "'get foo' called on an object" is good enough for that purpose, and does make for a much simpler change than the +4 approach.  In that case, we can get rid of MSG_SETTER_THIS_DOES_NOT_IMPLEMENT_INTERFACE and MSG_GETTER_THIS_DOES_NOT_IMPLEMENT_INTERFACE altogether.

> as the testcase uses Object.getOwnPropertyDescriptor to get raw function of the getter property

That's just for readability of the testcase.  The way this would come up in practice is if someone sets the __proto__ of some random object to a DOM prototype or DOM object and then tries to do the get.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Remove getter/setter variant for ThrowInvalidThis message.

Prepared 2 patches for each plan.

This is based on previous parts 1-5 (of course part 5 and this patch will be merged into part 2 later).

This removes "getter"/"setter" from error message.
I feel now GetInvalidThisErrorFor* functions are not needed, as there are only 2 variants, "security error or not", so passing that boolean value as an argument would be simpler.

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: Part 5 plan B: Skip accessor name prefix in ThrowInvalidThis.

Another plan.
now I think attachment 8704347 [details] [diff] [review] is better than this, so, attaching just in case.

this is based on previous parts 1-4, to keep the message same as before.
this changes ThrowInvalidThis to receive the type of the function (method/getter/setter), and the "security error or not" bool.
then, if it's getter or setter, skips first 4 chars.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8704347 [details] [diff] [review]
Part 6: Remove getter/setter variant for ThrowInvalidThis message.

Hmm, given that bz feels much stronger about this stuff than I do, he should probably review this. (but he is not accepting review requests atm)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8704347 [details] [diff] [review]
Part 6: Remove getter/setter variant for ThrowInvalidThis message.

r=me, since I just read this over an hour or so ago....

Comment 32

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

just noticed that Part 6 don't have to be folded to Part 2, as folding Part 5 solves the bisect issue.
Will renumber Part 6 to 5, for simplicity.

Comment 33

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b51771070a8993305e2e73a31bc3e82c32de3ae4
Bug 1180290 - Part 1: Add prefix parameter to IdToFunctionName. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/44725d3301db2d4371e087076aa95801622cf8b2
Bug 1180290 - Part 2: Handle prefix in DefinePropertyById. r=till,smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd562b7c8512ecfacb413bd901b0c1ae89a029b4
Bug 1180290 - Part 3: Use canonical name in native getter. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/a59c19e0b14d38b02ae4c12e5e39659756e2d586
Bug 1180290 - Part 4: Add tests for builtin getter name. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bc312c15f1fd17866f6f177dfedb7e2a96880b0
Bug 1180290 - Part 5: Remove getter/setter variant for ThrowInvalidThis message. r=bz

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b51771070a89
https://hg.mozilla.org/mozilla-central/rev/44725d3301db
https://hg.mozilla.org/mozilla-central/rev/dd562b7c8512
https://hg.mozilla.org/mozilla-central/rev/a59c19e0b14d
https://hg.mozilla.org/mozilla-central/rev/7bc312c15f1f