Title: [146392] trunk/Source/_javascript_Core
Revision
146392
Author
[email protected]
Date
2013-03-20 14:41:55 -0700 (Wed, 20 Mar 2013)

Log Message

Objective-C API: Fix over-releasing in allocateConstructorAndPrototypeWithSuperClassInfo:
https://bugs.webkit.org/show_bug.cgi?id=112832

Reviewed by Geoffrey Garen.

If either the m_constructor or m_prototype (but not both) is collected, we will call 
allocateConstructorAndPrototypeWithSuperClassInfo, which will create a new object to replace the one 
that was collected, but at the end of the method we call release on both of them. 
This is incorrect since we autorelease the JSValue in the case that the object doesn't need to be 
reallocated. Thus we'll end up overreleasing later during the drain of the autorelease pool.

* API/JSWrapperMap.mm:
(objectWithCustomBrand): We no longer alloc here. We instead call the JSValue valueWithValue class method,
which autoreleases for us.
(-[JSObjCClassInfo allocateConstructorAndPrototypeWithSuperClassInfo:]): We no longer call release on the 
constructor or prototype JSValues.
* API/tests/testapi.mm: Added a new test that crashes on ToT due to over-releasing.

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/API/JSWrapperMap.mm (146391 => 146392)


--- trunk/Source/_javascript_Core/API/JSWrapperMap.mm	2013-03-20 21:36:25 UTC (rev 146391)
+++ trunk/Source/_javascript_Core/API/JSWrapperMap.mm	2013-03-20 21:41:55 UTC (rev 146392)
@@ -135,7 +135,7 @@
 // Make an object that is in all ways a completely vanilla _javascript_ object,
 // other than that it has a native brand set that will be displayed by the default
 // Object.prototype.toString conversion.
-static JSValue *createObjectWithCustomBrand(JSContext *context, NSString *brand, JSClassRef parentClass = 0, Class cls = 0)
+static JSValue *objectWithCustomBrand(JSContext *context, NSString *brand, JSClassRef parentClass = 0, Class cls = 0)
 {
     JSClassDefinition definition;
     definition = kJSClassDefinitionEmpty;
@@ -144,7 +144,7 @@
     JSClassRef classRef = JSClassCreate(&definition);
     JSObjectRef result = makeWrapper([context globalContextRef], classRef, cls);
     JSClassRelease(classRef);
-    return [[JSValue alloc] initWithValue:result inContext:context];
+    return [JSValue valueWithValue:result inContext:context];
 }
 
 // Look for @optional properties in the prototype containing a selector to property
@@ -379,12 +379,12 @@
         if (m_prototype)
             prototype = [JSValue valueWithValue:toRef(m_prototype.get()) inContext:m_context];
         else
-            prototype = createObjectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sPrototype", className]);
+            prototype = objectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sPrototype", className]);
 
         if (m_constructor)
             constructor = [JSValue valueWithValue:toRef(m_constructor.get()) inContext:m_context];
         else
-            constructor = createObjectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sConstructor", className], wrapperClass(), [m_class retain]);
+            constructor = objectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sConstructor", className], wrapperClass(), [m_class retain]);
 
         JSContextRef cContext = [m_context globalContextRef];
         m_prototype = toJS(JSValueToObject(cContext, valueInternalValue(prototype), 0));
@@ -401,9 +401,6 @@
 
         // Set [Prototype].
         JSObjectSetPrototype([m_context globalContextRef], toRef(m_prototype.get()), toRef(superClassInfo->m_prototype.get()));
-
-        [constructor release];
-        [prototype release];
     }
 }
 

Modified: trunk/Source/_javascript_Core/API/tests/testapi.mm (146391 => 146392)


--- trunk/Source/_javascript_Core/API/tests/testapi.mm	2013-03-20 21:36:25 UTC (rev 146391)
+++ trunk/Source/_javascript_Core/API/tests/testapi.mm	2013-03-20 21:41:55 UTC (rev 146392)
@@ -562,6 +562,22 @@
 
     @autoreleasepool {
         JSContext *context = [[JSContext alloc] init];
+        TestObject *testObject = [TestObject testObject];
+        @autoreleasepool {
+            context[@"testObject"] = testObject;
+            [context evaluateScript:@"var constructor = Object.getPrototypeOf(testObject).constructor; constructor.prototype = undefined;"];
+            [context evaluateScript:@"testObject = undefined"];
+        }
+        
+        JSSynchronousGarbageCollectForDebugging([context globalContextRef]);
+
+        @autoreleasepool {
+            context[@"testObject"] = testObject;
+        }
+    }
+
+    @autoreleasepool {
+        JSContext *context = [[JSContext alloc] init];
         TextXYZ *testXYZ = [[TextXYZ alloc] init];
 
         @autoreleasepool {

Modified: trunk/Source/_javascript_Core/ChangeLog (146391 => 146392)


--- trunk/Source/_javascript_Core/ChangeLog	2013-03-20 21:36:25 UTC (rev 146391)
+++ trunk/Source/_javascript_Core/ChangeLog	2013-03-20 21:41:55 UTC (rev 146392)
@@ -1,3 +1,23 @@
+2013-03-20  Mark Hahnenberg  <[email protected]>
+
+        Objective-C API: Fix over-releasing in allocateConstructorAndPrototypeWithSuperClassInfo:
+        https://bugs.webkit.org/show_bug.cgi?id=112832
+
+        Reviewed by Geoffrey Garen.
+
+        If either the m_constructor or m_prototype (but not both) is collected, we will call 
+        allocateConstructorAndPrototypeWithSuperClassInfo, which will create a new object to replace the one 
+        that was collected, but at the end of the method we call release on both of them. 
+        This is incorrect since we autorelease the JSValue in the case that the object doesn't need to be 
+        reallocated. Thus we'll end up overreleasing later during the drain of the autorelease pool.
+
+        * API/JSWrapperMap.mm:
+        (objectWithCustomBrand): We no longer alloc here. We instead call the JSValue valueWithValue class method,
+        which autoreleases for us.
+        (-[JSObjCClassInfo allocateConstructorAndPrototypeWithSuperClassInfo:]): We no longer call release on the 
+        constructor or prototype JSValues.
+        * API/tests/testapi.mm: Added a new test that crashes on ToT due to over-releasing.
+
 2013-03-19  Filip Pizlo  <[email protected]>
 
         It's called "Hash Consing" not "Hash Consting"
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to