Objective-C API: JSObjCClassInfo creates reference cycle with JSContext
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Jan 2013 00:07:38 +0000 (00:07 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Jan 2013 00:07:38 +0000 (00:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=107839

Reviewed by Oliver Hunt.

JSContext has a JSWrapperMap, which has an NSMutableDictionary m_classMap, which has values that
are JSObjCClassInfo objects, which have strong references to two JSValue *'s, m_prototype and
m_constructor, which in turn have strong references to the JSContext, creating a reference cycle.
We should make m_prototype and m_constructor Weak<JSObject>. This gets rid of the strong reference
to the JSContext and also prevents clients from accidentally creating reference cycles by assigning
to the prototype of the constructor. If Weak<JSObject> fields are ever garbage collected, we will
reallocate them.

* API/JSContext.mm:
(-[JSContext wrapperMap]):
* API/JSContextInternal.h:
* API/JSWrapperMap.mm:
(-[JSObjCClassInfo initWithContext:forClass:superClassInfo:]):
(-[JSObjCClassInfo dealloc]):
(-[JSObjCClassInfo allocateConstructorAndPrototypeWithSuperClassInfo:]):
(-[JSObjCClassInfo allocateConstructorAndPrototype]):
(-[JSObjCClassInfo wrapperForObject:]):
(-[JSObjCClassInfo constructor]):

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

Source/JavaScriptCore/API/JSContext.mm
Source/JavaScriptCore/API/JSContextInternal.h
Source/JavaScriptCore/API/JSWrapperMap.mm
Source/JavaScriptCore/ChangeLog

index 0061b73..99b66e1 100644 (file)
     return [JSValue valueWithValue:result inContext:self];
 }
 
     return [JSValue valueWithValue:result inContext:self];
 }
 
+- (JSWrapperMap *)wrapperMap
+{
+    return m_wrapperMap;
+}
+
 - (JSValue *)globalObject
 {
     return [JSValue valueWithValue:JSContextGetGlobalObject(m_context) inContext:self];
 - (JSValue *)globalObject
 {
     return [JSValue valueWithValue:JSContextGetGlobalObject(m_context) inContext:self];
index 1f555d4..22532c4 100644 (file)
@@ -52,6 +52,8 @@ private:
     JSContext *m_weakContext;
 };
 
     JSContext *m_weakContext;
 };
 
+@class JSWrapperMap;
+
 @interface JSContext(Internal)
 
 JSGlobalContextRef contextInternalContext(JSContext *);
 @interface JSContext(Internal)
 
 JSGlobalContextRef contextInternalContext(JSContext *);
@@ -65,6 +67,8 @@ JSGlobalContextRef contextInternalContext(JSContext *);
 
 - (JSValue *)wrapperForObject:(id)object;
 
 
 - (JSValue *)wrapperForObject:(id)object;
 
+@property (readonly, retain) JSWrapperMap *wrapperMap;
+
 @end
 
 #endif
 @end
 
 #endif
index 9f8ca7f..a268a56 100644 (file)
 #import <wtf/TCSpinLock.h>
 #import "wtf/Vector.h"
 
 #import <wtf/TCSpinLock.h>
 #import "wtf/Vector.h"
 
