REGRESSION (r270874): Some React Native apps are reported broken on iOS

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

Reviewed by Saam Barati.

Source/JavaScriptCore:

r270874 fixed for/in shadowing issue by introducing an invariant: a property
returned by getOwn*PropertyNames() in DontEnumPropertiesMode::Exclude should be
reported as [[Enumerable]] by getOwnPropertySlot(). Otherwise, for/in skips the
property, which causes RN apps to break.

Since there is no way to enforce this invariant for opaque API objects like
JSCallbackObject, this change skips [[Enumerable]] check for them by introducing
GetOwnPropertySlotMayBeWrongAboutDontEnum out of line type info flag.

Also, this patch reverts JSCallbackObject::getOwnPropertySlot() changes of r270874
that are no longer necessary and observable (via Object.getOwnPropertyDescriptor).

* API/JSCallbackObject.h:
* API/JSCallbackObjectFunctions.h:
(JSC::JSCallbackObject<Parent>::getOwnPropertySlot):
* API/tests/testapiScripts/testapi.js:
* runtime/JSObject.cpp:
(JSC::JSObject::hasEnumerableProperty const):
* runtime/JSTypeInfo.h:
(JSC::TypeInfo::getOwnPropertySlotMayBeWrongAboutDontEnum const):

Source/WebCore:

* bridge/runtime_object.h:

Source/WebKit:

* WebProcess/Plugins/Netscape/JSNPObject.h:


Canonical link: https://commits.webkit.org/233334@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271873 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/API/JSCallbackObject.h

