JSWrapperMap should check if existing prototype properties are wrappers when copying...
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 May 2019 01:23:39 +0000 (01:23 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 May 2019 01:23:39 +0000 (01:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197324
<rdar://problem/50253144>

Reviewed by Saam Barati.

The current implementation prevents using JSExport to shadow a
method from a super class. This was because we would only add a
method if the prototype didn't already claim to have the
property. Normally this would only happen if an Objective-C super
class already exported a ObjCCallbackFunction for the method,
however, if the user exports a property that is already on
Object.prototype the overriden method won't be exported.

This patch fixes the object prototype issue by checking if the
property on the prototype chain is an ObjCCallbackFunction, if
it's not then it adds an override.

* API/JSWrapperMap.mm:
(copyMethodsToObject):
* API/tests/testapi.mm:
(-[ToStringClass toString]):
(-[ToStringClass other]):
(-[ToStringSubclass toString]):
(-[ToStringSubclassNoProtocol toString]):
(testToString):
(testObjectiveCAPI):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244999 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/API/JSWrapperMap.mm
Source/JavaScriptCore/API/tests/testapi.mm
Source/JavaScriptCore/ChangeLog

index 201cb1d..d469b75 100644 (file)
@@ -268,7 +268,16 @@ static void copyMethodsToObject(JSContext *context, Class objcClass, Protocol *p
             name = renameMap[name];
             if (!name)
                 name = selectorToPropertyName(nameCStr);
-            if ([object hasProperty:name])
+            auto exec = toJS([context JSGlobalContextRef]);
+            JSValue *existingMethod = object[name];
+            // ObjCCallbackFunction does a dynamic lookup for the
+            // selector before calling the method. In order to save
+            // memory we only put one callback object in any givin
+            // prototype chain. However, to handle the client trying
+            // to override normal builtins e.g. "toString" we check if
+            // the existing value on the prototype chain is an ObjC
+            // callback already.
+            if ([existingMethod isObject] && JSC::jsDynamicCast<JSC::ObjCCallbackFunction*>(exec->vm(), toJS(exec, [existingMethod JSValueRef])))
                 return;
             JSObjectRef method = objCCallbackFunctionForMethod(context, objcClass, protocol, isInstanceMethod, sel, types);
             if (method)
index e27a339..5649677 100644 (file)
@@ -2535,6 +2535,62 @@ static void testJSScriptURL()
     }
 }
 
+
+@protocol ToString <JSExport>
+- (NSString *)toString;
+@end
+
+@interface ToStringClass : NSObject<ToString>
+@end
+
+@implementation ToStringClass
+- (NSString *)toString
+{
+    return @"foo";
+}
+@end
+
+@interface ToStringSubclass : ToStringClass<ToString>
+@end
+
+@implementation ToStringSubclass
+- (NSString *)toString
+{
+    return @"baz";
+}
+@end
+
+@interface ToStringSubclassNoProtocol : ToStringClass
+@end
+
+@implementation ToStringSubclassNoProtocol
+- (NSString *)toString
+{
+    return @"baz";
+}
+@end
+
+static void testToString()
+{
+    @autoreleasepool {
+        JSContext *context = [[JSContext alloc] init];
+
+        JSValue *toStringClass = [JSValue valueWithObject:[[ToStringClass alloc] init] inContext:context];
+        checkResult(@"exporting a property with the same name as a builtin on Object.prototype should still be exported", [[toStringClass invokeMethod:@"toString" withArguments:@[]] isEqualToObject:@"foo"]);
+        checkResult(@"converting an object with an exported custom toObject property should use that method", [[toStringClass toString] isEqualToString:@"foo"]);
+
+        toStringClass = [JSValue valueWithObject:[[ToStringSubclass alloc] init] inContext:context];
+        checkResult(@"Calling a method on a derived class should call the derived implementation", [[toStringClass invokeMethod:@"toString" withArguments:@[]] isEqualToObject:@"baz"]);
+        checkResult(@"Converting an object with an exported custom toObject property should use that method", [[toStringClass toString] isEqualToString:@"baz"]);
+        context[@"toStringValue"] = toStringClass;
+        JSValue *hasOwnProperty = [context evaluateScript:@"toStringValue.__proto__.hasOwnProperty('toString')"];
+        checkResult(@"A subclass that exports a method exported by a super class shouldn't have a duplicate prototype method", [hasOwnProperty toBool]);
+
+        toStringClass = [JSValue valueWithObject:[[ToStringSubclassNoProtocol alloc] init] inContext:context];
+        checkResult(@"Calling a method on a derived class should call the derived implementation even when not exported on the derived class", [[toStringClass invokeMethod:@"toString" withArguments:@[]] isEqualToObject:@"baz"]);
+    }
+}
+
 #define RUN(test) do { \
         if (!shouldRun(#test)) \
             break; \
@@ -2555,6 +2611,7 @@ void testObjectiveCAPI(const char* filter)
 
     RUN(checkNegativeNSIntegers());
     RUN(runJITThreadLimitTests());
+    RUN(testToString());
 
     RUN(testLoaderResolvesAbsoluteScriptURL());
     RUN(testFetch());
index e2538ac..a8db599 100644 (file)
@@ -1,3 +1,33 @@
+2019-05-06  Keith Miller  <keith_miller@apple.com>
+
+        JSWrapperMap should check if existing prototype properties are wrappers when copying exported methods.
+        https://bugs.webkit.org/show_bug.cgi?id=197324
+        <rdar://problem/50253144>
+
+        Reviewed by Saam Barati.
+
+        The current implementation prevents using JSExport to shadow a
+        method from a super class. This was because we would only add a
+        method if the prototype didn't already claim to have the
+        property. Normally this would only happen if an Objective-C super
+        class already exported a ObjCCallbackFunction for the method,
+        however, if the user exports a property that is already on
+        Object.prototype the overriden method won't be exported.
+
+        This patch fixes the object prototype issue by checking if the
+        property on the prototype chain is an ObjCCallbackFunction, if
+        it's not then it adds an override.
+
+        * API/JSWrapperMap.mm:
+        (copyMethodsToObject):
+        * API/tests/testapi.mm:
+        (-[ToStringClass toString]):
+        (-[ToStringClass other]):
+        (-[ToStringSubclass toString]):
+        (-[ToStringSubclassNoProtocol toString]):
+        (testToString):
+        (testObjectiveCAPI):
+
 2019-05-06  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] We should check OOM for description string of Symbol