Array.prototype.values() does not exist

RESOLVED FIXED in Firefox 60

Status

()

defect
P5
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: neil.kronlage, Assigned: evilpie)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

57 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171102181127

Steps to reproduce:

[1, 2].values()


Actual results:

[1, 2].values()
TypeError: [1, 2].values is not a function
[Learn More]


Expected results:

Returned an array iterator
(Reporter)

Comment 1

2 years ago
This appears to have been disabled due to https://bugzilla.mozilla.org/show_bug.cgi?id=1299593

I'd like to use this feature, but couldn't find any bugs tracking re-enabling it so I've opened this one.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
See Also: → 1299593
Hmm. Well, it's been another 12 months. According to MDN, Edge and Safari are shipping Array#values now.

Then again, we've enabled this (at least) twice and been burned each time. What do you think, Tom?
Flags: needinfo?(evilpies)
In the mean time, Neil, you can work around it using this:

    if (!("values" in Array.prototype)) {
      Object.defineProperty(Array.prototype, "values",
        Object.getOwnPropertyDescriptor(Array.prototype, Symbol.iterator));
    }

(I think that'll work in the other browsers, too, not just Firefox.)
Priority: -- → P5
(Assignee)

Comment 4

2 years ago
We can try this again, but I would definitely like to coordinate with v8 / chrome.
Flags: needinfo?(evilpies)
(Assignee)

Updated

2 years ago
Blocks: es6
(In reply to Tom Schuster [:evilpie] from comment #4)
> We can try this again, but I would definitely like to coordinate with v8 /
> chrome.

Adam, do you have any plans around this?
Flags: needinfo?(adamk)
(Assignee)

Comment 6

2 years ago
Already got a reply via twitter https://twitter.com/_gsathya/status/937353960242814976
Flags: needinfo?(adamk)

Comment 7

2 years ago
If Sathya's up for doing the V8/Chrome-side work, I'm supportive of a coordinated effort (I've assigned the V8 tracking issue, https://bugs.chromium.org/p/v8/issues/detail?id=4247, to him).

What did you have in mind for coordination, though? The problems we've run into in the past were due to enterprise software stuck at some old version. Is your thought to just tell bug-filers "please update your software, the standard changed"? I'm worried that that isn't a very satisfying answer. Or is the idea to see if the ecosystem has moved enough that we won't get reports of breakage?
Flags: needinfo?(evilpies)
(Assignee)

Comment 8

2 years ago
I think landing at the same time would be useful to find breakage better. And if the amount of breakage is acceptable, being broken in all browsers might be a motivator *hint*
Flags: needinfo?(evilpies)

Comment 9

a year ago
Blink tries it again
Intent to Ship: Array.prototype.values

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yX4U__z67xo
(Assignee)

Updated

a year ago
Assignee: nobody → evilpies
(Assignee)

Comment 11

a year ago
I added a new preference javascript.options.disable_array_prototype_values. Instead of conditionally adding the property to Array.prototype, we just delete it when we don't want it. This should also work better with JSXrays.
Attachment #8947916 - Flags: feedback?(jdemooij)

Updated

a year ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8947916 [details] [diff] [review]
Add a pref to disable Array.prototype.values

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

Makes sense.

Maybe also add the pref to the lists in dom/ipc/ContentPrefs.cpp and modules/libpref/init/all.js.

What about workers? Maybe we are less worried about problems there but it might be nice to set a global flag so we get consistent behavior.
Attachment #8947916 - Flags: feedback?(jdemooij) → feedback+
(Assignee)

Comment 13

a year ago
Now that I added the pref to all.js, I also changed it to be default enabled, which matches most of our other preferences.
Attachment #8947916 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8948477 - Flags: review?(jdemooij)
Attachment #8948477 - Flags: review?(bzbarsky)
Comment on attachment 8948477 [details] [diff] [review]
Add default enabled pref for Array.prototype.values

Does DeleteProperty not badly deoptimize accesses on the object anymore?

r=me if so.
Attachment #8948477 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> Does DeleteProperty not badly deoptimize accesses on the object anymore?

DeleteProperty will convert to dictionary mode if the property is not the last one. Maybe we can make sure it's the last property added to Array.prototype? Dictionary mode is probably not too bad here, but if we can easily avoid it it would be nice.

Do you see any perf differences on shell benchmarks?
Flags: needinfo?(evilpies)
(Assignee)

Comment 16

a year ago
Changed the code to only conditionally add "values" to the Array.prototype object. It's not perfect, because we can't tell the different between an Array instance and Array.prototype, but we don't call JS_DefineFunctions with some arbitrary object and functionspec combination.

I also removed the nightly restriction on "values" from test_xrayToJS.xul.
Attachment #8948477 - Attachment is obsolete: true
Attachment #8948477 - Flags: review?(jdemooij)
Flags: needinfo?(evilpies)
Attachment #8949071 - Flags: review?(jdemooij)
What if we move [1] before [2] to ensure "constructor" isn't added as the last property? It doesn't look like we have any strict rules when to call LinkConstructorAndPrototype when comparing other built-ins, so hopefully reordering the "constructor" property doesn't break other code... 

[1] https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/js/src/vm/GlobalObject.cpp#251-253
[2] https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/js/src/vm/GlobalObject.cpp#230
Comment on attachment 8949071 [details] [diff] [review]
v2 - Add default enabled pref for Array.prototype.values

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

::: js/src/jsobj.cpp
@@ +2967,4 @@
>      if (!PropertySpecNameToId(cx, fs->name, &id))
>          return false;
>  
> +    if (StandardProtoKeyOrNull(obj) == JSProto_Array && id == NameToId(cx->names().values)) {

This is fine. I was going to say another option is to strcmp the self-hosted function name, but that would break @@iterator :)
Attachment #8949071 - Flags: review?(jdemooij) → review+

Comment 19

a year ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ca344ca6b5
Add default enabled pref for Array.prototype.values. r=jandem,bz

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e1ca344ca6b5
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1453867
(Assignee)

Updated

11 months ago
Blocks: 1469540
You need to log in before you can comment on or make changes to this bug.