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

RESOLVED WONTFIX

Status

()

defect
--
major
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: BenB, Unassigned)

Tracking

({dev-doc-complete, regression, site-compat})

Trunk
Points:
---

Firefox Tracking Flags

(firefox28- affected, firefox29- affected)

Details

(URL)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
It works in Chromium 31.
Ben, are you willing to hunt down a regression range for this?
Flags: needinfo?(ben.bucksch)
(Reporter)

Comment 3

5 years ago
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 .
Severity: normal → major
Flags: needinfo?(ben.bucksch)
OS: Linux → All
Hardware: x86_64 → All
Summary: [regression] "Node 'undefined' is not in graph" in dagre-d3 → [regression] Object.keys() breaks web apps, e.g. dagre-d3
Version: unspecified → Trunk
(Reporter)

Comment 4

5 years ago
The dagre dev says he'll work around it, but this is likely to affect other web apps/libs, too.
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.
Blocks: 894658
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.
Another relevant question: what is dagre-d3 and how widely is it used?
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.
(Reporter)

Comment 9

5 years ago
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.
(Reporter)

Comment 11

5 years ago
(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.
> 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...
We will be tracking this thanks!
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
Summary: [regression] Object.keys() breaks web apps, e.g. dagre-d3 → Array.prototype.keys() breaks web apps, e.g. dagre-d3
Given the status update in Comment 14, not tracking.
You need to log in before you can comment on or make changes to this bug.