Skip to content

Commit 4792d4f

Browse files
woessgilles-duboscq
authored andcommitted
[GR-40880] Fix [[DefineOwnProperty]] on dictionary object with a not fully populated property descriptor.
PullRequest: js/2584
2 parents 600a3e4 + e94a9d1 commit 4792d4f

File tree

8 files changed

+259
-126
lines changed

8 files changed

+259
-126
lines changed
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/*
2+
* Copyright (c) 2022, 2022, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* Licensed under the Universal Permissive License v 1.0 as shown at http://oss.oracle.com/licenses/upl.
6+
*/
7+
8+
load("assert.js");
9+
10+
/**
11+
* Tests that a property descriptor that is neither a data nor an accessor property descriptor is handled correctly by dictionary object.
12+
*
13+
* @option debug-builtin
14+
*/
15+
16+
function replacer(key, value) {
17+
if (typeof value === 'undefined' || typeof value === 'function') {
18+
return typeof value;
19+
} else {
20+
return value;
21+
}
22+
}
23+
24+
function verifyFullyPopulatedPropertyDescriptor(desc) {
25+
let hasOwnProperty = key => Object.hasOwn(desc, key);
26+
if (['get', 'set', 'enumerable', 'configurable'].every(hasOwnProperty)) {
27+
assertFalse(['value', 'writable'].some(hasOwnProperty));
28+
return true;
29+
} else if (['value', 'writable', 'enumerable', 'configurable'].every(hasOwnProperty)) {
30+
assertFalse(['get', 'set'].some(hasOwnProperty));
31+
return true;
32+
} else {
33+
throw new Error(`Not a fully populated property descriptor: ${JSON.stringify(desc, replacer)}`);
34+
}
35+
}
36+
37+
const obj = {};
38+
obj["0"] = "h";
39+
obj["1"] = "e";
40+
obj["2"] = "l";
41+
obj["3"] = "l";
42+
obj["4"] = "o";
43+
obj["unexpected"] = 404;
44+
45+
if (typeof Debug != "undefined" && Debug.shape(obj).includes("unexpected")) {
46+
throw new Error(`obj should be a dictionary object but has an unexpected shape: ${Debug.shape(obj)}`);
47+
}
48+
49+
Object.defineProperty(obj, "call", ({}));
50+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "call"));
51+
assertSame(undefined, obj["call"]);
52+
53+
Object.defineProperty(obj, "call1", {configurable: true});
54+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "call1"));
55+
assertSame(undefined, obj["call1"]);
56+
Object.defineProperty(obj, "call2", {configurable: true, enumerable: false});
57+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "call2"));
58+
assertSame(undefined, obj["call2"]);
59+
Object.defineProperty(obj, "call3", {configurable: true, enumerable: false, writable: true});
60+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "call3"));
61+
assertSame(undefined, obj["call3"]);
62+
63+
Object.defineProperty(obj, "accessor", {get: function(){ return 42; }});
64+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "accessor"));
65+
Object.defineProperty(obj, "accessor1", {get: function(){ return 42; }, configurable: true});
66+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "accessor1"));
67+
Object.defineProperty(obj, "accessor2", {get: function(){ return 42; }, configurable: true, enumerable: false});
68+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "accessor2"));
69+
70+
Object.defineProperty(obj, "call3", {get: function(){ return 42; }, configurable: true, enumerable: false});
71+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "call3"));
72+
assertSame(42, obj["call3"]);
73+
Object.defineProperty(obj, "call3", {value: 43, configurable: true, enumerable: false});
74+
verifyFullyPopulatedPropertyDescriptor(Object.getOwnPropertyDescriptor(obj, "call3"));
75+
assertSame(43, obj["call3"]);

graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/decorators/DefineMethodPropertyNode.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,10 @@ public void doMethod(JSDynamicObject homeObject, ClassElementDefinitionRecord re
7373

7474
@Specialization(guards = {"record.isGetter()", "!record.isPrivate()"})
7575
public void doGetter(JSDynamicObject homeObject, ClassElementDefinitionRecord record, boolean enumerable) {
76-
JSDynamicObject otherAccessor = Undefined.instance;
76+
Object otherAccessor = Undefined.instance;
7777
PropertyDescriptor ownProperty = JSObject.getOwnProperty(homeObject, record.getKey());
7878
if (ownProperty != null) {
79-
otherAccessor = (JSDynamicObject) ownProperty.getSet();
79+
otherAccessor = ownProperty.getSet();
8080
}
8181
PropertyDescriptor desc = PropertyDescriptor.createAccessor(record.getValue(), otherAccessor, enumerable, true);
8282
JSRuntime.definePropertyOrThrow(homeObject, record.getKey(), desc);

graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/JSGetOwnPropertyNode.java

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,20 @@ PropertyDescriptor array(JSDynamicObject thisObj, Object propertyKey,
126126
if (JSRuntime.isArrayIndex(idx)) {
127127
ScriptArray array = typeProfile.profile(JSAbstractArray.arrayGetArrayType(thisObj));
128128
if (array.hasElement(thisObj, idx)) {
129-
Object value = needValue ? array.getElement(thisObj, idx) : null;
130-
return PropertyDescriptor.createData(value, true, needWritability && !array.isFrozen(), needConfigurability && !array.isSealed());
129+
PropertyDescriptor desc = PropertyDescriptor.createEmpty();
130+
if (needEnumerability) {
131+
desc.setEnumerable(true);
132+
}
133+
if (needConfigurability) {
134+
desc.setConfigurable(!array.isSealed());
135+
}
136+
if (needWritability) {
137+
desc.setWritable(!array.isFrozen());
138+
}
139+
if (needValue) {
140+
desc.setValue(array.getElement(thisObj, idx));
141+
}
142+
return desc;
131143
}
132144
}
133145
noSuchElementBranch.enter();
@@ -188,22 +200,18 @@ private PropertyDescriptor ordinaryGetOwnProperty(JSDynamicObject thisObj, Prope
188200
if (hasPropertyBranch.profile(prop == null)) {
189201
return null;
190202
}
191-
PropertyDescriptor d;
203+
PropertyDescriptor d = PropertyDescriptor.createEmpty();
192204
if (isDataPropertyBranch.profile(JSProperty.isData(prop))) {
193-
Object value = needValue ? getDataPropertyValue(thisObj, prop) : null;
194-
d = PropertyDescriptor.createData(value);
195205
if (needWritability) {
196206
d.setWritable(JSProperty.isWritable(prop));
197207
}
208+
if (needValue) {
209+
d.setValue(getDataPropertyValue(thisObj, prop));
210+
}
198211
} else if (isAccessorPropertyBranch.profile(JSProperty.isAccessor(prop))) {
199212
if (needValue) {
200-
Accessor acc = (Accessor) prop.getLocation().get(thisObj);
201-
d = PropertyDescriptor.createAccessor(acc.getGetter(), acc.getSetter());
202-
} else {
203-
d = PropertyDescriptor.createAccessor(null, null);
213+
d.setAccessor((Accessor) prop.getLocation().get(thisObj));
204214
}
205-
} else {
206-
d = PropertyDescriptor.createEmpty();
207215
}
208216
if (needEnumerability) {
209217
d.setEnumerable(JSProperty.isEnumerable(prop));

graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/runtime/builtins/JSDictionary.java

Lines changed: 120 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,16 @@
4040
*/
4141
package com.oracle.truffle.js.runtime.builtins;
4242

43-
import java.util.AbstractMap;
43+
import static com.oracle.truffle.js.runtime.util.DefinePropertyUtil.nonConfigurableMessage;
44+
import static com.oracle.truffle.js.runtime.util.DefinePropertyUtil.nonWritableMessage;
45+
import static com.oracle.truffle.js.runtime.util.DefinePropertyUtil.notExtensibleMessage;
46+
import static com.oracle.truffle.js.runtime.util.DefinePropertyUtil.reject;
47+
4448
import java.util.ArrayList;
4549
import java.util.Collections;
4650
import java.util.List;
47-
import java.util.Map;
4851

4952
import org.graalvm.collections.EconomicMap;
50-
import org.graalvm.collections.MapCursor;
5153

5254
import com.oracle.truffle.api.CompilerAsserts;
5355
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
@@ -76,7 +78,6 @@
7678
import com.oracle.truffle.js.runtime.objects.JSShape;
7779
import com.oracle.truffle.js.runtime.objects.Null;
7880
import com.oracle.truffle.js.runtime.objects.PropertyDescriptor;
79-
import com.oracle.truffle.js.runtime.objects.PropertyProxy;
8081
import com.oracle.truffle.js.runtime.objects.Undefined;
8182
import com.oracle.truffle.js.runtime.util.DefinePropertyUtil;
8283

@@ -128,7 +129,7 @@ public Object getOwnHelper(JSDynamicObject store, Object thisObj, Object key, No
128129

129130
public static Object getValue(PropertyDescriptor property, Object receiver, Node encapsulatingNode) {
130131
if (property.isAccessorDescriptor()) {
131-
JSDynamicObject getter = (JSDynamicObject) property.getGet();
132+
Object getter = property.getGet();
132133
if (getter != Undefined.instance) {
133134
return JSRuntime.call(getter, receiver, JSArguments.EMPTY_ARGUMENTS_ARRAY, encapsulatingNode);
134135
} else {
@@ -222,7 +223,7 @@ protected static boolean dictionaryObjectSet(JSDynamicObject thisObj, Object key
222223

223224
private static boolean setValue(Object key, PropertyDescriptor property, JSDynamicObject store, Object thisObj, Object value, boolean isStrict, Node encapsulatingNode) {
224225
if (property.isAccessorDescriptor()) {
225-
JSDynamicObject setter = (JSDynamicObject) property.getSet();
226+
Object setter = property.getSet();
226227
if (setter != Undefined.instance) {
227228
JSRuntime.call(setter, thisObj, new Object[]{value}, encapsulatingNode);
228229
return true;
@@ -260,15 +261,122 @@ public PropertyDescriptor getOwnProperty(JSDynamicObject thisObj, Object key) {
260261
@Override
261262
public boolean defineOwnProperty(JSDynamicObject thisObj, Object key, PropertyDescriptor desc, boolean doThrow) {
262263
assert JSRuntime.isPropertyKey(key);
263-
if (!hasOwnProperty(thisObj, key) && JSObject.isExtensible(thisObj)) {
264-
getHashMap(thisObj).put(key, desc);
264+
PropertyDescriptor current = getHashMap(thisObj).get(key);
265+
if (current == null) {
266+
current = super.getOwnProperty(thisObj, key);
267+
boolean extensible = JSObject.isExtensible(thisObj);
268+
if (current == null) {
269+
if (!extensible) {
270+
return reject(doThrow, notExtensibleMessage(key, doThrow));
271+
}
272+
validateAndPutDesc(thisObj, key, makeFullyPopulatedPropertyDescriptor(desc));
273+
return true;
274+
} else {
275+
return DefinePropertyUtil.validateAndApplyPropertyDescriptor(thisObj, key, extensible, desc, current, doThrow);
276+
}
277+
} else {
278+
return validateAndApplyPropertyDescriptorExisting(thisObj, key, desc, current, doThrow);
279+
}
280+
}
281+
282+
private static PropertyDescriptor validateAndPutDesc(JSDynamicObject thisObj, Object key, PropertyDescriptor newDesc) {
283+
assert newDesc.isFullyPopulatedPropertyDescriptor();
284+
return getHashMap(thisObj).put(key, newDesc);
285+
}
286+
287+
/**
288+
* Returns a fully populated property descriptor that is either an accessor property descriptor
289+
* or a data property descriptor that has all of the corresponding fields. Missing fields are
290+
* filled with default values.
291+
*/
292+
private static PropertyDescriptor makeFullyPopulatedPropertyDescriptor(PropertyDescriptor desc) {
293+
if (desc.isAccessorDescriptor()) {
294+
if (desc.hasGet() && desc.hasSet() && desc.hasEnumerable() && desc.hasConfigurable()) {
295+
return desc;
296+
} else {
297+
return PropertyDescriptor.createAccessor(desc.getGet(), desc.getSet(), desc.getEnumerable(), desc.getConfigurable());
298+
}
299+
} else if (desc.isDataDescriptor()) {
300+
if (desc.hasValue() && desc.hasWritable() && desc.hasEnumerable() && desc.hasConfigurable()) {
301+
return desc;
302+
} else {
303+
Object value = desc.hasValue() ? desc.getValue() : Undefined.instance;
304+
return PropertyDescriptor.createData(value, desc.getEnumerable(), desc.getWritable(), desc.getConfigurable());
305+
}
306+
} else {
307+
assert desc.isGenericDescriptor();
308+
return PropertyDescriptor.createData(Undefined.instance, desc.getEnumerable(), desc.getWritable(), desc.getConfigurable());
309+
}
310+
}
311+
312+
private static boolean validateAndApplyPropertyDescriptorExisting(JSDynamicObject thisObj, Object key, PropertyDescriptor descriptor, PropertyDescriptor currentDesc, boolean doThrow) {
313+
CompilerAsserts.neverPartOfCompilation();
314+
assert currentDesc.isFullyPopulatedPropertyDescriptor();
315+
if (descriptor.hasNoFields()) {
265316
return true;
266317
}
267318

268-
// defineProperty is currently not supported on dictionary objects,
269-
// so we need to convert back to a normal shape-based object.
270-
makeOrdinaryObject(thisObj, "defineOwnProperty");
271-
return super.defineOwnProperty(thisObj, key, desc, doThrow);
319+
if (!currentDesc.getConfigurable()) {
320+
if ((descriptor.hasConfigurable() && descriptor.getConfigurable()) ||
321+
(descriptor.hasEnumerable() && (descriptor.getEnumerable() != currentDesc.getEnumerable()))) {
322+
return reject(doThrow, nonConfigurableMessage(key, doThrow));
323+
}
324+
if (!descriptor.isGenericDescriptor() && descriptor.isAccessorDescriptor() != currentDesc.isAccessorDescriptor()) {
325+
return reject(doThrow, nonConfigurableMessage(key, doThrow));
326+
}
327+
if (currentDesc.isAccessorDescriptor()) {
328+
if ((descriptor.hasGet() && !JSRuntime.isSameValue(descriptor.getGet(), currentDesc.getGet())) ||
329+
(descriptor.hasSet() && !JSRuntime.isSameValue(descriptor.getSet(), currentDesc.getSet()))) {
330+
return reject(doThrow, nonConfigurableMessage(key, doThrow));
331+
}
332+
} else {
333+
assert currentDesc.isDataDescriptor();
334+
if (!currentDesc.getWritable()) {
335+
if (descriptor.hasWritable() && descriptor.getWritable()) {
336+
return reject(doThrow, nonConfigurableMessage(key, doThrow));
337+
}
338+
if (descriptor.hasValue() && !JSRuntime.isSameValue(descriptor.getValue(), currentDesc.getValue())) {
339+
return reject(doThrow, nonWritableMessage(key, doThrow));
340+
}
341+
}
342+
}
343+
}
344+
345+
if (currentDesc.isDataDescriptor() && descriptor.isAccessorDescriptor()) {
346+
PropertyDescriptor newDesc = PropertyDescriptor.createAccessor(descriptor.getGet(), descriptor.getSet(),
347+
descriptor.getIfHasEnumerable(currentDesc.getEnumerable()),
348+
descriptor.getIfHasConfigurable(currentDesc.getConfigurable()));
349+
validateAndPutDesc(thisObj, key, newDesc);
350+
return true;
351+
} else if (currentDesc.isAccessorDescriptor() && descriptor.isDataDescriptor()) {
352+
Object value = descriptor.hasValue() ? descriptor.getValue() : Undefined.instance;
353+
PropertyDescriptor newDesc = PropertyDescriptor.createData(value,
354+
descriptor.getIfHasEnumerable(currentDesc.getEnumerable()),
355+
descriptor.getIfHasConfigurable(currentDesc.getConfigurable()),
356+
descriptor.getWritable());
357+
validateAndPutDesc(thisObj, key, newDesc);
358+
return true;
359+
} else {
360+
if (descriptor.hasConfigurable()) {
361+
currentDesc.setConfigurable(descriptor.getConfigurable());
362+
}
363+
if (descriptor.hasEnumerable()) {
364+
currentDesc.setEnumerable(descriptor.getEnumerable());
365+
}
366+
if (descriptor.hasWritable()) {
367+
currentDesc.setWritable(descriptor.getWritable());
368+
}
369+
if (descriptor.hasValue()) {
370+
currentDesc.setValue(descriptor.getValue());
371+
}
372+
if (descriptor.hasGet()) {
373+
currentDesc.setGet(descriptor.getGet());
374+
}
375+
if (descriptor.hasSet()) {
376+
currentDesc.setSet(descriptor.getSet());
377+
}
378+
return true;
379+
}
272380
}
273381

274382
@SuppressWarnings("unchecked")
@@ -357,63 +465,6 @@ private static PropertyDescriptor toPropertyDescriptor(Property p, Object value)
357465
return desc;
358466
}
359467

360-
private static void makeOrdinaryObject(JSDynamicObject obj, String reason) {
361-
CompilerAsserts.neverPartOfCompilation();
362-
if (JSConfig.TraceDictionaryObject) {
363-
System.out.printf("transitioning from dictionary object to ordinary object: %s\n", reason);
364-
}
365-
366-
EconomicMap<Object, PropertyDescriptor> hashMap = getHashMap(obj);
367-
Shape oldShape = obj.getShape();
368-
JSContext context = JSObject.getJSContext(obj);
369-
Shape newRootShape = makeEmptyShapeForNewType(context, oldShape, JSOrdinary.INSTANCE, obj);
370-
371-
DynamicObjectLibrary lib = DynamicObjectLibrary.getUncached();
372-
373-
List<Property> allProperties = oldShape.getPropertyListInternal(true);
374-
List<Map.Entry<Property, Object>> archive = new ArrayList<>(allProperties.size());
375-
for (Property prop : allProperties) {
376-
Object key = prop.getKey();
377-
Object value = Properties.getOrDefault(lib, obj, key, null);
378-
if (HASHMAP_PROPERTY_NAME.equals(key)) {
379-
continue;
380-
}
381-
archive.add(new AbstractMap.SimpleImmutableEntry<>(prop, value));
382-
}
383-
384-
lib.resetShape(obj, newRootShape);
385-
386-
for (int i = 0; i < archive.size(); i++) {
387-
Map.Entry<Property, Object> e = archive.get(i);
388-
Property p = e.getKey();
389-
Object key = p.getKey();
390-
if (!newRootShape.hasProperty(key)) {
391-
Object value = e.getValue();
392-
if (p.getLocation().isConstant()) {
393-
Properties.putConstant(lib, obj, key, value, p.getFlags());
394-
} else {
395-
Properties.putWithFlags(lib, obj, key, value, p.getFlags());
396-
}
397-
}
398-
}
399-
400-
MapCursor<Object, PropertyDescriptor> cursor = hashMap.getEntries();
401-
while (cursor.advance()) {
402-
Object key = cursor.getKey();
403-
assert JSRuntime.isPropertyKey(key);
404-
PropertyDescriptor desc = cursor.getValue();
405-
if (desc.isDataDescriptor()) {
406-
Object value = desc.getValue();
407-
assert !(value instanceof Accessor || value instanceof PropertyProxy);
408-
JSObjectUtil.defineDataProperty(obj, key, value, desc.getFlags());
409-
} else {
410-
JSObjectUtil.defineAccessorProperty(obj, key, new Accessor(desc.getGet(), desc.getSet()), desc.getFlags());
411-
}
412-
}
413-
414-
assert JSOrdinary.isJSOrdinaryObject(obj) && obj.getShape().getProperty(HASHMAP_PROPERTY_NAME) == null;
415-
}
416-
417468
public static Shape makeDictionaryShape(JSContext context, JSDynamicObject prototype) {
418469
assert prototype != Null.instance;
419470
return JSObjectUtil.getProtoChildShape(prototype, JSDictionary.INSTANCE, context);

graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/runtime/builtins/JSNonProxy.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ protected static boolean setPropertySlow(JSDynamicObject thisObj, Object key, Ob
422422
protected static boolean invokeAccessorPropertySetter(PropertyDescriptor desc, JSDynamicObject thisObj, Object key, Object value, Object receiver, boolean isStrict, Node encapsulatingNode) {
423423
CompilerAsserts.neverPartOfCompilation();
424424
assert desc.isAccessorDescriptor();
425-
JSDynamicObject setter = (JSDynamicObject) desc.getSet();
425+
Object setter = desc.getSet();
426426
if (setter != Undefined.instance) {
427427
JSRuntime.call(setter, receiver, new Object[]{value}, encapsulatingNode);
428428
return true;

0 commit comments

Comments
 (0)