+@class JSObjCClassInfo;
+
+@interface JSWrapperMap () 
+
+- (JSObjCClassInfo*)classInfoForClass:(Class)cls;
+
+@end
+
 static void wrapperFinalize(JSObjectRef object)
 {
     [(id)JSObjectGetPrivate(object) release];
 static void wrapperFinalize(JSObjectRef object)
 {
     [(id)JSObjectGetPrivate(object) release];
@@ -291,8 +299,8 @@ static void copyPrototypeProperties(JSContext *context, Class objcClass, Protoco
     Class m_class;
     bool m_block;
     JSClassRef m_classRef;
     Class m_class;
     bool m_block;
     JSClassRef m_classRef;
-    JSValue *m_prototype;
-    JSValue *m_constructor;
+    JSC::Weak<JSC::JSObject> m_prototype;
+    JSC::Weak<JSC::JSObject> m_constructor;
 }
 
 - (id)initWithContext:(JSContext *)context forClass:(Class)cls superClassInfo:(JSObjCClassInfo*)superClassInfo;
 }
 
 - (id)initWithContext:(JSContext *)context forClass:(Class)cls superClassInfo:(JSObjCClassInfo*)superClassInfo;
@@ -319,36 +327,60 @@ static void copyPrototypeProperties(JSContext *context, Class objcClass, Protoco
     definition.parentClass = wrapperClass();
     m_classRef = JSClassCreate(&definition);
 
     definition.parentClass = wrapperClass();
     m_classRef = JSClassCreate(&definition);
 
-    ASSERT((cls == [NSObject class]) == !superClassInfo);
+    [self allocateConstructorAndPrototypeWithSuperClassInfo:superClassInfo];
+
+    return self;
+}
+
+- (void)dealloc
+{
+    JSClassRelease(m_classRef);
+    [super dealloc];
+}
+
+- (void)allocateConstructorAndPrototypeWithSuperClassInfo:(JSObjCClassInfo*)superClassInfo
+{
+    ASSERT((m_class == [NSObject class]) == !superClassInfo);
     if (!superClassInfo) {
     if (!superClassInfo) {
-        m_constructor = [context[@"Object"] retain];
-        m_prototype = [m_constructor[@"prototype"] retain];
+        JSContextRef cContext = contextInternalContext(m_context);
+        JSValue *constructor = m_context[@"Object"];
+        m_constructor = toJS(JSValueToObject(cContext, valueInternalValue(constructor), 0));
+
+        JSValue *prototype = constructor[@"prototype"];
+        m_prototype = toJS(JSValueToObject(cContext, valueInternalValue(prototype), 0));
     } else {
     } else {
+        const char* className = class_getName(m_class);
+
         // Create the prototype/constructor pair.
         // Create the prototype/constructor pair.
-        m_prototype = createObjectWithCustomBrand(context, [NSString stringWithFormat:@"%sPrototype", className]);
-        m_constructor = createObjectWithCustomBrand(context, [NSString stringWithFormat:@"%sConstructor", className], wrapperClass(), [cls retain]);
-        putNonEnumerable(m_prototype, @"constructor", m_constructor);
-        putNonEnumerable(m_constructor, @"prototype", m_prototype);
+        JSValue *prototype = createObjectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sPrototype", className]);
+        JSValue *constructor = createObjectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sConstructor", className], wrapperClass(), [m_class retain]);
+
+        JSContextRef cContext = contextInternalContext(m_context);
+        m_prototype = toJS(JSValueToObject(cContext, valueInternalValue(prototype), 0));
+        m_constructor = toJS(JSValueToObject(cContext, valueInternalValue(constructor), 0));
+
+        putNonEnumerable(prototype, @"constructor", constructor);
+        putNonEnumerable(constructor, @"prototype", prototype);
 
         Protocol *exportProtocol = getJSExportProtocol();
 
         Protocol *exportProtocol = getJSExportProtocol();
-        forEachProtocolImplementingProtocol(cls, exportProtocol, ^(Protocol *protocol){
-            copyPrototypeProperties(context, cls, protocol, m_prototype);
-            copyMethodsToObject(context, cls, protocol, NO, m_constructor);
+        forEachProtocolImplementingProtocol(m_class, exportProtocol, ^(Protocol *protocol){
+            copyPrototypeProperties(m_context, m_class, protocol, prototype);
+            copyMethodsToObject(m_context, m_class, protocol, NO, constructor);
         });
 
         // Set [Prototype].
         });
 
         // Set [Prototype].
-        m_prototype[@"__proto__"] = superClassInfo->m_prototype;
-    }
+        prototype[@"__proto__"] = [JSValue valueWithValue:toRef(superClassInfo->m_prototype.get()) inContext:m_context];
 
 
-    return self;
+        [constructor release];
+        [prototype release];
+    }
 }
 
 }
 
