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

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: leonardo.balter, Assigned: arai)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(7 attachments)

Reporter

Description

4 years ago
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.
Reporter

Updated

4 years ago
Summary: Map.prototype.size accessor get function name should return `get size` → Map and Set prototype.size accessor get function name should return `get size`
Reporter

Comment 1

4 years ago
Updating this issue to extend it to Set.prototype.size, the same issue is present there.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Map and Set prototype.size accessor get function name should return `get size` → Builtin accessor functions must be named 'get foo' or 'set foo' (where 'foo' is the name of the property used to access the accessor)
(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.
Reporter

Comment 4

4 years ago
> 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__
Should we also have this "get"/"set" thing on WebIDL getters setters?  Seems like we should.
Reporter

Comment 6

4 years ago
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."
Here's a question.  What do class getter/setter names default to in ES6?
Reporter

Comment 8

4 years ago
> 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.
OK.  So once again we have to decide whether web idl should act more like builtins or more like classes, yay.
Reporter

Comment 10

4 years ago
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.
Assignee

Comment 11

4 years ago
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' ?
Assignee

Comment 12

4 years ago
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?)
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".
Assignee

Comment 14

4 years ago
Thank you :D
I'll go with current patch that changes all names for getter/setter.
Will post it after some more test.
Thanks for testing, Boris.  I've made the change in the spec:

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

Comment 16

4 years ago
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.
Assignee: nobody → arai.unmht
Attachment #8703886 - Flags: review?(till)
Assignee

Comment 17

4 years ago
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.
Attachment #8703898 - Flags: review?(till)
Assignee

Comment 18

4 years ago
Part 3 fixes getters for ArrayBuffer, DataView, and TypedArray.  They calls NewNativeFunction directly, so added Atomize with "get FOO" string literal.
Attachment #8703899 - Flags: review?(till)
Assignee

Comment 19

4 years ago
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.
Attachment #8703900 - Flags: review?(till)
Assignee

Comment 20

4 years ago
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
Attachment #8703903 - Flags: review?(bugs)
Attachment #8703903 - Flags: review?(bugs) → review+
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 "=".
Attachment #8703886 - Flags: review?(till) → review+
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.
Attachment #8703898 - Flags: review?(till) → review+
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.
Attachment #8703899 - Flags: review?(till) → review+
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.
Attachment #8703900 - Flags: review?(till) → review+
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...
Flags: needinfo?(arai.unmht)
Assignee

Comment 26

4 years ago
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.
Flags: needinfo?(arai.unmht)
> 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.
Assignee

Comment 28

4 years ago
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.
Attachment #8704347 - Flags: review?(bugs)
Assignee

Comment 29

4 years ago
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 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)
Attachment #8704347 - Flags: review?(bugs)
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....
Attachment #8704347 - Flags: review+
Assignee

Comment 32

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.