Skip to content
Permalink
Browse files
Non-enumerable property fails to shadow inherited enumerable property…
… from for-in

https://bugs.webkit.org/show_bug.cgi?id=38970

Reviewed by Keith Miller.

JSTests:

* stress/arguments-bizarre-behaviour-disable-enumerability.js:
* stress/for-in-redefine-enumerable.js: Added.
* stress/for-in-shadow-non-enumerable.js: Added.
* test262/expectations.yaml: Mark 4 test cases as passing.

Source/JavaScriptCore:

While for/in was initially specified with notion of "shadowing", it wasn't clarified
until ES5 that [[Enumerable]] attributes are ignored when determining if a property
has already been processed. Recently, for/in spec was expanded [1] to pin down common
case enumeration as it's currently implemented by V8 and SpiderMonkey.

Since keeping track of DontEnum properties is a massive slowdown for uncached runs
(with any data structure used), this patch simply adds [[Enumerable]] check to
has_{indexed,structure,generic}_property bytecode ops and does renaming chores.

Common code is now shared between HasIndexedProperty (emitted for `0 in arr`) and
HasEnumerableIndexedProperty DFG nodes via passing different slow path ops rather
than having OpInfo with PropertySlot::InternalMethodType, which is a nice refactor.

While this change aligns common case for/in enumeration with the spec and other
engines, it also introduces a few observable discrepancies from V8 and SpiderMonkey,
which are permitted by the spec [2]:
a) properties that have been redefined as DontEnum within loop body are skipped,
   which matches the spec [3] and seems like expected behavior;
b) "shadowing" is broken if a DontEnum property of already visited object is
   added / deleted / redefined within loop body, which (pretty much) never happens.

This patch introduces a new invariant: all properties getOwn*PropertyNames() returns
in DontEnumPropertiesMode::Exclude should be reported as [[Enumerable]] by
getOwnPropertySlot(). JSCallbackObject and RuntimeArray are fixed to follow it.

for/in and Object.keys microbenchmarks are neutral. This change does not affect
JSPropertyNameEnumerator caching, nor fast paths of its bytecodes.

[1]: tc39/ecma262#1791
[2]: https://tc39.es/ecma262/#sec-enumerate-object-properties (last paragraph)
[3]: https://tc39.es/ecma262/#sec-%foriniteratorprototype%.next (step 7.b.iii)

