958095 - Array.prototype.keys() breaks web apps, e.g. dagre-d3

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Ben Bucksch (:BenB)]

I can't tell whether this is a bug in Firefox, dagre or my code.

URL: http://bible.bucksch.org/free/#detail=@I0025@

Reproduction:
1. Go to above URL in Firefox 24
2. Go to above URL in Firefox nightly from 2013-11-18 or 2014-01-07

Actual result:
1. You see a genealogy graph
2. You see an error "Node 'undefined' is not in graph" and all graph nodes are in the upper left corner (no position/translation).

Expected result:
2. like 1.


Page code:
I reduced and changed the page code (for testing, not on the server) to ensure that I never pass null as ID (first parameter to addNode()/addEdge(), but the error still appears.

Dagre:
I also filed https://github.com/cpettitt/dagre-d3/issues/19

Firefox:
Given that this worked in earlier Firefox releases, and JS web page code normally shouldn't break with new Firefox releases, I am filing this as Firefox bug.

I can't tell whether this is a bug in Firefox, dagre or my code. Sorry if this is not a Firefox bug.

Comment 1

[Ben Bucksch (:BenB)]

It works in Chromium 31.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ben, are you willing to hunt down a regression range for this?

Comment 3

[Ben Bucksch (:BenB)]

According to the dagre dev, the problem is Object.keys():
'keys' in []
gives true in Firefox nightly (where I see the bug) and false in Firefox 24 (where I do not see the bug).
|keys| is not a reserved word in JavaScript, so normally apps are free to use it and it shouldn't be used by the language. That's ECMAScript 5: http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.14 .

Comment 4

[Ben Bucksch (:BenB)]

The dagre dev says he'll work around it, but this is likely to affect other web apps/libs, too.

Comment 5

[Tom S [:evilpie]]

Array.prototype.keys was implemented in Bug 894658. It's specified by ES6: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.keys.

Reserved words are kind of irrelevant here. Not every function is a reserved word, because that wouldn't make any sense.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ben, this isn't about things being reserved.  "indexOf" is no reserved either, yet "'indexof' in []" tests true, for the same reason that your 'keys' example does.

Also, nothing prevents an app from assigning [].keys or anything; it just sounds like this app assumed the property is never set unless they set it, which is not a good assumption if new features are ever added to the language.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Another relevant question: what is dagre-d3 and how widely is it used?

Comment 8

[Boris Zbarsky [:bzbarsky]]

And what exactly are they doing with "keys" that gets broken?  That might be relevant for deciding how to change things if they get changed.

Comment 9

[Ben Bucksch (:BenB)]

https://github.com/cpettitt/dagre-d3  . I don't think it's widely used.

I just worry about the fact that things work in Firefox N and fail in Firefox N+1, because I think there might be more out there breaking than this. If you guys think that's acceptable, I'll shut up and let you close this as WONTFIX.

Comment 10

[Ben Bucksch (:BenB)]

fix for dagre: https://github.com/cpettitt/dagre-d3/issues/19#issuecomment-31952723

Comment 11

[Ben Bucksch (:BenB)]

(Copying here for reference:) When I change dagre-d3 line
keys = cur.keys ? cur.keys() : cur;
to
keys = cur instanceof Set ? cur.keys() : cur;
it works in Firefox nightly.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I just worry about the fact that things work in Firefox N and fail in Firefox N+1

Sure.  That's a legitimate worry, but taken to the extreme it means never adding any new functionality to any ES objects (because it's always possible to write code that depends on the functionality not being there, as we saw in this case), which is not great either.  That's where we have to try to figure out the probably extent of the breakage...

Comment 13

[Benjamin Kerensa [:bkerensa]]

We will be tracking this thanks!

Comment 14

[Jason Orendorff [:jorendorff]]

Comment 12 is right. We just have to keep watching this.

Like any new feature, if it turns out a lot of existing web content depends on the feature *not* being there, we will have to shake our heads and try a different approach.

This bustage is not a great sign, but it may be a random isolated thing. WONTFIX for now. We'll see.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Given the status update in Comment 14, not tracking.

Comment 16

[Kohei Yoshino [:kohei]]

Added this to the site compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/28/Site_Compatibility#JavaScript

(It's almost a copy of https://developer.mozilla.org/en-US/Firefox/Releases/25/Site_Compatibility#JavaScript)