1313891 - Array.prototype.push doesn't invoke prototype setters

Status: RESOLVED DUPLICATE of bug 1282104

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase:

      Object.defineProperty(Object.prototype, '0', { set: function(val){console.log('GOT '+val)} });
      [].push('FOO')

this should log, but does not.

I think the reason it does not is that we correctly land inside SetArrayElement as called from InitArrayElements, because we've been checking ObjectMayHaveExtraIndexedProperties(obj).  But inside SetArrayElement we do NOT check ObjectMayHaveExtraIndexedProperties(obj), so end up doing the fast-path set when we should not.  This buggy fast path predates the changes made in bug 1163091, fwiw; it seems to date back quite a ways.

I would expect other callers of InitArrayElements are likewise affected; that seems to be unshift() and sort().  Other callers of SetArrayElement might also be affected; that would be reverse(), shift(), splice(), unshift(), sort() (these last two both call InitArrayElements() and call SetArrayElement() directly).

Jeff, do you know who would be the right person to look into this?

Comment 1

[ljharb]

Note that the same should be true of
```
Object.defineProperty(Array.prototype, '0', { set: function(val){console.log('GOT '+val)} });
[].push('FOO');
```

Comment 2

[Jann Horn]

See also: https://esdiscuss.org/topic/change-behavior-of-array-prototype-push

I think that the specified behavior is stupid from a security standpoint (unnecessarily makes [].push on a normal, non-sparse array leak numeric properties to the prototype) and from a performance standpoint (how are you supposed to optimize array accesses if every push has to look at the prototype chain?), and since nobody has noticed Firefox' non-compliance, it would likely be safe to just change the spec to some sane behavior.

Comment 3

[ljharb]

I don't see what security has to do with this - I can just as easily replace Array.prototype.push.

Conversely, every other engine seems to have optimized these array operations, so I don't see why Firefox can't.

Comment 5

[Jann Horn]

> I don't see what security has to do with this - I can just as easily replace Array.prototype.push.

But it's pretty simple (although annoying) to work around that issue by using bound copies of the Array.prototype members if trusted code can run before any untrusted code is executed:

const ARRAY_PUSH = Function.prototype.call.bind([].push);
const ARRAY_POP = Function.prototype.call.bind([].pop);

var arr = [];
ARRAY_PUSH(arr, 123);
ARRAY_POP(arr);

Well, I guess the alternative is to just make a new constructor for safe pseudo-arrays, copy all the methods from the array prototype to it and freeze it and its prototype.

> Conversely, every other engine seems to have optimized these array operations

Yeah, I guess I don't know much about performance.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> how are you supposed to optimize array accesses 

In practice, this can be done as follows:

1)  You have a reasonably fast way to check for indexed things on your prototypes (and yourself, in case someone defined an accessor property on you).

2)  In the JIT, you output fast paths when there are no such indexed things, and invalidate jitcode if someone defines indexed properties on objects that are prototypes of something else.

Comment 7

[Jeff Walden [:Waldo]]

This is pretty ancient, and I'd say pretty much anyone who works on the VM is a reasonable person to fix it.  Just a matter of finding the time -- and moreover, justifying working on this particular problem as opposed to some other.  That's the tricky part.  :-\

Comment 8

[ljharb]

I'll add it to the kangax compat table tonight - that should help motivate the fix ;-)

Comment 9

[Jeff Walden [:Waldo]]

(In reply to ljharb from comment #8)
> I'll add it to the kangax compat table tonight - that should help motivate
> the fix ;-)

You can do whatever you want, of course.  But I want to caution you about this.

The value of "scoreboards" is that they track what functionality is broadly correct.  Developers can use the results to conclude, "These features (at least) basically work."

But take evenhanded, feature-focused scoreboards and weaponize them by focusing on specific, esoteric browser lapses in implemented features, and you fundamentally change what you're doing.  You're not focusing on what people can use, you're playing a "gotcha" game.  Until implementations are mechanically generated by supercompiler from a full denotational semantics, there's no end to that road.  If this one weird case is worth targeting, why not that other case targeting another engine?  What neutral justification will prevent scope creep in your tests?

This is *especially* true for this particular bug.  Do you have any evidence that installing element setters on Array.prototype (or Object.prototype) (or on an object inserted into Array.prototype's [[Prototype]] chain) is an operation web developers do frequently?  Do you have any evidence that it's something they'd *want* to do?  Do you have any evidence that they're enduring developer pain because of this error?

Any site like this, short of a fully comprehensive attempt like test262 (whose scope you surely can't duplicate), must determine its scope.  The kangax compat tables perform a reasonably useful service, when they test the general contours of features, whose contours developers will actually use.  The value proposition -- and not to put too fine a point on it, how much vendors are willing to care about your site -- is much worse when you test things that are broken, just because they're pet peeves.  (Consider Acid3, and consider Ian Hickson's later regrets about what it tested, including a number of browser features since *removed* from browsers, or that are being removed.)

Comment 10

[ljharb]

In that specific case, it's probably not common (although I do it in my shim tests to make sure methods follow the spec).

What happens with Array subclassing? In this code:
```
class NoPushArray extends Array {
  constructor(...args) {
    super(...args);
    Object.defineProperty(this, this.length, { set(v) { throw 'nope' } });
  }
}

var arr = new NoPushArray(3);
Array.prototype.push.call(arr, true); // does this throw?
```
If so, and this bug is a specific issue with Array.prototype or Object.prototype only, then clearly it's not high impact - but if it means it could spill out into other areas of the spec, then it could have more troubling consequences.

Putting these sorts of tiny little bugs in the compat table has motivated getting them fixed before - you implied the problem was justification, and my half-joking comment was meant to make that justification perhaps more palatable to some on the team.

Eventually a better form of the compat table would be a literal webview on test262 results - which ideally indeed would be precise and test every observable facet of the spec. We're not quite there yet, of course :-)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Array.prototype.push.call(arr, true); // does this throw?

It does not, because when you did the defineProperty() call you defined the property at index 3 and hence the length became 4.  The push() is trying to add a property at index 4, which is not a problem.  This testcase doesn't throw in any other browser, or per spec.  ;)

> If so, and this bug is a specific issue with Array.prototype or Object.prototype only

SetArrayElement takes the slow path if there are any own accessor properties defined.  So this bug is a specific issue with indexed props on the proto chain of the array object.

This is not limited to Array.prototype and Object.prototype, in the sense that this testcase:

  class NoPushArray extends Array {
    constructor(...args) {
      super(...args);
      Object.defineProperty(NoPushArray.prototype, this.length, { set: function(v) { throw 'nope' } });
    }
  }

  var arr = new NoPushArray(3);
  Array.prototype.push.call(arr, true);

does not throw when it should per spec.  But that would be a pretty odd thing to do too.  ;)

Comment 12

[Jeff Walden [:Waldo]]

(In reply to ljharb from comment #10)
> my half-joking comment was meant to make that justification perhaps more
> palatable to some on the team.

Understood as such, but recognize that implicit reports of unimportant-to-developers minutiae, dilute the effectiveness of reports of issues that meaningingfully impede web developers writing code on the web.  We prioritize things partly because of site prominence, but also to large degree because the bugs tend to be functionality people would use.  Start indulging mostly-irrelevancies, and your signal-to-noise ratio for "things that are important" versus "things we shouldn't work on quite yet" will drop.

> Eventually a better form of the compat table would be a literal webview on
> test262 results - which ideally indeed would be precise and test every
> observable facet of the spec.

This is a fine idea, but I caution you against fully replacing the existing tables.  The existing tables seem useful to web developers, in a way that tests of spec-nitpickeries would not be.