909602 - Array.prototype.pop removes all dense elements above length even for non-Array objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

y = (function() {
    x = arguments.callee.caller.arguments
})
z = new String;
(function() {
    z.valueOf = (function() {
        return function() {
            y()
        }
    })()
})();
+ z;
Array.prototype.forEach.call(x, (function() {}))
for (var m = 0; m < 99; m++) {
    Array.prototype.push.call(x, "")
}
x.length = 3
Array.prototype.pop.call(x)
print(x[11])

on js debug (64-bit threadsafe deterministic) shell on m-c changeset 17143a9a0d83 shows "undefined" with --ion-eager, but shows only a blank line without any CLI arguments.

Setting needinfo for jandem as this blocks differential testing.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This has probably existed since a long time ago.

Comment 2

[Jan de Mooij [:jandem]]

I think this is an arguments object bug. If I run the testcase below, I get "undefined" with SpiderMonkey and "3" with d8.

The weird part is, if I change String("length") to just "length", SpiderMonkey also prints "3". It seems to be related to the GetLengthProperty function in Interpreter-inl.h: we use that fast path for arguments.length but not arguments[String("length")], and don't call the resolve hook when we use that path.

Waldo, would you mind taking a look? I think it's caused by args_resolve, ArgGetter or ArgSetter, but I'm not too familiar with the arguments object code.

function foo() {
    return arguments;
}

x = foo();
x[String("length")];

for (var m = 0; m < 5; m++)
    Array.prototype.push.call(x, m);

x.length = 3;
Array.prototype.pop.call(x);

print(x[3]);

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

The arguments object has nothing to do with this, unfortunately.  The problem is Array.prototype.pop is dumb, and it thinks that popping the element at [length - 1] means it can trim dense initialized length.  But it can only do that if there are no elements above that one, which is only the case if the "length" property is an authoritative bound on the object's elements, which is only true for arrays.  Dumb.  How did I ever not notice this when merging the two implementations of [].pop (the one containing this test was isArray()-guarded, so correct) in bug 858381?  :-(

Not that I expect too much code is breaking because of this, and it's not breaking dangerously.  But this is on all branches, so we may want to take this on branches to fix this stupid behavior bug.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 805757 [details] [diff] [review]
Patch

Review of attachment 805757 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

::: js/src/tests/ecma_5/Array/pop-nonarray-higher-elements.js
@@ +20,5 @@
> +// non-zero length
> +var obj1 = { length: 2, 3: 42 };
> +assertEq(Array.prototype.pop.call(obj1), undefined);
> +assertEq(3 in obj1, true);
> +assertEq(obj1[3], 42);

Can you rewrite these tests to something like:

function test1() {
  var obj1 = { length: 2, 3: 42 };
  assertEq(Array.prototype.pop.call(obj1), undefined);
  assertEq(3 in obj1, true);
  assertEq(obj1[3], 42);
}

for (var i = 0; i < 10; i++)
  test1();

This should put more stress on the JIT, which inlines Array.pop in some cases.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> This should put more stress on the JIT, which inlines Array.pop in some
> cases.

Maybe also add a jit-test instead of jstest? Tinderbox runs jit-tests with --ion-eager, many jstests only run in the interpreter or baseline JIT...

Comment 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bce276f9c2d

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9bce276f9c2d