@@ -125,7 +125,7 @@ template <class Parent>
 class JSCallbackObject final : public Parent {
 public:
     using Base = Parent;
-    static constexpr unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | OverridesGetOwnSpecialPropertyNames | OverridesGetCallData | InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | ImplementsHasInstance | ProhibitsPropertyCaching;
+    static constexpr unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | OverridesGetOwnSpecialPropertyNames | OverridesGetCallData | InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | ImplementsHasInstance | ProhibitsPropertyCaching | GetOwnPropertySlotMayBeWrongAboutDontEnum;
     static_assert(!(StructureFlags & ImplementsDefaultHasInstance), "using customHasInstance");
 
     ~JSCallbackObject();

Source/JavaScriptCore/API/JSCallbackObjectFunctions.h

@@ -162,9 +162,7 @@ bool JSCallbackObject<Parent>::getOwnPropertySlot(JSObject* object, JSGlobalObje
     RefPtr<OpaqueJSString> propertyNameRef;
 
     if (StringImpl* name = propertyName.uid()) {
-        // FIXME: Set ReadOnly conditionally, based on setProperty presence in class inheritance chain.
-        // https://bugs.webkit.org/show_bug.cgi?id=219924
-        unsigned attributes = static_cast<unsigned>(PropertyAttribute::ReadOnly);
+        unsigned attributes = PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum;
         for (JSClassRef jsClass = thisObject->classRef(); jsClass; jsClass = jsClass->parentClass) {
             // optional optimization to bypass getProperty in cases when we only need to know if the property exists
             if (JSObjectHasPropertyCallback hasProperty = jsClass->hasProperty) {
@@ -196,21 +194,19 @@ bool JSCallbackObject<Parent>::getOwnPropertySlot(JSObject* object, JSGlobalObje
             }
 
             if (OpaqueJSClassStaticValuesTable* staticValues = jsClass->staticValues(globalObject)) {
-                if (StaticValueEntry* entry = staticValues->get(name)) {
-                    // FIXME: getStaticValue() performs the same loop & checks just to acquire `entry`.
-                    // https://bugs.webkit.org/show_bug.cgi?id=219925
+                if (staticValues->contains(name)) {
                     JSValue value = thisObject->getStaticValue(globalObject, propertyName);
                     RETURN_IF_EXCEPTION(scope, false);
                     if (value) {
-                        slot.setValue(thisObject, entry->attributes, value);
+                        slot.setValue(thisObject, attributes, value);
                         return true;
                     }
                 }
             }
 
             if (OpaqueJSClassStaticFunctionsTable* staticFunctions = jsClass->staticFunctions(globalObject)) {
-                if (StaticFunctionEntry* entry = staticFunctions->get(name)) {
-                    slot.setCustom(thisObject, entry->attributes, getStaticFunctionGetter());
+                if (staticFunctions->contains(name)) {
+                    slot.setCustom(thisObject, attributes, getStaticFunctionGetter());
                     return true;
                 }
             }

Source/JavaScriptCore/API/tests/testapiScripts/testapi.js

@@ -144,12 +144,12 @@ var alwaysOneDescriptor = Object.getOwnPropertyDescriptor(MyObject, "alwaysOne")
 shouldBe('typeof alwaysOneDescriptor', "object");
 shouldBe('alwaysOneDescriptor.value', MyObject.alwaysOne);
 shouldBe('alwaysOneDescriptor.configurable', true);
-shouldBe('alwaysOneDescriptor.enumerable', true);
+shouldBe('alwaysOneDescriptor.enumerable', false); // Actually it is.
 var cantFindDescriptor = Object.getOwnPropertyDescriptor(MyObject, "cantFind");
 shouldBe('typeof cantFindDescriptor', "object");
 shouldBe('cantFindDescriptor.value', MyObject.cantFind);
 shouldBe('cantFindDescriptor.configurable', true);
-shouldBe('cantFindDescriptor.enumerable', true);
+shouldBe('cantFindDescriptor.enumerable', false);
 try {
     // If getOwnPropertyDescriptor() returned an access descriptor, this wouldn't throw.
     Object.getOwnPropertyDescriptor(MyObject, "throwOnGet");
@@ -160,7 +160,7 @@ var myPropertyNameDescriptor = Object.getOwnPropertyDescriptor(MyObject, "myProp
 shouldBe('typeof myPropertyNameDescriptor', "object");
 shouldBe('myPropertyNameDescriptor.value', MyObject.myPropertyName);
 shouldBe('myPropertyNameDescriptor.configurable', true);
-shouldBe('myPropertyNameDescriptor.enumerable', true);
+shouldBe('myPropertyNameDescriptor.enumerable', false); // Actually it is.
 try {
     // if getOwnPropertyDescriptor() returned an access descriptor, this wouldn't throw.
     Object.getOwnPropertyDescriptor(MyObject, "hasPropertyLie");
@@ -247,23 +247,23 @@ var baseDupDescriptor = Object.getOwnPropertyDescriptor(derived, "baseDup");
 shouldBe('typeof baseDupDescriptor', "object");
 shouldBe('baseDupDescriptor.value', derived.baseDup);
 shouldBe('baseDupDescriptor.configurable', true);
-shouldBe('baseDupDescriptor.enumerable', true);
+shouldBe('baseDupDescriptor.enumerable', false);
 var baseOnlyDescriptor = Object.getOwnPropertyDescriptor(derived, "baseOnly");
 shouldBe('typeof baseOnlyDescriptor', "object");
 shouldBe('baseOnlyDescriptor.value', derived.baseOnly);
 shouldBe('baseOnlyDescriptor.configurable', true);
-shouldBe('baseOnlyDescriptor.enumerable', true);
+shouldBe('baseOnlyDescriptor.enumerable', false);
 shouldBe('Object.getOwnPropertyDescriptor(derived, "protoOnly")', undefined);
 var protoDupDescriptor = Object.getOwnPropertyDescriptor(derived, "protoDup");
 shouldBe('typeof protoDupDescriptor', "object");
 shouldBe('protoDupDescriptor.value', derived.protoDup);
 shouldBe('protoDupDescriptor.configurable', true);
-shouldBe('protoDupDescriptor.enumerable', true);
+shouldBe('protoDupDescriptor.enumerable', false);
 var derivedOnlyDescriptor = Object.getOwnPropertyDescriptor(derived, "derivedOnly");
 shouldBe('typeof derivedOnlyDescriptor', "object");
 shouldBe('derivedOnlyDescriptor.value', derived.derivedOnly);
 shouldBe('derivedOnlyDescriptor.configurable', true);
-shouldBe('derivedOnlyDescriptor.enumerable', true);
+shouldBe('derivedOnlyDescriptor.enumerable', false);
 
 shouldBe("undefined instanceof MyObject", false);
 EvilExceptionObject.hasInstance = function f() { return f(); };

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,31 @@
+2021-01-25  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        REGRESSION (r270874): Some React Native apps are reported broken on iOS
+        https://bugs.webkit.org/show_bug.cgi?id=220809
+
+        Reviewed by Saam Barati.
+
+        r270874 fixed for/in shadowing issue by introducing an invariant: a property
+        returned by getOwn*PropertyNames() in DontEnumPropertiesMode::Exclude should be
+        reported as [[Enumerable]] by getOwnPropertySlot(). Otherwise, for/in skips the
+        property, which causes RN apps to break.
+
+        Since there is no way to enforce this invariant for opaque API objects like
+        JSCallbackObject, this change skips [[Enumerable]] check for them by introducing
+        GetOwnPropertySlotMayBeWrongAboutDontEnum out of line type info flag.
+
+        Also, this patch reverts JSCallbackObject::getOwnPropertySlot() changes of r270874
+        that are no longer necessary and observable (via Object.getOwnPropertyDescriptor).
+
+        * API/JSCallbackObject.h:
+        * API/JSCallbackObjectFunctions.h:
+        (JSC::JSCallbackObject<Parent>::getOwnPropertySlot):
+        * API/tests/testapiScripts/testapi.js:
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::hasEnumerableProperty const):
+        * runtime/JSTypeInfo.h:
+        (JSC::TypeInfo::getOwnPropertySlotMayBeWrongAboutDontEnum const):
+
 2021-01-25  Chris Dumez  <cdumez@apple.com>
 
         Update availability annotations to match the macOS 11.0 and iOS 14.0 GM SDKs

Source/JavaScriptCore/runtime/JSObject.cpp

@@ -1998,7 +1998,9 @@ bool JSObject::hasEnumerableProperty(JSGlobalObject* globalObject, PropertyName
     PropertySlot slot(this, PropertySlot::InternalMethodType::GetOwnProperty);
     bool hasProperty = const_cast<JSObject*>(this)->getPropertySlot(globalObject, propertyName, slot);
     RETURN_IF_EXCEPTION(scope, false);
-    return hasProperty && !(slot.attributes() & PropertyAttribute::DontEnum);
+    if (!hasProperty)
+        return false;
+    return !(slot.attributes() & PropertyAttribute::DontEnum) || (slot.slotBase() && slot.slotBase()->structure(vm)->typeInfo().getOwnPropertySlotMayBeWrongAboutDontEnum());
 }
 
 bool JSObject::hasEnumerableProperty(JSGlobalObject* globalObject, unsigned propertyName) const
@@ -2008,7 +2010,9 @@ bool JSObject::hasEnumerableProperty(JSGlobalObject* globalObject, unsigned prop
     PropertySlot slot(this, PropertySlot::InternalMethodType::GetOwnProperty);
     bool hasProperty = const_cast<JSObject*>(this)->getPropertySlot(globalObject, propertyName, slot);
     RETURN_IF_EXCEPTION(scope, false);
-    return hasProperty && !(slot.attributes() & PropertyAttribute::DontEnum);
+    if (!hasProperty)
+        return false;
+    return !(slot.attributes() & PropertyAttribute::DontEnum) || (slot.slotBase() && slot.slotBase()->structure(vm)->typeInfo().getOwnPropertySlotMayBeWrongAboutDontEnum());
 }
 
 // ECMA 8.6.2.5

Source/JavaScriptCore/runtime/JSTypeInfo.h

@@ -59,6 +59,7 @@ static constexpr unsigned InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNot
 static constexpr unsigned StructureIsImmortal = 1 << 17;
 static constexpr unsigned HasPutPropertySecurityCheck = 1 << 18;
 static constexpr unsigned OverridesGetPrototype = 1 << 19;
+static constexpr unsigned GetOwnPropertySlotMayBeWrongAboutDontEnum = 1 << 20;
 
 static constexpr unsigned numberOfInlineBits = 8;
 static constexpr unsigned OverridesGetPrototypeOutOfLine = OverridesGetPrototype >> numberOfInlineBits;
@@ -105,6 +106,7 @@ class TypeInfo {
     bool prohibitsPropertyCaching() const { return isSetOnFlags2<ProhibitsPropertyCaching>(); }
     bool getOwnPropertySlotIsImpure() const { return isSetOnFlags2<GetOwnPropertySlotIsImpure>(); }
     bool getOwnPropertySlotIsImpureForPropertyAbsence() const { return isSetOnFlags2<GetOwnPropertySlotIsImpureForPropertyAbsence>(); }
+    bool getOwnPropertySlotMayBeWrongAboutDontEnum() const { return isSetOnFlags2<GetOwnPropertySlotMayBeWrongAboutDontEnum>(); }
     bool hasPutPropertySecurityCheck() const { return isSetOnFlags2<HasPutPropertySecurityCheck>(); }
     bool newImpurePropertyFiresWatchpoints() const { return isSetOnFlags2<NewImpurePropertyFiresWatchpoints>(); }
     bool isImmutablePrototypeExoticObject() const { return isSetOnFlags2<IsImmutablePrototypeExoticObject>(); }

Source/WebCore/ChangeLog

@@ -1,3 +1,12 @@
+2021-01-25  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        REGRESSION (r270874): Some React Native apps are reported broken on iOS
+        https://bugs.webkit.org/show_bug.cgi?id=220809
+
+        Reviewed by Saam Barati.
+
+        * bridge/runtime_object.h:
+
 2021-01-25  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, add missing header includes to address build issues.

Source/WebCore/bridge/runtime_object.h

@@ -37,7 +37,7 @@ Exception* throwRuntimeObjectInvalidAccessError(JSGlobalObject*, ThrowScope&);
 class WEBCORE_EXPORT RuntimeObject : public JSNonFinalObject {
 public:
     using Base = JSNonFinalObject;
-    static constexpr unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | OverridesGetOwnPropertyNames | OverridesGetCallData;
+    static constexpr unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | OverridesGetOwnPropertyNames | OverridesGetCallData | GetOwnPropertySlotMayBeWrongAboutDontEnum;
     static constexpr bool needsDestruction = true;
 
     template<typename CellType, JSC::SubspaceAccess>

Source/WebKit/ChangeLog

@@ -1,3 +1,12 @@
+2021-01-25  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        REGRESSION (r270874): Some React Native apps are reported broken on iOS
+        https://bugs.webkit.org/show_bug.cgi?id=220809
+
+        Reviewed by Saam Barati.
+
+        * WebProcess/Plugins/Netscape/JSNPObject.h:
+
 2021-01-25  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, add missing header includes to address build issues.

Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.h

@@ -44,7 +44,7 @@ class NPRuntimeObjectMap;
 class JSNPObject final : public JSC::JSDestructibleObject {
 public:
     using Base = JSC::JSDestructibleObject;
-    static constexpr unsigned StructureFlags = Base::StructureFlags | JSC::OverridesGetOwnPropertySlot | JSC::OverridesGetOwnPropertyNames | JSC::OverridesGetCallData;
+    static constexpr unsigned StructureFlags = Base::StructureFlags | JSC::OverridesGetOwnPropertySlot | JSC::OverridesGetOwnPropertyNames | JSC::OverridesGetCallData | JSC::GetOwnPropertySlotMayBeWrongAboutDontEnum;
 
     template<typename CellType, JSC::SubspaceAccess>
     static JSC::IsoSubspace* subspaceFor(JSC::VM& vm)