-- (void)dealloc
+- (void)allocateConstructorAndPrototype
 {
 {
-    JSClassRelease(m_classRef);
-    [m_prototype release];
-    [m_constructor release];
-    [super dealloc];
+    ASSERT(!m_constructor.get());
+    ASSERT(!m_prototype.get());
+    [self allocateConstructorAndPrototypeWithSuperClassInfo:[m_context.wrapperMap classInfoForClass:class_getSuperclass(m_class)]];
 }
 
 - (JSValue *)wrapperForObject:(id)object
 }
 
 - (JSValue *)wrapperForObject:(id)object
@@ -360,18 +392,27 @@ static void copyPrototypeProperties(JSContext *context, Class objcClass, Protoco
             return [JSValue valueWithValue:method inContext:m_context];
     }
 
             return [JSValue valueWithValue:method inContext:m_context];
     }
 
-    JSValueRef prototypeValue = valueInternalValue(m_prototype);
-    ASSERT(JSValueIsObject(contextInternalContext(m_context), prototypeValue));
-    JSObjectRef prototype = JSValueToObject(contextInternalContext(m_context), prototypeValue, 0);
+    JSC::JSObject* prototype = m_prototype.get();
+    if (!prototype) {
+        [self allocateConstructorAndPrototype];
+        prototype = m_prototype.get();
+        ASSERT(prototype);
+    }
 
     JSObjectRef wrapper = JSObjectMake(contextInternalContext(m_context), m_classRef, [object retain]);
 
     JSObjectRef wrapper = JSObjectMake(contextInternalContext(m_context), m_classRef, [object retain]);
-    JSObjectSetPrototype(contextInternalContext(m_context), wrapper, prototype);
+    JSObjectSetPrototype(contextInternalContext(m_context), wrapper, toRef(prototype));
     return [JSValue valueWithValue:wrapper inContext:m_context];
 }
 
 - (JSValue *)constructor
 {
     return [JSValue valueWithValue:wrapper inContext:m_context];
 }
 
 - (JSValue *)constructor
 {
-    return m_constructor;
+    JSC::JSObject* constructor = m_constructor.get();
+    if (!constructor) {
+        [self allocateConstructorAndPrototype];
+        constructor = m_constructor.get();
+        ASSERT(constructor);
+    }
+    return [JSValue valueWithValue:toRef(constructor) inContext:m_context];
 }
 
 @end
 }
 
 @end
index 8a89904..e47e991 100644 (file)
@@ -1,3 +1,29 @@
+2013-01-29  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        Objective-C API: JSObjCClassInfo creates reference cycle with JSContext
+        https://bugs.webkit.org/show_bug.cgi?id=107839
+
+        Reviewed by Oliver Hunt.
+
+        JSContext has a JSWrapperMap, which has an NSMutableDictionary m_classMap, which has values that 
+        are JSObjCClassInfo objects, which have strong references to two JSValue *'s, m_prototype and 
+        m_constructor, which in turn have strong references to the JSContext, creating a reference cycle. 
+        We should make m_prototype and m_constructor Weak<JSObject>. This gets rid of the strong reference 
+        to the JSContext and also prevents clients from accidentally creating reference cycles by assigning 
+        to the prototype of the constructor. If Weak<JSObject> fields are ever garbage collected, we will 
+        reallocate them.
+
+        * API/JSContext.mm:
+        (-[JSContext wrapperMap]):
+        * API/JSContextInternal.h:
+        * API/JSWrapperMap.mm:
+        (-[JSObjCClassInfo initWithContext:forClass:superClassInfo:]):
+        (-[JSObjCClassInfo dealloc]):
+        (-[JSObjCClassInfo allocateConstructorAndPrototypeWithSuperClassInfo:]):
+        (-[JSObjCClassInfo allocateConstructorAndPrototype]):
+        (-[JSObjCClassInfo wrapperForObject:]):
+        (-[JSObjCClassInfo constructor]):
+
 2013-01-29  Oliver Hunt  <oliver@apple.com>
 
         REGRESSION (r140594): RELEASE_ASSERT_NOT_REACHED in JSC::Interpreter::execute
 2013-01-29  Oliver Hunt  <oliver@apple.com>
 
         REGRESSION (r140594): RELEASE_ASSERT_NOT_REACHED in JSC::Interpreter::execute