Attachment 368567 Details for Bug 197324 – Patch

[patch] Patch

bug-197324-20190430083301.patch (text/plain), 6.20 KB, created by Keith Miller on 2019-04-30 08:33:12 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2019-04-30 08:33:12 PDT

Size: 6.20 KB

patch

obsolete

>Subversion Revision: 244699
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 6114aa50feaf3826d4e386dcbcbe476a8d24d40d..e152ff153ff3e53c1017f0159ed9712dd95a711e 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2019-04-29  Keith Miller  <keith_miller@apple.com>
>+
>+        JSWrapperMap should only look for own properties when copying methods.
>+        https://bugs.webkit.org/show_bug.cgi?id=197324
>+        <rdar://problem/50253144>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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 a Objective-C super
>+        class already exported a ObjCCallbackFunction for the method,
>+        however, if the user exports a property that is already on
>+        Object.protype 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..d794325b290606a7b734b1b08ee1d6cfd1912cc1 100644
>--- a/Source/JavaScriptCore/API/JSWrapperMap.mm
>+++ b/Source/JavaScriptCore/API/JSWrapperMap.mm
>@@ -268,7 +268,18 @@ 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]);
>+            auto jsObject = JSC::jsCast<JSC::JSObject*>(toJS(exec, [object JSValueRef]));
>+            JSC::PropertySlot slot(jsObject, JSC::PropertySlot::InternalMethodType::Get);
>+            bool hasProperty = jsObject->getPropertySlot(exec, JSC::Identifier::fromString(exec, String(name)), slot);
>+            // 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 (hasProperty && slot.isValue() && JSC::jsDynamicCast<JSC::ObjCCallbackFunction*>(exec->vm(), slot.getPureResult()))
>                 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..e2fd2eeb6c9c35ba971fc4b34a618e00729cb4e2 100644
>--- a/Source/JavaScriptCore/API/tests/testapi.mm
>+++ b/Source/JavaScriptCore/API/tests/testapi.mm
>@@ -2535,6 +2535,67 @@ static void testJSScriptURL()
>     }
> }
> 
>+
>+@protocol ToString <JSExport>
>+- (NSString *)toString;
>+- (BOOL)other;
>+@end
>+
>+@interface ToStringClass : NSObject<ToString>
>+@end
>+
>+@implementation ToStringClass
>+- (NSString *)toString
>+{
>+    return @"foo";
>+}
>+- (BOOL)other
>+{
>+    return YES;
>+}
>+@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 +2616,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