[webkit-changes] [164088] trunk/Source/JavaScriptCore

[mhahnenberg]

Title: [164088] trunk/Source/_javascript_Core

Revision

164088

Author

mhahnenb...@apple.com

Date

2014-02-13 18:36:44 -0800 (Thu, 13 Feb 2014)

Log Message

JSManagedValue::dealloc modifies NSMapTable while iterating it
https://bugs.webkit.org/show_bug.cgi?id=128713

Reviewed by Geoffrey Garen.

Having to write a test for this revealed a bug in how addManagedReference:withOwner:
actually notifies JSManagedValues of new owners.

* API/JSManagedValue.mm:
(-[JSManagedValue dealloc]):
* API/JSVirtualMachine.mm:
(-[JSVirtualMachine addManagedReference:withOwner:]):
(-[JSVirtualMachine removeManagedReference:withOwner:]):
* API/tests/testapi.mm:
(testObjectiveCAPI):

Modified Paths

• trunk/Source/_javascript_Core/API/JSManagedValue.mm
• trunk/Source/_javascript_Core/API/JSVirtualMachine.mm
• trunk/Source/_javascript_Core/API/tests/testapi.mm
• trunk/Source/_javascript_Core/ChangeLog

Diff

Modified: trunk/Source/_javascript_Core/API/JSManagedValue.mm (164087 => 164088)

--- trunk/Source/_javascript_Core/API/JSManagedValue.mm	2014-02-14 02:32:02 UTC (rev 164087)
+++ trunk/Source/_javascript_Core/API/JSManagedValue.mm	2014-02-14 02:36:44 UTC (rev 164088)
@@ -221,11 +221,13 @@
 {
     JSVirtualMachine *virtualMachine = [[[self value] context] virtualMachine];
     if (virtualMachine) {
-        for (id owner in [m_owners keyEnumerator]) {
+        NSMapTable *copy = [m_owners copy];
+        for (id owner in [copy keyEnumerator]) {
             size_t count = reinterpret_cast<size_t>(NSMapGet(m_owners, owner));
             while (count--)
                 [virtualMachine removeManagedReference:self withOwner:owner];
         }
+        [copy release];
     }
 
     [self disconnectValue];

Modified: trunk/Source/_javascript_Core/API/JSVirtualMachine.mm (164087 => 164088)

--- trunk/Source/_javascript_Core/API/JSVirtualMachine.mm	2014-02-14 02:32:02 UTC (rev 164087)
+++ trunk/Source/_javascript_Core/API/JSVirtualMachine.mm	2014-02-14 02:36:44 UTC (rev 164088)
@@ -148,6 +148,9 @@
 
 - (void)addManagedReference:(id)object withOwner:(id)owner
 {    
+    if ([object isKindOfClass:[JSManagedValue class]])
+        [object didAddOwner:owner];
+        
     object = getInternalObjcObject(object);
     owner = getInternalObjcObject(owner);
     
@@ -166,15 +169,15 @@
         [ownedObjects release];
     }
 
-    if ([object isKindOfClass:[JSManagedValue class]])
-        [object didAddOwner:owner];
-        
     size_t count = reinterpret_cast<size_t>(NSMapGet(ownedObjects, object));
     NSMapInsert(ownedObjects, object, reinterpret_cast<void*>(count + 1));
 }
 
 - (void)removeManagedReference:(id)object withOwner:(id)owner
 {
+    if ([object isKindOfClass:[JSManagedValue class]])
+        [object didRemoveOwner:owner];
+
     object = getInternalObjcObject(object);
     owner = getInternalObjcObject(owner);
     
@@ -196,9 +199,6 @@
     if (count == 1)
         NSMapRemove(ownedObjects, object);
 
-    if ([object isKindOfClass:[JSManagedValue class]])
-        [object didRemoveOwner:owner];
-
     if (![ownedObjects count])
         [m_externalObjectGraph removeObjectForKey:owner];
 }

Modified: trunk/Source/_javascript_Core/API/tests/testapi.mm (164087 => 164088)

--- trunk/Source/_javascript_Core/API/tests/testapi.mm	2014-02-14 02:32:02 UTC (rev 164087)
+++ trunk/Source/_javascript_Core/API/tests/testapi.mm	2014-02-14 02:36:44 UTC (rev 164088)
@@ -539,6 +539,20 @@
 
     @autoreleasepool {
         JSContext *context = [[JSContext alloc] init];
+        JSValue *message = [JSValue valueWithObject:@"hello" inContext:context];
+        TestObject *rootObject = [TestObject testObject];
+        JSCollection *collection = [[JSCollection alloc] init];
+        context[@"root"] = rootObject;
+        @autoreleasepool {
+            JSValue *jsCollection = [JSValue valueWithObject:collection inContext:context];
+            JSManagedValue *weakCollection = [JSManagedValue managedValueWithValue:jsCollection andOwner:rootObject];
+            [context.virtualMachine addManagedReference:weakCollection withOwner:message];
+            JSSynchronousGarbageCollectForDebugging([context JSGlobalContextRef]);
+        }
+    }
+
+    @autoreleasepool {
+        JSContext *context = [[JSContext alloc] init];
         __block int result;
         context[@"blockCallback"] = ^(int value){
             result = value;

Modified: trunk/Source/_javascript_Core/ChangeLog (164087 => 164088)

--- trunk/Source/_javascript_Core/ChangeLog	2014-02-14 02:32:02 UTC (rev 164087)
+++ trunk/Source/_javascript_Core/ChangeLog	2014-02-14 02:36:44 UTC (rev 164088)
@@ -1,3 +1,21 @@
+2014-02-13  Mark Hahnenberg  <mhahnenb...@apple.com>
+
+        JSManagedValue::dealloc modifies NSMapTable while iterating it
+        https://bugs.webkit.org/show_bug.cgi?id=128713
+
+        Reviewed by Geoffrey Garen.
+
+        Having to write a test for this revealed a bug in how addManagedReference:withOwner:
+        actually notifies JSManagedValues of new owners.
+
+        * API/JSManagedValue.mm:
+        (-[JSManagedValue dealloc]):
+        * API/JSVirtualMachine.mm:
+        (-[JSVirtualMachine addManagedReference:withOwner:]):
+        (-[JSVirtualMachine removeManagedReference:withOwner:]):
+        * API/tests/testapi.mm:
+        (testObjectiveCAPI):
+
 2014-02-13  Filip Pizlo  <fpi...@apple.com>
 
         Unreviewed, fix build.

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes