Attachment 369211 Details for Bug 197324 – Patch for landing

[patch] Patch for landing

bug-197324-20190506174440.patch (text/plain), 5.95 KB, created by Keith Miller on 2019-05-06 17:44:42 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2019-05-06 17:44:42 PDT

Size: 5.95 KB

patch

obsolete

>Subversion Revision: 244699
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 6114aa50feaf3826d4e386dcbcbe476a8d24d40d..e395dcacdbf390d4f4a956e37b993cfe14e220fc 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2019-04-30  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-04-26  Don Olmstead  <don.olmstead@sony.com>
> 
>         Add WTF::findIgnoringASCIICaseWithoutLength to replace strcasestr
>diff --git a/Source/JavaScriptCore/API/JSWrapperMap.mm b/Source/JavaScriptCore/API/JSWrapperMap.mm
>index 201cb1df9c5f333fc2984b54e266c21d49bee9cb..d469b75a31f0efca9d07abb03b0be66161b39567 100644
>--- a/Source/JavaScriptCore/API/JSWrapperMap.mm
>+++ b/Source/JavaScriptCore/API/JSWrapperMap.mm
>@@ -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)
>diff --git a/Source/JavaScriptCore/API/tests/testapi.mm b/Source/JavaScriptCore/API/tests/testapi.mm
>index e27a3392a38af1d9c076148b805212bb83b35edd..5649677bdb017e254c09676d63b6a1711330c0a1 100644
>--- a/Source/JavaScriptCore/API/tests/testapi.mm
>+++ b/Source/JavaScriptCore/API/tests/testapi.mm
>@@ -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());

Actions: View | Formatted Diff | Diff

Attachments on bug 197324: 368567 | 368569 | 368577 | 369211