Closed Bug 1420101 Opened 6 years ago Closed 6 years ago

Array.prototype.values() does not exist

Categories

(Core :: JavaScript Engine, defect, P5)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: neil.kronlage, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 2 obsolete files)

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
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
We can try this again, but I would definitely like to coordinate with v8 / chrome.
Flags: needinfo?(evilpies)
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)
Already got a reply via twitter https://twitter.com/_gsathya/status/937353960242814976
Flags: needinfo?(adamk)
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)
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)
Blink tries it again
Intent to Ship: Array.prototype.values

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yX4U__z67xo
Assignee: nobody → evilpies
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)
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+
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
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/e1ca344ca6b5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Blocks: 1469540
You need to log in before you can comment on or make changes to this bug.