Closed Bug 1075059 Opened 10 years ago Closed 10 years ago

non-enumerable Array.prototype.contains is not web-compatible (breaks jsfiddle.net)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox35 + fixed

People

(Reporter: bzbarsky, Assigned: till)

References

Details

(4 keywords)

Attachments

(2 files)

[Tracking Requested - why for this release]: Web compat regression

STEPS TO REPRODUCE:

1)  Load http://jsfiddle.net/
2)  Type something in the "HTML" area.
3)  Click the "Save" button near the top.

ACTUAL RESULTS: Goes to http://jsfiddle.net/#save and doesn't actually save anything.  Error console during initial page load shows:

  TypeError: this.contains is not a function moo-clientcide-1.3.js:850

EXPECTED RESULTS: Saving works, no error on load.

ANALYSIS:

The issue is in the script at http://jsfiddle.net/js/moo-clientcide-1.3.js [yes, "cide"]

The relevant bit looks like this: 

		if (!this.contains(item)) this.push(item);

where "this" is an Elements instance.

Earlier on, Elements is set up like so:

  Array.forEachMethod(function(method, name){
    Elements.implement(name, method);
  });

where forEachMethod is a function that looks like this:

  object.forEachMethod = function(fn){
    if (!methodsEnumerable) for (var i = 0, l = methods.length; i < l; i++){
      fn.call(prototype, prototype[methods[i]], methods[i]);
    }
    for (var key in prototype) fn.call(prototype, prototype[key], key)
  };

here "prototype" is object.prototype, which in this case means Array.prototype.

The script also has a polyfill for contains() like so:

  Array.implement({
...
    contains: function(item, from){
      return this.indexOf(item, from) != -1;
    },

which ends up assigning to Array.prototype.contains.  But, and this is key, assignment does not change enumerability, so it clobbers the default Array.prototype.contains but the result is still not enumerable.

So putting the pieces together: before bug 1069063, the Array.implement() call would set up an enumerable "contains" property on Array.prototype, then the forEachMethod() call would end up enumerating and and calling back to the callback, which would call Elements.implement and stick a "contains" property on Elements.prototype.

In today's build, Array.implement() leaves "contains" as non-enumerable, forEachMethod doesn't see it during its for/in enumeration, Elements.prototype ends up without a "contains" property, and then doing this.contains() on an Elements instance throws.

I've verified that either removing our "contains" implementation or adding JSPROP_ENUMERATE to it make the site work again.
Summary: non-enumerable Array.prototype.contains is not web-compatible → non-enumerable Array.prototype.contains is not web-compatible (breaks jsfiddle.net)
Note that this makes using nightly for Firefox development a bit of a pain, since it's very common to have to deal with jsfiddle for user-submitted testcases, testing things in other browsers via browserstack, etc.
Right, but that doesn't help us until everyone deploys the fixed version.
I see no choice but to back this out.
Assignee: nobody → jorendorff
I've just patched the MooTools in JSFiddle's clientcide file.
Please check if it's fine now
Thanks Piotr, I can confirm that it works. The fast turnaround here is very much appreciated!

I still don't see a chance for us to keep this in for now, though: too many sites are affected. :(
Attachment #8498084 - Flags: review?(jorendorff)
Assignee: jorendorff → till
Status: NEW → ASSIGNED
Attachment #8498085 - Flags: review?(jorendorff)
Attachment #8498084 - Flags: review?(jorendorff) → review+
Attachment #8498085 - Flags: review?(jorendorff) → review+
Backout try-servering here (with a backout of bug 1072889 included):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=36adae95adf7
Keywords: dev-doc-needed
Keywords: site-compat
OS: Mac OS X → All
Hardware: x86 → All
Till, is this happening?
Flags: needinfo?(till)
Huh, this has long since landed. Looks like I forgot to post the inbound links here, and sheriffs didn't close it when landing on central. Weird.

https://hg.mozilla.org/mozilla-central/rev/3c341e9e6639
https://hg.mozilla.org/mozilla-central/rev/7009237d5e47
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: needinfo?(till)
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Version: unspecified → Trunk
If I had to guess, mcMerge tripped over the "backout" in your commit messages.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: