Array.prototype.{slice,splice} should use [[DefineOwnProperty]] to generate return value

Array.prototype.{slice,splice} should use [[DefineOwnProperty]] to generate return value

BUG=chromium:423633
LOG=N
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24784

[adamk, 2014-10-15 21:29:36]

adamk@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[adamk, 2014-10-20 23:02:39]

Ping?

[Michael Starzinger, 2014-10-21 08:07:48]

LGTM (rubber-stamped).

https://codereview.chromium.org/649063003/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/649063003/diff/1/src/array.js#newcode288
src/array.js:288: if (!IS_UNDEFINED(current) || index in array) {
I know this is not part of your change, but the ordering of the [[Get]] and
[[Has]] calls in this predicate is also off. It first loads the property and
then (if the value is undefined) checks presence of the property. The following
is a repro:

var a = [1, 2, 3];
Object.defineProperty(a, '1', {
  get: function() { delete this[1]; return undefined; },
  configurable: true
});
var s = a.slice(1);
assertTrue('0' in s);

[adamk, 2014-10-21 17:36:58]

On 2014/10/21 at 08:07:48, mstarzinger wrote:
> LGTM (rubber-stamped).
> 
> https://codereview.chromium.org/649063003/diff/1/src/array.js
> File src/array.js (right):
> 
> https://codereview.chromium.org/649063003/diff/1/src/array.js#newcode288
> src/array.js:288: if (!IS_UNDEFINED(current) || index in array) {
> I know this is not part of your change, but the ordering of the [[Get]] and
[[Has]] calls in this predicate is also off. It first loads the property and
then (if the value is undefined) checks presence of the property. The following
is a repro:
> 
> var a = [1, 2, 3];
> Object.defineProperty(a, '1', {
>   get: function() { delete this[1]; return undefined; },
>   configurable: true
> });
> var s = a.slice(1);
> assertTrue('0' in s);

I am completely unsurprised there are more bugs here. This looks like the same
old bug we used to have in Array.prototype.forEach (for speed!). But I'd like to
leave it separately due to the performance regression I expect with it.

[adamk, 2014-10-21 17:46:48]

Committed patchset #1 (id:1) manually as 24784 (presubmit successful).