Changeset 210458 in webkit

Timestamp:

Jan 6, 2017 3:38:31 PM (7 years ago)

Author:

mark.lam@apple.com

Message:

The ObjC API's JSVirtualMachine's map tables need to be guarded by a lock.
​https://bugs.webkit.org/show_bug.cgi?id=166778
<rdar://problem/29761198>

Reviewed by Filip Pizlo.

Now that we have a concurrent GC, access to JSVirtualMachine's
m_externalObjectGraph and m_externalRememberedSet need to be guarded by a lock
since both the GC marker thread and the mutator thread may access them at the
same time.

• API/JSVirtualMachine.mm:

(-[JSVirtualMachine addExternalRememberedObject:]):
(-[JSVirtualMachine addManagedReference:withOwner:]):
(-[JSVirtualMachine removeManagedReference:withOwner:]):
(-[JSVirtualMachine externalDataMutex]):
(scanExternalObjectGraph):
(scanExternalRememberedSet):

• API/JSVirtualMachineInternal.h:
• Deleted externalObjectGraph method. There's no need to expose this.

Location:

trunk/Source/JavaScriptCore

Files:

• API/JSVirtualMachine.mm (modified) (10 diffs)
• API/JSVirtualMachineInternal.h (modified) (2 diffs)
• ChangeLog (modified) (1 diff)

trunk/Source/JavaScriptCore/API/JSVirtualMachine.mm

@@ -1 +1 @@
 /*
- * Copyright (C) 2013-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -82 +82 @@
 @implementation JSVirtualMachine {
     JSContextGroupRef m_group;
+    Lock m_externalDataMutex;
     NSMapTable *m_contextCache;
     NSMapTable *m_externalObjectGraph;
@@ -157 +158 @@
 - (void)addExternalRememberedObject:(id)object
 {
+    auto locker = holdLock(m_externalDataMutex);
     ASSERT([self isOldExternalObject:object]);
     [m_externalRememberedSet setObject:@YES forKey:object];
@@ -176 +178 @@
         [self addExternalRememberedObject:owner];
  
+    auto externalDataMutexLocker = holdLock(m_externalDataMutex);
     NSMapTable *ownedObjects = [m_externalObjectGraph objectForKey:owner];
     if (!ownedObjects) {
@@ -203 +206 @@
     JSC::JSLockHolder locker(toJS(m_group));
     
+    auto externalDataMutexLocker = holdLock(m_externalDataMutex);
     NSMapTable *ownedObjects = [m_externalObjectGraph objectForKey:owner];
     if (!ownedObjects)
@@ -249 +253 @@
 }
 
+- (Lock&)externalDataMutex
+{
+    return m_externalDataMutex;
+}
+
 - (NSMapTable *)externalObjectGraph
 {
@@ -261 +270 @@
 @end
 
-void scanExternalObjectGraph(JSC::VM& vm, JSC::SlotVisitor& visitor, void* root)
+static void scanExternalObjectGraph(JSC::VM& vm, JSC::SlotVisitor& visitor, void* root, bool lockAcquired)
 {
     @autoreleasepool {
@@ -268 +277 @@
             return;
         NSMapTable *externalObjectGraph = [virtualMachine externalObjectGraph];
+        Lock& externalDataMutex = [virtualMachine externalDataMutex];
         Vector<void*> stack;
         stack.append(root);
@@ -276 +286 @@
                 continue;
             visitor.addOpaqueRoot(nextRoot);
-            
-            NSMapTable *ownedObjects = [externalObjectGraph objectForKey:static_cast<id>(nextRoot)];
-            for (id ownedObject in ownedObjects)
-                stack.append(static_cast<void*>(ownedObject));
+
+            auto appendOwnedObjects = [&] {
+                NSMapTable *ownedObjects = [externalObjectGraph objectForKey:static_cast<id>(nextRoot)];
+                for (id ownedObject in ownedObjects)
+                    stack.append(static_cast<void*>(ownedObject));
+            };
+
+            if (lockAcquired)
+                appendOwnedObjects();
+            else {
+                auto locker = holdLock(externalDataMutex);
+                appendOwnedObjects();
+            }
         }
     }
+}
+
+void scanExternalObjectGraph(JSC::VM& vm, JSC::SlotVisitor& visitor, void* root)
+{
+    bool lockAcquired = false;
+    scanExternalObjectGraph(vm, visitor, root, lockAcquired);
 }
 
@@ -290 +315 @@
         if (!virtualMachine)
             return;
+        Lock& externalDataMutex = [virtualMachine externalDataMutex];
+        auto locker = holdLock(externalDataMutex);
         NSMapTable *externalObjectGraph = [virtualMachine externalObjectGraph];
         NSMapTable *externalRememberedSet = [virtualMachine externalRememberedSet];
         for (id key in externalRememberedSet) {
             NSMapTable *ownedObjects = [externalObjectGraph objectForKey:key];
+            bool lockAcquired = true;
             for (id ownedObject in ownedObjects)
-                scanExternalObjectGraph(vm, visitor, ownedObject);
+                scanExternalObjectGraph(vm, visitor, ownedObject, lockAcquired);
         }
         [externalRememberedSet removeAllObjects];

trunk/Source/JavaScriptCore/API/JSVirtualMachineInternal.h

@@ -1 +1 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -48 +48 @@
 - (void)addContext:(JSContext *)wrapper forGlobalContextRef:(JSGlobalContextRef)globalContext;
 
-- (NSMapTable *)externalObjectGraph;
-
 @end
 #endif // defined(__OBJC__)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-01-06  Mark Lam  <mark.lam@apple.com>
+
+        The ObjC API's JSVirtualMachine's map tables need to be guarded by a lock.
+        https://bugs.webkit.org/show_bug.cgi?id=166778
+        <rdar://problem/29761198>
+
+        Reviewed by Filip Pizlo.
+
+        Now that we have a concurrent GC, access to JSVirtualMachine's
+        m_externalObjectGraph and m_externalRememberedSet need to be guarded by a lock
+        since both the GC marker thread and the mutator thread may access them at the
+        same time.
+
+        * API/JSVirtualMachine.mm:
+        (-[JSVirtualMachine addExternalRememberedObject:]):
+        (-[JSVirtualMachine addManagedReference:withOwner:]):
+        (-[JSVirtualMachine removeManagedReference:withOwner:]):
+        (-[JSVirtualMachine externalDataMutex]):
+        (scanExternalObjectGraph):
+        (scanExternalRememberedSet):
+
+        * API/JSVirtualMachineInternal.h:
+        - Deleted externalObjectGraph method.  There's no need to expose this.
+
 2017-01-06  Michael Saboff  <msaboff@apple.com>