Changeset 87588 in webkit

Timestamp:

May 27, 2011 5:28:56 PM (13 years ago)

Author:

ggaren@apple.com

Message:

2011-05-27 Geoffrey Garen <​ggaren@apple.com>

Reviewed by Oliver Hunt.

JS API is too aggressive about throwing exceptions for NULL get or set operations
​https://bugs.webkit.org/show_bug.cgi?id=61678

• API/JSCallbackObject.h: Changed our staticValueGetter to a regular function that returns a JSValue, so it can fail and still forward to normal property lookup.

• API/JSCallbackObjectFunctions.h: (JSC::::getOwnPropertySlot): Don't throw an exception when failing to access a static property -- just forward the access. This allows objects to observe get/set operations but still let the JS object manage lifetime.

(JSC::::put): Ditto.

(JSC::::getStaticValue): Same as JSCallbackObject.h.

• API/tests/testapi.c: (MyObject_set_nullGetForwardSet):
• API/tests/testapi.js: Updated tests to reflect slightly less strict behavior, which matches headerdoc claims.

Location:

trunk/Source/JavaScriptCore

Files:

• API/JSCallbackObject.h (modified) (1 diff)
• API/JSCallbackObjectFunctions.h (modified) (4 diffs)
• API/tests/testapi.c (modified) (1 diff)
• API/tests/testapi.js (modified) (2 diffs)
• ChangeLog (modified) (1 diff)

trunk/Source/JavaScriptCore/API/JSCallbackObject.h

@@ -189 +189 @@
     static EncodedJSValue JSC_HOST_CALL construct(ExecState*);
    
-    static JSValue staticValueGetter(ExecState*, JSValue, const Identifier&);
+    JSValue getStaticValue(ExecState*, const Identifier&);
     static JSValue staticFunctionGetter(ExecState*, JSValue, const Identifier&);
     static JSValue callbackGetter(ExecState*, JSValue, const Identifier&);

trunk/Source/JavaScriptCore/API/JSCallbackObjectFunctions.h

@@ -150 +150 @@
         if (OpaqueJSClassStaticValuesTable* staticValues = jsClass->staticValues(exec)) {
             if (staticValues->contains(propertyName.impl())) {
-                slot.setCustom(this, staticValueGetter);
-                return true;
+                JSValue value = getStaticValue(exec, propertyName);
+                if (value) {
+                    slot.setValue(value);
+                    return true;
+                }
             }
         }
@@ -226 +229 @@
                     if (result || exception)
                         return;
-                } else
-                    throwError(exec, createReferenceError(exec, "Attempt to set a property that is not settable."));
+                }
             }
         }
@@ -516 +518 @@
 
 template <class Base>
-JSValue JSCallbackObject<Base>::staticValueGetter(ExecState* exec, JSValue slotBase, const Identifier& propertyName)
-{
-    JSCallbackObject* thisObj = asCallbackObject(slotBase);
-    
-    JSObjectRef thisRef = toRef(thisObj);
+JSValue JSCallbackObject<Base>::getStaticValue(ExecState* exec, const Identifier& propertyName)
+{
+    JSObjectRef thisRef = toRef(this);
     RefPtr<OpaqueJSString> propertyNameRef;
     
-    for (JSClassRef jsClass = thisObj->classRef(); jsClass; jsClass = jsClass->parentClass)
+    for (JSClassRef jsClass = classRef(); jsClass; jsClass = jsClass->parentClass)
         if (OpaqueJSClassStaticValuesTable* staticValues = jsClass->staticValues(exec))
             if (StaticValueEntry* entry = staticValues->get(propertyName.impl()))
@@ -543 +543 @@
                 }
 
-    return throwError(exec, createReferenceError(exec, "Static value property defined with NULL getProperty callback."));
+    return JSValue();
 }
 

trunk/Source/JavaScriptCore/API/tests/testapi.c

@@ -312 +312 @@
 }
 
+static bool MyObject_set_nullGetForwardSet(JSContextRef ctx, JSObjectRef object, JSStringRef propertyName, JSValueRef value, JSValueRef* exception)
+{
+    UNUSED_PARAM(ctx);
+    UNUSED_PARAM(object);
+    UNUSED_PARAM(propertyName);
+    UNUSED_PARAM(value);
+    UNUSED_PARAM(exception);
+    return false; // Forward to parent class.
+}
+
 static JSStaticValue evilStaticValues[] = {
     { "nullGetSet", 0, 0, kJSPropertyAttributeNone },
+    { "nullGetForwardSet", 0, MyObject_set_nullGetForwardSet, kJSPropertyAttributeNone },
     { 0, 0, 0, 0 }
 };

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

@@ -95 +95 @@
 shouldBe("'throwOnHasInstance' instanceof MyObject", "an exception");
 
+MyObject.nullGetForwardSet = 1;
+shouldBe("MyObject.nullGetForwardSet", 1);
+
 var foundMyPropertyName = false;
 var foundRegularType = false;
@@ -163 +166 @@
 shouldBe("(new Object()) instanceof MyObject", false);
 
-shouldThrow("MyObject.nullGetSet = 1");
-shouldThrow("MyObject.nullGetSet");
+MyObject.nullGetSet = 1;
+shouldBe("MyObject.nullGetSet", 1);
 shouldThrow("MyObject.nullCall()");
 shouldThrow("MyObject.hasPropertyLie");

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2011-05-27  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        JS API is too aggressive about throwing exceptions for NULL get or set operations
+        https://bugs.webkit.org/show_bug.cgi?id=61678
+
+        * API/JSCallbackObject.h: Changed our staticValueGetter to a regular
+        function that returns a JSValue, so it can fail and still forward to
+        normal property lookup.
+
+        * API/JSCallbackObjectFunctions.h:
+        (JSC::::getOwnPropertySlot): Don't throw an exception when failing to
+        access a static property -- just forward the access. This allows objects
+        to observe get/set operations but still let the JS object manage lifetime.
+
+        (JSC::::put): Ditto.
+
+        (JSC::::getStaticValue): Same as JSCallbackObject.h.
+
+        * API/tests/testapi.c:
+        (MyObject_set_nullGetForwardSet):
+        * API/tests/testapi.js: Updated tests to reflect slightly less strict
+        behavior, which matches headerdoc claims.
+
 2011-05-27  Geoffrey Garen  <ggaren@apple.com>