Closed Bug 909602 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: gkw, Assigned: Waldo)

References

Details

(Keywords: testcase)

Attachments

(1 file)

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.
Flags: needinfo?(jdemooij)
This has probably existed since a long time ago.
Keywords: regression
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]);
Flags: needinfo?(jdemooij) → needinfo?(jwalden+bmo)
Attached patch PatchSplinter Review
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #805757 - Flags: review?(bhackett1024)
Flags: needinfo?(jwalden+bmo)
Blocks: 858381
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: Differential Testing: Different output message involving arguments → Array.prototype.pop removes all dense elements above length even for non-Array objects
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.
Attachment #805757 - Flags: review?(bhackett1024) → review+
(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...
https://hg.mozilla.org/mozilla-central/rev/9bce276f9c2d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.