* API/JSCallbackObjectFunctions.h:
(JSC::JSCallbackObject<Parent>::getOwnPropertySlot):
* API/tests/testapi.c:
* API/tests/testapiScripts/testapi.js:
* bytecode/BytecodeList.rb:
* bytecode/BytecodeUseDef.cpp:
(JSC::computeUsesForBytecodeIndexImpl):
(JSC::computeDefsForBytecodeIndexImpl):
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::finishCreation):
* bytecode/Opcode.h:
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::emitHasEnumerableIndexedProperty):
(JSC::BytecodeGenerator::emitHasEnumerableStructureProperty):
(JSC::BytecodeGenerator::emitHasEnumerableProperty):
(JSC::BytecodeGenerator::emitHasGenericProperty): Deleted.
(JSC::BytecodeGenerator::emitHasIndexedProperty): Deleted.
(JSC::BytecodeGenerator::emitHasStructureProperty): Deleted.
* bytecompiler/BytecodeGenerator.h:
* bytecompiler/NodesCodegen.cpp:
(JSC::ForInNode::emitBytecode):
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGCapabilities.cpp:
(JSC::DFG::capabilityLevel):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::convertToHasIndexedProperty):
* dfg/DFGNode.h:
(JSC::DFG::Node::hasArrayMode):
(JSC::DFG::Node::hasInternalMethodType const): Deleted.
(JSC::DFG::Node::internalMethodType const): Deleted.
(JSC::DFG::Node::setInternalMethodType): Deleted.
* dfg/DFGNodeType.h:
* dfg/DFGOperations.cpp:
(JSC::DFG::JSC_DEFINE_JIT_OPERATION):
* dfg/DFGOperations.h:
* dfg/DFGPredictionPropagationPhase.cpp:
* dfg/DFGSSALoweringPhase.cpp:
(JSC::DFG::SSALoweringPhase::handleNode):
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileHasEnumerableProperty):
(JSC::DFG::SpeculativeJIT::compileHasEnumerableStructureProperty):
(JSC::DFG::SpeculativeJIT::compileHasIndexedProperty):
(JSC::DFG::SpeculativeJIT::compileHasGenericProperty): Deleted.
(JSC::DFG::SpeculativeJIT::compileHasStructureProperty): Deleted.
* dfg/DFGSpeculativeJIT.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileNode):
(JSC::FTL::DFG::LowerDFGToB3::compileHasIndexedProperty):
(JSC::FTL::DFG::LowerDFGToB3::compileHasEnumerableProperty):
(JSC::FTL::DFG::LowerDFGToB3::compileHasEnumerableStructureProperty):
(JSC::FTL::DFG::LowerDFGToB3::compileHasGenericProperty): Deleted.
(JSC::FTL::DFG::LowerDFGToB3::compileHasStructureProperty): Deleted.
* jit/JIT.cpp:
(JSC::JIT::privateCompileMainPass):
(JSC::JIT::privateCompileSlowCases):
* jit/JIT.h:
* jit/JITOpcodes.cpp:
(JSC::JIT::emit_op_has_enumerable_structure_property):
(JSC::JIT::emit_op_has_enumerable_indexed_property):
(JSC::JIT::emitSlow_op_has_enumerable_indexed_property):
(JSC::JIT::emit_op_has_structure_property): Deleted.
(JSC::JIT::emit_op_has_indexed_property): Deleted.
(JSC::JIT::emitSlow_op_has_indexed_property): Deleted.
* jit/JITOpcodes32_64.cpp:
(JSC::JIT::emit_op_has_enumerable_structure_property):
(JSC::JIT::emit_op_has_enumerable_indexed_property):
(JSC::JIT::emitSlow_op_has_enumerable_indexed_property):
(JSC::JIT::emit_op_has_structure_property): Deleted.
(JSC::JIT::emit_op_has_indexed_property): Deleted.
(JSC::JIT::emitSlow_op_has_indexed_property): Deleted.
* jit/JITOperations.cpp:
(JSC::JSC_DEFINE_JIT_OPERATION):
* jit/JITOperations.h:
* llint/LowLevelInterpreter.asm:
* llint/LowLevelInterpreter64.asm:
* runtime/CommonSlowPaths.cpp:
(JSC::JSC_DEFINE_COMMON_SLOW_PATH):
* runtime/CommonSlowPaths.h:
* runtime/JSObject.cpp:
(JSC::JSObject::hasProperty const):
(JSC::JSObject::hasEnumerableProperty const):
(JSC::JSObject::hasPropertyGeneric const): Deleted.
* runtime/JSObject.h:

Source/WebCore:

Report RuntimeArray indices as [[Enumerable]].

Test: platform/mac/fast/dom/wrapper-classes-objc.html

* bridge/runtime_array.cpp:
(JSC::RuntimeArray::getOwnPropertySlot):
(JSC::RuntimeArray::getOwnPropertySlotByIndex):

LayoutTests:

* platform/mac/fast/dom/wrapper-classes-objc-expected.txt:
* platform/mac/fast/dom/wrapper-classes-objc.html:


Canonical link: https://commits.webkit.org/232510@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@270874 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
shvaikalesh committed Dec 16, 2020
1 parent 2679e9c commit 55e381346da1c49d278ad233d5e6be41e76566d6
Showing 52 changed files with 693 additions and 197 deletions.
@@ -1,3 +1,15 @@
2020-12-15 Alexey Shvayka <shvaikalesh@gmail.com>

Non-enumerable property fails to shadow inherited enumerable property from for-in
https://bugs.webkit.org/show_bug.cgi?id=38970

Reviewed by Keith Miller.

* stress/arguments-bizarre-behaviour-disable-enumerability.js:
* stress/for-in-redefine-enumerable.js: Added.
* stress/for-in-shadow-non-enumerable.js: Added.
* test262/expectations.yaml: Mark 4 test cases as passing.

2020-12-15 Yusuke Suzuki <ysuzuki@apple.com>

