Changeset 122494 in webkit

Timestamp:

Jul 12, 2012 12:59:23 PM (12 years ago)

Author:

barraclough@apple.com

Message:

Threadsafety issues in WebScriptObject
​https://bugs.webkit.org/show_bug.cgi?id=90849

Reviewed by Filip Pizlo & Oliver Hunt.

Updated fix for this bug. Taking the JSC API lock from WebScriptObject::release
may not be safe; better to just guard the JSWrapperCache with its own spinlock.

• bindings/objc/WebScriptObject.mm:

(WebCore::getJSWrapper):

• Added spinlock; also retain/autorelease the returned wrapper - it is unsafe to wait for the caller to do so, due to a race condition vs release removing the wrapper from the map.

(WebCore::addJSWrapper):

• Take the spinlock guarding the cache.

(WebCore::removeJSWrapper):

• Take the spinlock guarding the cache.

(WebCore::removeJSWrapperIfRetainCountOne):

• Take the spinlock guarding the cache, remove the wrapper if retainCount is one.

(WebCore::createJSWrapper):

• Remove the API lock; this method no longer needs to retain/autorelease (this is done by getJSWrapper).

(-[WebScriptObject _setImp:originRootObject:rootObject:]):

• Remove the API lock.

(-[WebScriptObject release]):

• Remove the API lock, retainCount check moved into removeJSWrapperIfRetainCountOne.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/objc/WebScriptObject.mm (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-07-12  Gavin Barraclough  <barraclough@apple.com>
+
+        Threadsafety issues in WebScriptObject
+        https://bugs.webkit.org/show_bug.cgi?id=90849
+
+        Reviewed by Filip Pizlo & Oliver Hunt.
+
+        Updated fix for this bug. Taking the JSC API lock from WebScriptObject::release
+        may not be safe; better to just guard the JSWrapperCache with its own spinlock.
+
+        * bindings/objc/WebScriptObject.mm:
+        (WebCore::getJSWrapper):
+            - Added spinlock; also retain/autorelease the returned wrapper - it is unsafe
+              to wait for the caller to do so, due to a race condition vs release removing
+              the wrapper from the map.
+        (WebCore::addJSWrapper):
+            - Take the spinlock guarding the cache.
+        (WebCore::removeJSWrapper):
+            - Take the spinlock guarding the cache.
+        (WebCore::removeJSWrapperIfRetainCountOne):
+            - Take the spinlock guarding the cache, remove the wrapper if retainCount is one.
+        (WebCore::createJSWrapper):
+            - Remove the API lock; this method no longer needs to retain/autorelease (this is
+              done by getJSWrapper).
+        (-[WebScriptObject _setImp:originRootObject:rootObject:]):
+            - Remove the API lock.
+        (-[WebScriptObject release]):
+            - Remove the API lock, retainCount check moved into removeJSWrapperIfRetainCountOne.
+
 2012-07-11  David Hyatt  <hyatt@apple.com>
 

trunk/Source/WebCore/bindings/objc/WebScriptObject.mm

@@ -51 +51 @@
 #import <runtime/Completion.h>
 #import <runtime/Completion.h>
+#import <wtf/TCSpinLock.h>
 #import <wtf/Threading.h>
 
@@ -61 +62 @@
 
 static NSMapTable* JSWrapperCache;
+static SpinLock spinLock = SPINLOCK_INITIALIZER;
 
 NSObject* getJSWrapper(JSObject* impl)
 {
+    ASSERT(isMainThread());
+    SpinLockHolder holder(&spinLock);
+
     if (!JSWrapperCache)
         return nil;
-    return static_cast<NSObject*>(NSMapGet(JSWrapperCache, impl));
+    NSObject* wrapper = static_cast<NSObject*>(NSMapGet(JSWrapperCache, impl));
+    return wrapper ? [[wrapper retain] autorelease] : nil;
 }
 
 void addJSWrapper(NSObject* wrapper, JSObject* impl)
 {
+    ASSERT(isMainThread());
+    SpinLockHolder holder(&spinLock);
+
     if (!JSWrapperCache)
         JSWrapperCache = createWrapperCache();
@@ -78 +87 @@
 void removeJSWrapper(JSObject* impl)
 {
+    SpinLockHolder holder(&spinLock);
+
     if (!JSWrapperCache)
         return;
@@ -83 +94 @@
 }
 
+static void removeJSWrapperIfRetainCountOne(NSObject* wrapper, JSObject* impl)
+{
+    SpinLockHolder holder(&spinLock);
+
+    if (!JSWrapperCache)
+        return;
+    if ([wrapper retainCount] == 1)
+        NSMapRemove(JSWrapperCache, impl);
+}
+
 id createJSWrapper(JSC::JSObject* object, PassRefPtr<JSC::Bindings::RootObject> origin, PassRefPtr<JSC::Bindings::RootObject> root)
 {
-    // NSMap is not thread safe, hold the JSC API lock; also synchronize this vs. release.
-    JSC::JSLockHolder holder(JSDOMWindowBase::commonJSGlobalData());
-
     if (id wrapper = getJSWrapper(object))
-        return [[wrapper retain] autorelease];
+        return wrapper;
     return [[[WebScriptObject alloc] _initWithJSObject:object originRootObject:origin rootObject:root] autorelease];
 }
@@ -149 +167 @@
     _private->originRootObject = originRootObject.leakRef();
 
-    // NSMap is not thread safe, hold the JSC API lock.
-    JSC::JSLockHolder holder(JSDOMWindowBase::commonJSGlobalData());
-
     WebCore::addJSWrapper(self, imp);
 
@@ -232 +247 @@
 - (oneway void)release
 {
-    {
-        // NSMap is not thread safe, hold the JSC API lock; also synchronize this vs. getJSWrapper.
-        JSC::JSLockHolder holder(JSDOMWindowBase::commonJSGlobalData());
-
-        // If we're releasing the last reference to this object, remove if from the map,
-        // this will prevent this object from being returned by getJSWrapper.
-        if (_private->imp && [self retainCount] == 1)
-            WebCore::removeJSWrapper(_private->imp);
-    }
+    // If we're releasing the last reference to this object, remove if from the map.
+    if (_private->imp)
+        WebCore::removeJSWrapperIfRetainCountOne(self, _private->imp);
 
     [super release];