Modified: trunk/Source/WebCore/ChangeLog (122493 => 122494)
--- trunk/Source/WebCore/ChangeLog 2012-07-12 19:49:33 UTC (rev 122493)
+++ trunk/Source/WebCore/ChangeLog 2012-07-12 19:59:23 UTC (rev 122494)
@@ -1,3 +1,32 @@
+2012-07-12 Gavin Barraclough <[email protected]>
+
+ 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 <[email protected]>
https://bugs.webkit.org/show_bug.cgi?id=91000
Modified: trunk/Source/WebCore/bindings/objc/WebScriptObject.mm (122493 => 122494)
--- trunk/Source/WebCore/bindings/objc/WebScriptObject.mm 2012-07-12 19:49:33 UTC (rev 122493)
+++ trunk/Source/WebCore/bindings/objc/WebScriptObject.mm 2012-07-12 19:59:23 UTC (rev 122494)
@@ -50,6 +50,7 @@
#import <runtime/JSLock.h>
#import <runtime/Completion.h>
#import <runtime/Completion.h>
+#import <wtf/TCSpinLock.h>
#import <wtf/Threading.h>
@@ -60,16 +61,24 @@
namespace WebCore {
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();
NSMapInsert(JSWrapperCache, impl, wrapper);
@@ -77,18 +86,27 @@
void removeJSWrapper(JSObject* impl)
{
+ SpinLockHolder holder(&spinLock);
+
if (!JSWrapperCache)
return;
NSMapRemove(JSWrapperCache, impl);
}
+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];
}
@@ -148,9 +166,6 @@
_private->rootObject = rootObject.leakRef();
_private->originRootObject = originRootObject.leakRef();
- // NSMap is not thread safe, hold the JSC API lock.
- JSC::JSLockHolder holder(JSDOMWindowBase::commonJSGlobalData());
-
WebCore::addJSWrapper(self, imp);
if (_private->rootObject)
@@ -231,16 +246,10 @@
- (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.
+ if (_private->imp)
+ WebCore::removeJSWrapperIfRetainCountOne(self, _private->imp);
- // 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);
- }
-
[super release];
}