Suppress noise from JSTests/stress/lars-sab-workers.js
@@ -18,7 +18,7 @@ var array = [];
for (var s in result[2])
array.push(s);

if (array.join(",") != "0")
if (array.join(",") != "")
throw new Error();

if (Object.keys(result[2]).join(",") != "0")
@@ -0,0 +1,107 @@
function assert(x) {
if (!x)
throw new Error("Bad assertion!");
}

function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error(`Bad value: ${actual}.`);
}

const enumDesc = { value: 0, writable: true, enumerable: true, configurable: true };
const dontEnumDesc = { value: 0, writable: true, enumerable: false, configurable: true };

// indexed property
(() => {
function test() {
var arr = Object.defineProperties([0, 0, 0], { 1: dontEnumDesc });
for (var i in arr) {
assert(i in arr);
shouldBe(arr[i], 0);
++arr[i];
if (i === "0")
Object.defineProperties(arr, { 1: enumDesc, 2: dontEnumDesc });
}
shouldBe(arr[0], 1);
shouldBe(arr[1], 0);
shouldBe(arr[2], 0);
}

for (var i = 0; i < 1e5; ++i)
test();
})();

// structure property
(() => {
function test() {
var obj = Object.create(null, { a: enumDesc, b: enumDesc, c: dontEnumDesc });
for (var key in obj) {
assert(key in obj);
shouldBe(obj[key], 0);
++obj[key];
if (key === "a")
Object.defineProperties(obj, { b: dontEnumDesc, c: enumDesc });
}
shouldBe(obj.a, 1);
shouldBe(obj.b, 0);
shouldBe(obj.c, 0);
}

for (var i = 0; i < 1e5; ++i)
test();
})();

// generic property (Proxy)
(() => {
function test() {
var target = { a: 0, b: 0, c: 0 };
var enumMap = { a: true, b: true, c: false };
var proxy = new Proxy(target, {
getOwnPropertyDescriptor: (_, key) => {
return { value: target[key], writable: true, enumerable: enumMap[key], configurable: true };
},
});

for (var key in proxy) {
assert(key in proxy);
shouldBe(proxy[key], 0);
++target[key];
if (key === "a") {
enumMap.b = false;
enumMap.c = true;
}
}
shouldBe(target.a, 1);
shouldBe(target.b, 0);
shouldBe(target.c, 0);
}

for (var i = 0; i < 1e5; ++i)
test();
})();

// generic property (in prototype)
(() => {
function test() {
var seen = {};
var proto = Object.create(null, { b: enumDesc, c: dontEnumDesc, d: enumDesc, e: enumDesc });
var heir = Object.create(proto, { a: enumDesc, e: dontEnumDesc });
for (var key in heir) {
assert(key in heir);
shouldBe(heir[key], 0);
seen[key] = true;
if (key === "a")
Object.defineProperties(proto, { b: dontEnumDesc, c: enumDesc });
if (key === "d")
Object.defineProperties(heir, { e: enumDesc });
}
assert(seen.a);
assert(!seen.b);
assert(!seen.c);
assert(seen.d);
assert(seen.e);
}

for (var i = 0; i < 1e5; ++i)
test();
})();
@@ -0,0 +1,174 @@
const dontEnumDesc = { value: 1, writable: true, enumerable: false, configurable: true };
const testCases = [
{
name: "Object",
createObject: () => Object.create(null, { foo: dontEnumDesc, bar: dontEnumDesc }),
dontEnumKeys: ["foo", "bar"],
},
{
name: "Error",
createObject: () => new Error(),
dontEnumKeys: ["line", "column", "sourceURL", "stack"],
},
{
name: "Array (empty)",
createObject: () => [],
dontEnumKeys: ["length"],
},
{
name: "Array (sparse)",
createObject: () => Object.defineProperties([0, 1, 2], { 0: dontEnumDesc, 2: dontEnumDesc }),
dontEnumKeys: ["0", "2", "length"],
},
{
name: "Function (strict)",
createObject: () => function() { "use strict"; },
dontEnumKeys: ["length", "name", "prototype"],
},
{
name: "Function (non-strict)",
createObject: () => function() {},
dontEnumKeys: ["arguments", "caller", "length", "name", "prototype"],
},
{
name: "RegExp",
createObject: () => /(?:)/g,
dontEnumKeys: ["lastIndex"],
},
{
name: "String",
createObject: () => new String("foo"),
dontEnumKeys: ["length"],
},
{
name: "Arguments (strict)",
createObject: function(foo) { "use strict"; return arguments; },
dontEnumKeys: ["length", "callee"],
},
{
name: "Arguments (non-strict)",
createObject: function(foo) { return arguments; },
dontEnumKeys: ["length", "callee"],
},
{
name: "Reflect",
createObject: () => $vm.createGlobalObject().Reflect,
dontEnumKeys: ["apply", "get", "has", "set"],
},
{
name: "Date.prototype",
createObject: () => $vm.createGlobalObject().Date.prototype,
dontEnumKeys: ["toISOString", "getTime", "setYear"],
},
];

