Bug 75788

Summary: Array.prototype.pop should throw if property is not configurable
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: metaweta
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   

Description Gavin Barraclough 2012-01-07 22:51:22 PST
See ES5.1 15.4.4.6 step 5c.
Comment 1 metaweta 2012-06-25 11:47:12 PDT
This is far worse than merely not throwing, which is acceptable outside of strict mode.  The current behavior violates the frozenness invariant:

var a = [1,2,3];
Object.freeze(a);
a.pop(); // returns 3
a; // [1,2]
Object.isFrozen(a); // true

It's a security bug and should be escalated.
Comment 2 Gavin Barraclough 2012-06-29 18:47:20 PDT
I don't think this bug still exists in ToT:

var a = [1,2,3];
Object.freeze(a);
a.pop(); // Exception: TypeError: Attempted to assign to readonly property.
a; // 1,2,3
Object.isFrozen(a); // true

Technically I think we now implement the spec correctly, though we do give a misleading error message (which is outside of the spec).

We should be throwing a type error at the point pop() tries to delete the old property,
    15.4.4.6 step 5.c. -> 8.12.7 step 4
Instead we're silently ignoring the failed delete, but we'll throw a type error when pop tries to change the array's length.
    15.4.4.6 step 5.d. -> 15.4.5.1 step 3.g.

We will generate the right error message if the last property in the array is not configurable but length is writable:

var a = [1,2,3];
Object.defineProperty(a, 2, {configurable:false})
a.pop() // Exception: TypeError: Unable to delete property.

This works because the first attempted delete will (erroneously) silently fail, but when the length property is changed we'll attempt to remove all properties beyond the new array bounds, and this will throw as expected with the correct error message.
    15.4.4.6 step 5.d. -> 15.4.5.1 step 3.l.iii.4.

I'll put up a patch to make the first delete catch the error, as it should, so the TypeError has the right error message.
Comment 3 Gavin Barraclough 2012-07-02 12:26:15 PDT
Fixed in r121700