Skip to content

Commit 02b1e5b

Browse files
webkit-commit-queuewebkit-commit-queue
committed
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 Canonical link: https://commits.webkit.org/211222@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244323 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 6251bcc commit 02b1e5b

7 files changed

Lines changed: 30 additions & 104 deletions

File tree

‎Source/JavaScriptCore/API/JSContext.mm‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -369,11 +369,6 @@ - (JSValue *)wrapperForJSObject:(JSValueRef)value
369369
return [[self wrapperMap] objcWrapperForJSValueRef:value inContext:self];
370370
}
371371

372-
- (void)removeWrapper:(JSValue *)value
373-
{
374-
return [[self wrapperMap] removeWrapper:value];
375-
}
376-
377372
+ (JSContext *)contextWithJSGlobalContextRef:(JSGlobalContextRef)globalContext
378373
{
379374
JSContext *context = (__bridge JSContext *)toJSGlobalObject(globalContext)->apiWrapper();

‎Source/JavaScriptCore/API/JSContextInternal.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ struct CallbackData {
5454
- (JSWrapperMap *)wrapperMap;
5555
- (JSValue *)wrapperForObjCObject:(id)object;
5656
- (JSValue *)wrapperForJSObject:(JSValueRef)value;
57-
- (void)removeWrapper:(JSValue *)value;
5857

5958
@end
6059

‎Source/JavaScriptCore/API/JSValue.mm‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ @implementation JSValue {
7171

7272
- (void)dealloc
7373
{
74-
[_context removeWrapper:self];
7574
JSValueUnprotect([_context JSGlobalContextRef], m_value);
7675
[_context release];
7776
_context = nil;
@@ -1076,7 +1075,6 @@ - (JSValue *)initWithValue:(JSValueRef)value inContext:(JSContext *)context
10761075
if (!self)
10771076
return nil;
10781077

1079-
ASSERT(context);
10801078
_context = [context retain];
10811079
m_value = value;
10821080
JSValueProtect([_context JSGlobalContextRef], m_value);

‎Source/JavaScriptCore/API/JSWrapperMap.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,6 @@
3737

3838
- (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value inContext:(JSContext *)context;
3939

40-
- (void)removeWrapper:(JSValue *)wrapper;
41-
4240
@end
4341

4442
id tryUnwrapObjcObject(JSGlobalContextRef, JSValueRef);

‎Source/JavaScriptCore/API/JSWrapperMap.mm‎

Lines changed: 12 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -581,77 +581,10 @@ - (ConstructorPrototypePair)allocateConstructorAndPrototypeInContext:(JSContext
581581

582582
@end
583583

584-
struct WrapperKey {
585-
static constexpr uintptr_t hashTableDeletedValue() { return 1; }
586-
587-
WrapperKey() = default;
588-
589-
explicit WrapperKey(WTF::HashTableDeletedValueType)
590-
: m_wrapper(reinterpret_cast<JSValue *>(hashTableDeletedValue()))
591-
{
592-
}
593-
594-
explicit WrapperKey(JSValue *wrapper)
595-
: m_wrapper(wrapper)
596-
{
597-
}
598-
599-
bool isHashTableDeletedValue() const
600-
{
601-
return reinterpret_cast<uintptr_t>(m_wrapper) == hashTableDeletedValue();
602-
}
603-
604-
__unsafe_unretained JSValue *m_wrapper { nil };
605-
606-
struct Hash {
607-
static unsigned hash(const WrapperKey& key)
608-
{
609-
return DefaultHash<JSValueRef>::Hash::hash([key.m_wrapper JSValueRef]);
610-
}
611-
612-
static bool equal(const WrapperKey& lhs, const WrapperKey& rhs)
613-
{
614-
return lhs.m_wrapper == rhs.m_wrapper;
615-
}
616-
617-
static const bool safeToCompareToEmptyOrDeleted = false;
618-
};
619-
620-
struct Traits : public SimpleClassHashTraits<WrapperKey> {
621-
static const bool hasIsEmptyValueFunction = true;
622-
static bool isEmptyValue(const WrapperKey& key)
623-
{
624-
return key.m_wrapper == nullptr;
625-
}
626-
};
627-
628-
struct Translator {
629-
struct ValueAndContext {
630-
__unsafe_unretained JSContext *m_context;
631-
JSValueRef m_value;
632-
};
633-
634-
static unsigned hash(const ValueAndContext& value)
635-
{
636-
return DefaultHash<JSValueRef>::Hash::hash(value.m_value);
637-
}
638-
639-
static bool equal(const WrapperKey& lhs, const ValueAndContext& value)
640-
{
641-
return [lhs.m_wrapper JSValueRef] == value.m_value;
642-
}
643-
644-
static void translate(WrapperKey& result, const ValueAndContext& value, unsigned)
645-
{
646-
result = WrapperKey([[[JSValue alloc] initWithValue:value.m_value inContext:value.m_context] autorelease]);
647-
}
648-
};
649-
};
650-
651584
@implementation JSWrapperMap {
652585
NSMutableDictionary *m_classMap;
653586
std::unique_ptr<JSC::WeakGCMap<__unsafe_unretained id, JSC::JSObject>> m_cachedJSWrappers;
654-
HashSet<WrapperKey, WrapperKey::Hash, WrapperKey::Traits> m_cachedObjCWrappers;
587+
NSMapTable *m_cachedObjCWrappers;
655588
}
656589

657590
- (instancetype)initWithGlobalContextRef:(JSGlobalContextRef)context
@@ -660,6 +593,10 @@ - (instancetype)initWithGlobalContextRef:(JSGlobalContextRef)context
660593
if (!self)
661594
return nil;
662595

596+
NSPointerFunctionsOptions keyOptions = NSPointerFunctionsOpaqueMemory | NSPointerFunctionsOpaquePersonality;
597+
NSPointerFunctionsOptions valueOptions = NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPersonality;
598+
m_cachedObjCWrappers = [[NSMapTable alloc] initWithKeyOptions:keyOptions valueOptions:valueOptions capacity:0];
599+
663600
m_cachedJSWrappers = std::make_unique<JSC::WeakGCMap<__unsafe_unretained id, JSC::JSObject>>(toJS(context)->vm());
664601

665602
ASSERT(!toJSGlobalObject(context)->wrapperMap());
@@ -670,6 +607,7 @@ - (instancetype)initWithGlobalContextRef:(JSGlobalContextRef)context
670607

671608
- (void)dealloc
672609
{
610+
[m_cachedObjCWrappers release];
673611
[m_classMap release];
674612
[super dealloc];
675613
}
@@ -724,14 +662,12 @@ - (JSValue *)jsWrapperForObject:(id)object inContext:(JSContext *)context
724662
- (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value inContext:context
725663
{
726664
ASSERT(toJSGlobalObject([context JSGlobalContextRef])->wrapperMap() == self);
727-
WrapperKey::Translator::ValueAndContext valueAndContext { context, value };
728-
auto addResult = m_cachedObjCWrappers.add<WrapperKey::Translator>(valueAndContext);
729-
return addResult.iterator->m_wrapper;
730-
}
731-
732-
- (void)removeWrapper:(JSValue *)wrapper
733-
{
734-
m_cachedObjCWrappers.remove(WrapperKey(wrapper));
665+
JSValue *wrapper = (__bridge JSValue *)NSMapGet(m_cachedObjCWrappers, value);
666+
if (!wrapper) {
667+
wrapper = [[[JSValue alloc] initWithValue:value inContext:context] autorelease];
668+
NSMapInsert(m_cachedObjCWrappers, value, (__bridge void*)wrapper);
669+
}
670+
return wrapper;
735671
}
736672

737673
@end

‎Source/JavaScriptCore/API/tests/testapi.mm‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -565,27 +565,11 @@ static void runJITThreadLimitTests()
565565
static void testObjectiveCAPIMain()
566566
{
567567
@autoreleasepool {
568-
JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
569-
JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
568+
JSVirtualMachine* vm = [[JSVirtualMachine alloc] init];
569+
JSContext* context = [[JSContext alloc] initWithVirtualMachine:vm];
570570
[context evaluateScript:@"bad"];
571571
}
572572

573-
@autoreleasepool {
574-
JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
575-
JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
576-
JSValue *number1 = [context evaluateScript:@"42092389"];
577-
JSValue *number2 = [context evaluateScript:@"42092389"];
578-
checkResult(@"wrapper cache for numbers", number1 == number2 && number1.isNumber && [number1 toInt32] == 42092389);
579-
}
580-
581-
@autoreleasepool {
582-
JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
583-
JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
584-
JSValue *object1 = [context evaluateScript:@"({})"];
585-
JSValue *object2 = [context evaluateScript:@"({})"];
586-
checkResult(@"wrapper cache for objects", object1 != object2);
587-
}
588-
589573
@autoreleasepool {
590574
JSContext *context = [[JSContext alloc] init];
591575
JSValue *result = [context evaluateScript:@"2 + 2"];

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
2019-04-15 Commit Queue <commit-queue@webkit.org>
2+
3+
Unreviewed, rolling out r243672.
4+
https://bugs.webkit.org/show_bug.cgi?id=196952
5+
6+
[JSValue release] should be thread-safe (Requested by
7+
yusukesuzuki on #webkit).
8+
9+
Reverted changeset:
10+
11+
"[JSC] JSWrapperMap should not use Objective-C Weak map
12+
(NSMapTable with NSPointerFunctionsWeakMemory) for
13+
m_cachedObjCWrappers"
14+
https://bugs.webkit.org/show_bug.cgi?id=196392
15+
https://trac.webkit.org/changeset/243672
16+
117
2019-04-15 Saam barati <sbarati@apple.com>
218

319
SafeToExecute for GetByOffset/GetGetterByOffset/PutByOffset is using the wrong child for the base

0 commit comments

Comments
 (0)