// basic tests
for (const t of testCases) {
assert(!contains(forIn(t.createObject()), t.dontEnumKeys), t.name);
assert(!contains(Object.keys(t.createObject()), t.dontEnumKeys), t.name);

assert(contains(Object.getOwnPropertyNames(t.createObject()), t.dontEnumKeys), t.name);
assert(contains(Reflect.ownKeys(t.createObject()), t.dontEnumKeys), t.name);
}

// shadowing: DontEnum => Enum
for (const t of testCases) {
assert(
!contains(
forIn(makePrototypeChain(t.createObject(), makeObject(t.dontEnumKeys))),
t.dontEnumKeys,
),
t.name,
);
}

// shadowing: {} => DontEnum => {} => Enum
for (const t of testCases) {
assert(
!contains(
forIn(makePrototypeChain({}, t.createObject(), {}, makeObject(t.dontEnumKeys))),
t.dontEnumKeys,
),
t.name,
);
}

// shadowing: DontEnum => {} => Enum => {} => Enum
for (const t of testCases) {
assert(
!contains(
forIn(makePrototypeChain(t.createObject(), {}, makeObject(t.dontEnumKeys), {}, makeObject(t.dontEnumKeys))),
t.dontEnumKeys,
),
t.name,
);
}

// shadowing: {} => DontEnum (enumerable: true => false) => {} => Enum
for (const t of testCases) {
const dontEnumObject = t.createObject();
const target = makePrototypeChain({}, dontEnumObject, {}, makeObject(t.dontEnumKeys));
assert(!contains(forIn(target), t.dontEnumKeys), t.name);

const enumKeys = t.dontEnumKeys.filter(key => Reflect.defineProperty(dontEnumObject, key, { enumerable: true }));
assert(contains(forIn(target), enumKeys), t.name);
}

// shadowing: {} => DontEnum (delete non-enumerable keys) => {} => Enum
for (const t of testCases) {
const dontEnumObject = t.createObject();
const target = makePrototypeChain({}, dontEnumObject, {}, makeObject(t.dontEnumKeys));
assert(!contains(forIn(target), t.dontEnumKeys), t.name);

const enumKeys = t.dontEnumKeys.filter(key => Reflect.deleteProperty(dontEnumObject, key));
assert(contains(forIn(target), enumKeys), t.name);
}

// shadowing: {} => DontEnum (materialized) => {} => Enum
for (const t of testCases) {
const dontEnumObject = t.createObject();
const target = makePrototypeChain({}, dontEnumObject, {}, makeObject(t.dontEnumKeys));
assert(!contains(forIn(target), t.dontEnumKeys), t.name);

const dontEnumKeys = t.dontEnumKeys.filter(key => {
const desc = Object.getOwnPropertyDescriptor(dontEnumObject, key);
if (!Reflect.deleteProperty(dontEnumObject, key)) return false;
Object.defineProperty(dontEnumObject, key, desc);
return true;
});

if (dontEnumKeys.length)
assert(!contains(forIn(target), dontEnumKeys), t.name);
}

// helpers
function assert(value, name) {
if (!value)
throw new Error(`Bad value: ${value}. Test case: ${name}.`);
}

