Title: [244999] trunk/Source/_javascript_Core
- Revision
- 244999
- Author
- keith_mil...@apple.com
- Date
- 2019-05-06 18:23:39 -0700 (Mon, 06 May 2019)
Log Message
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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/API/JSWrapperMap.mm (244998 => 244999)
--- trunk/Source/_javascript_Core/API/JSWrapperMap.mm 2019-05-07 01:03:01 UTC (rev 244998)
+++ trunk/Source/_javascript_Core/API/JSWrapperMap.mm 2019-05-07 01:23:39 UTC (rev 244999)
@@ -268,7 +268,16 @@
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)
Modified: trunk/Source/_javascript_Core/API/tests/testapi.mm (244998 => 244999)
--- trunk/Source/_javascript_Core/API/tests/testapi.mm 2019-05-07 01:03:01 UTC (rev 244998)
+++ trunk/Source/_javascript_Core/API/tests/testapi.mm 2019-05-07 01:23:39 UTC (rev 244999)
@@ -2535,6 +2535,62 @@
}
}
+
+@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 @@
RUN(checkNegativeNSIntegers());
RUN(runJITThreadLimitTests());
+ RUN(testToString());
RUN(testLoaderResolvesAbsoluteScriptURL());
RUN(testFetch());
Modified: trunk/Source/_javascript_Core/ChangeLog (244998 => 244999)
--- trunk/Source/_javascript_Core/ChangeLog 2019-05-07 01:03:01 UTC (rev 244998)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-05-07 01:23:39 UTC (rev 244999)
@@ -1,3 +1,33 @@
+2019-05-06 Keith Miller <keith_mil...@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 <ysuz...@apple.com>
[JSC] We should check OOM for description string of Symbol
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
