Unreviewed, rolling out r243672.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Apr 2019 02:12:26 +0000 (02:12 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Apr 2019 02:12:26 +0000 (02:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196952

[JSValue release] should be thread-safe (Requested by
yusukesuzuki on #webkit).

Reverted changeset:

"[JSC] JSWrapperMap should not use Objective-C Weak map
(NSMapTable with NSPointerFunctionsWeakMemory) for
m_cachedObjCWrappers"
https://bugs.webkit.org/show_bug.cgi?id=196392
https://trac.webkit.org/changeset/243672

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

Source/JavaScriptCore/API/JSContext.mm
Source/JavaScriptCore/API/JSContextInternal.h
Source/JavaScriptCore/API/JSValue.mm
Source/JavaScriptCore/API/JSWrapperMap.h
Source/JavaScriptCore/API/JSWrapperMap.mm
Source/JavaScriptCore/API/tests/testapi.mm
Source/JavaScriptCore/ChangeLog

index 5a85f30..094c937 100644 (file)
     return [[self wrapperMap] objcWrapperForJSValueRef:value inContext:self];
 }
 
-- (void)removeWrapper:(JSValue *)value
-{
-    return [[self wrapperMap] removeWrapper:value];
-}
-
 + (JSContext *)contextWithJSGlobalContextRef:(JSGlobalContextRef)globalContext
 {
     JSContext *context = (__bridge JSContext *)toJSGlobalObject(globalContext)->apiWrapper();
index 6b1ac57..958c479 100644 (file)
@@ -54,7 +54,6 @@ struct CallbackData {
 - (JSWrapperMap *)wrapperMap;
 - (JSValue *)wrapperForObjCObject:(id)object;
 - (JSValue *)wrapperForJSObject:(JSValueRef)value;
-- (void)removeWrapper:(JSValue *)value;
 
 @end
 
index 32e587f..86e10cf 100644 (file)
@@ -71,7 +71,6 @@ NSString * const JSPropertyDescriptorSetKey = @"set";
 
 - (void)dealloc
 {
-    [_context removeWrapper:self];
     JSValueUnprotect([_context JSGlobalContextRef], m_value);
     [_context release];
     _context = nil;
@@ -1076,7 +1075,6 @@ JSValueRef valueInternalValue(JSValue * value)
     if (!self)
         return nil;
 
-    ASSERT(context);
     _context = [context retain];
     m_value = value;
     JSValueProtect([_context JSGlobalContextRef], m_value);
index 08cef54..6c18c64 100644 (file)
@@ -37,8 +37,6 @@
 
 - (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value inContext:(JSContext *)context;
 
-- (void)removeWrapper:(JSValue *)wrapper;
-
 @end
 
 id tryUnwrapObjcObject(JSGlobalContextRef, JSValueRef);
index 82e1d68..201cb1d 100644 (file)
@@ -581,77 +581,10 @@ typedef std::pair<JSC::JSObject*, JSC::JSObject*> ConstructorPrototypePair;
 
 @end
 
-struct WrapperKey {
-    static constexpr uintptr_t hashTableDeletedValue() { return 1; }
-
-    WrapperKey() = default;
-
-    explicit WrapperKey(WTF::HashTableDeletedValueType)
-        : m_wrapper(reinterpret_cast<JSValue *>(hashTableDeletedValue()))
-    {
-    }
-
-    explicit WrapperKey(JSValue *wrapper)
-        : m_wrapper(wrapper)
-    {
-    }
-
-    bool isHashTableDeletedValue() const
-    {
-        return reinterpret_cast<uintptr_t>(m_wrapper) == hashTableDeletedValue();
-    }
-
-    __unsafe_unretained JSValue *m_wrapper { nil };
-
-    struct Hash {
-        static unsigned hash(const WrapperKey& key)
-        {
-            return DefaultHash<JSValueRef>::Hash::hash([key.m_wrapper JSValueRef]);
-        }
-
-        static bool equal(const WrapperKey& lhs, const WrapperKey& rhs)
-        {
-            return lhs.m_wrapper == rhs.m_wrapper;
-        }
-
-        static const bool safeToCompareToEmptyOrDeleted = false;
-    };
-
-    struct Traits : public SimpleClassHashTraits<WrapperKey> {
-        static const bool hasIsEmptyValueFunction = true;
-        static bool isEmptyValue(const WrapperKey& key)
-        {
-            return key.m_wrapper == nullptr;
-        }
-    };
-
-    struct Translator {
-        struct ValueAndContext {
-            __unsafe_unretained JSContext *m_context;
-            JSValueRef m_value;
-        };
-
-        static unsigned hash(const ValueAndContext& value)
-        {
-            return DefaultHash<JSValueRef>::Hash::hash(value.m_value);
-        }
-
-        static bool equal(const WrapperKey& lhs, const ValueAndContext& value)
-        {
-            return [lhs.m_wrapper JSValueRef] == value.m_value;
-        }
-
-        static void translate(WrapperKey& result, const ValueAndContext& value, unsigned)
-        {
-            result = WrapperKey([[[JSValue alloc] initWithValue:value.m_value inContext:value.m_context] autorelease]);
-        }
-    };
-};
-
 @implementation JSWrapperMap {
     NSMutableDictionary *m_classMap;
     std::unique_ptr<JSC::WeakGCMap<__unsafe_unretained id, JSC::JSObject>> m_cachedJSWrappers;
-    HashSet<WrapperKey, WrapperKey::Hash, WrapperKey::Traits> m_cachedObjCWrappers;
+    NSMapTable *m_cachedObjCWrappers;
 }
 
 - (instancetype)initWithGlobalContextRef:(JSGlobalContextRef)context
@@ -660,6 +593,10 @@ struct WrapperKey {
     if (!self)
         return nil;
 
+    NSPointerFunctionsOptions keyOptions = NSPointerFunctionsOpaqueMemory | NSPointerFunctionsOpaquePersonality;
+    NSPointerFunctionsOptions valueOptions = NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPersonality;
+    m_cachedObjCWrappers = [[NSMapTable alloc] initWithKeyOptions:keyOptions valueOptions:valueOptions capacity:0];
+
     m_cachedJSWrappers = std::make_unique<JSC::WeakGCMap<__unsafe_unretained id, JSC::JSObject>>(toJS(context)->vm());
 
     ASSERT(!toJSGlobalObject(context)->wrapperMap());
@@ -670,6 +607,7 @@ struct WrapperKey {
 
 - (void)dealloc
 {
+    [m_cachedObjCWrappers release];
     [m_classMap release];
     [super dealloc];
 }
@@ -724,14 +662,12 @@ struct WrapperKey {
 - (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value inContext:context
 {
     ASSERT(toJSGlobalObject([context JSGlobalContextRef])->wrapperMap() == self);
-    WrapperKey::Translator::ValueAndContext valueAndContext { context, value };
-    auto addResult = m_cachedObjCWrappers.add<WrapperKey::Translator>(valueAndContext);
-    return addResult.iterator->m_wrapper;
-}
-
-- (void)removeWrapper:(JSValue *)wrapper
-{
-    m_cachedObjCWrappers.remove(WrapperKey(wrapper));
+    JSValue *wrapper = (__bridge JSValue *)NSMapGet(m_cachedObjCWrappers, value);
+    if (!wrapper) {
+        wrapper = [[[JSValue alloc] initWithValue:value inContext:context] autorelease];
+        NSMapInsert(m_cachedObjCWrappers, value, (__bridge void*)wrapper);
+    }
+    return wrapper;
 }
 
 @end
index ac6dc19..5ad486f 100644 (file)
@@ -565,28 +565,12 @@ static void runJITThreadLimitTests()
 static void testObjectiveCAPIMain()
 {
     @autoreleasepool {
-        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
-        JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
+        JSVirtualMachinevm = [[JSVirtualMachine alloc] init];
+        JSContextcontext = [[JSContext alloc] initWithVirtualMachine:vm];
         [context evaluateScript:@"bad"];
     }
 
     @autoreleasepool {
-        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
-        JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
-        JSValue *number1 = [context evaluateScript:@"42092389"];
-        JSValue *number2 = [context evaluateScript:@"42092389"];
-        checkResult(@"wrapper cache for numbers", number1 == number2 && number1.isNumber && [number1 toInt32] == 42092389);
-    }
-
-    @autoreleasepool {
-        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
-        JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
-        JSValue *object1 = [context evaluateScript:@"({})"];
-        JSValue *object2 = [context evaluateScript:@"({})"];
-        checkResult(@"wrapper cache for objects", object1 != object2);
-    }
-
-    @autoreleasepool {
         JSContext *context = [[JSContext alloc] init];
         JSValue *result = [context evaluateScript:@"2 + 2"];
         checkResult(@"2 + 2", result.isNumber && [result toInt32] == 4);
index 059087f..501ef26 100644 (file)
@@ -1,3 +1,19 @@
+2019-04-15  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r243672.
+        https://bugs.webkit.org/show_bug.cgi?id=196952
+
+        [JSValue release] should be thread-safe (Requested by
+        yusukesuzuki on #webkit).
+
+        Reverted changeset:
+
+        "[JSC] JSWrapperMap should not use Objective-C Weak map
+        (NSMapTable with NSPointerFunctionsWeakMemory) for
+        m_cachedObjCWrappers"
+        https://bugs.webkit.org/show_bug.cgi?id=196392
+        https://trac.webkit.org/changeset/243672
+
 2019-04-15  Saam barati  <sbarati@apple.com>
 
         SafeToExecute for GetByOffset/GetGetterByOffset/PutByOffset is using the wrong child for the base