function contains(array, subarray) {
return subarray.every(item => array.includes(item));
}

function forIn(object) {
const keys = [];
for (const key in object)
keys.push(key);
return keys;
}

function makePrototypeChain(...objects) {
objects.reduce((object, prototype) => {
Object.setPrototypeOf(object, prototype);
return prototype;
});
return objects[0];
}

function makeObject(keys) {
const object = {};
for (const key of keys)
object[key] = key;
return object;
}
@@ -1858,16 +1858,10 @@ test/language/statements/for-await-of/ticks-with-async-iter-resolved-promise-and
test/language/statements/for-await-of/ticks-with-sync-iter-resolved-promise-and-constructor-lookup.js:
default: 'Test262:AsyncTestFailure:Test262Error: Test262Error: Expected [pre, tick 1, constructor, constructor, tick 2, tick 3, loop, tick 4, constructor] and [pre, constructor, constructor, tick 1, tick 2, loop, constructor, tick 3, tick 4, post] to have the same contents. Ticks and constructor lookups'
strict mode: 'Test262:AsyncTestFailure:Test262Error: Test262Error: Expected [pre, tick 1, constructor, constructor, tick 2, tick 3, loop, tick 4, constructor] and [pre, constructor, constructor, tick 1, tick 2, loop, constructor, tick 3, tick 4, post] to have the same contents. Ticks and constructor lookups'
test/language/statements/for-in/12.6.4-2.js:
default: 'Test262Error: accessedProp Expected SameValue(«true», «false») to be true'
strict mode: 'Test262Error: accessedProp Expected SameValue(«true», «false») to be true'
test/language/statements/for-in/head-lhs-let.js:
default: "SyntaxError: Cannot use the keyword 'in' as a lexical variable name."
test/language/statements/for-in/identifier-let-allowed-as-lefthandside-expression-not-strict.js:
default: "SyntaxError: Cannot use the keyword 'in' as a lexical variable name."
test/language/statements/for-in/order-enumerable-shadowed.js:
default: 'Test262Error: Expected [p1, p2] and [p1] to have the same contents. '
strict mode: 'Test262Error: Expected [p1, p2] and [p1] to have the same contents. '
test/language/statements/for-in/scope-body-lex-open.js:
default: 'Test262Error: Expected a ReferenceError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: Expected a ReferenceError to be thrown but no exception was thrown at all'
@@ -1,3 +1,13 @@
2020-12-15 Alexey Shvayka <shvaikalesh@gmail.com>

Non-enumerable property fails to shadow inherited enumerable property from for-in
https://bugs.webkit.org/show_bug.cgi?id=38970

Reviewed by Keith Miller.

* platform/mac/fast/dom/wrapper-classes-objc-expected.txt:
* platform/mac/fast/dom/wrapper-classes-objc.html:

2020-12-15 Alex Christensen <achristensen@webkit.org>

REGRESSION: [macOS] http/tests/inspector/network/resource-response-service-worker.html is a flaky failure
@@ -189,6 +189,7 @@ PASS typeof objCObjectOfClass('NSCFString') is 'string'
PASS typeof objCObjectOfClass('WebScriptObject') is 'object'
PASS objCObjectOfClass('NSArray') instanceof Array is true
PASS concatenateArray(objCArrayOfString()) is 'onetwothree'
PASS objCArrayOfString().every((_, i, arr) => arr.propertyIsEnumerable(i)) is true
PASS let arr = objCArrayOfString(); arr.length is 3
PASS let arr = objCArrayOfString(); arr.length = 0 threw exception RangeError: Range error.
PASS let arr = objCArrayOfString(); arr.length = 5 threw exception RangeError: Range error.
@@ -286,6 +286,7 @@
shouldBeTrue("objCObjectOfClass('NSArray') instanceof Array");

shouldBe("concatenateArray(objCArrayOfString())", "'onetwothree'");
shouldBeTrue("objCArrayOfString().every((_, i, arr) => arr.propertyIsEnumerable(i))");

shouldBe("let arr = objCArrayOfString(); arr.length", "3");
shouldThrow("let arr = objCArrayOfString(); arr.length = 0");

0 comments on commit 55e3813

Please sign in to comment.