Closed Bug 1369337 Opened 4 years ago Closed 4 years ago

defineProperty not working correctly for length/@@iterator properties of arguments object

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 1 obsolete file)

function f() {
    // Also reproducible for the unmapped arguments object.
    var pk = "length";
    Object.defineProperty(arguments, pk, { });
    print(JSON.stringify(Object.getOwnPropertyDescriptor(arguments, pk)));
}
f();

Expected: Prints {"value":0,"writable":true,"enumerable":false,"configurable":true}
Actual: Prints {"writable":false,"enumerable":false,"configurable":false}


NativeDefineProperty() marks the "length" property as overridden, NativeLookupOwnProperty->LookupOwnPropertyInline doesn't find the property and will then call the resolve hook (MappedArgumentsObject::obj_resolve). The resolve hook won't add the lazy property, because markLengthOverridden() was already called in the initial NativeDefineProperty call.
Attached patch bug1369337.patch (obsolete) — Splinter Review
Forcibly create the length and @@iterator properties when calling NativeDefineProperty. We may be to only use the approach with calling markIteratorOverridden/markLengthOverridden, but then we'd need to move the calls after the last GetExistingPropertyValue() call in NativeDefineProperty(), so I think it's easier to understand if we simply directly create the length/@@iterator property before redefining it. 

Simply calling NativeDefineProperty() in the new reifyLength/reifyIterator methods seems to work, so we may later want to update MappedArgSetter/UnmappedArgSetter to use the same approach, so we don't need to delete and then redefine properties in MappedArgSetter/UnmappedArgSetter. WDYT?
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8873789 - Flags: review?(evilpies)
Comment on attachment 8873789 [details] [diff] [review]
bug1369337.patch

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

Great find! Everything would be much easier if this was closer aligned to the spec and we didn't have resolve at all.

Your observation about delete + define seem correct as well. obj_delProperty just calls markLengthOverridden/markIteratorOverridden like your reify functions.
Attachment #8873789 - Flags: review?(evilpies) → review+
Attached patch bug1369337.patchSplinter Review
Updated to apply cleanly on inbound, carrying r+.
Attachment #8873789 - Attachment is obsolete: true
Attachment #8874742 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d976b9bf41
Forcibly create length and @@iterator properties for arguments when redefining. r=evilpie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0d976b9bf41
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.