Title: [122198] trunk/Source/WebCore
- Revision
- 122198
- Author
- [email protected]
- Date
- 2012-07-10 00:08:53 -0700 (Tue, 10 Jul 2012)
Log Message
Threadsafety issues in WebScriptObject
https://bugs.webkit.org/show_bug.cgi?id=90849
Reviewed by Filip Pizlo.
WebScriptObject maintains a NSMap of wrapper objects. A race condition exists
between a wrapper being retrieved from the map, and being released - if the
final release on an object is called between a call to getJSWrapper and the
subsequent retain, we may end up with a stale object reference.
We can make this safe by hoisting the removal from the map from delloc up into
release (if the retainCount is 1), and locking release against retrieval from
the map. Since release may be called from another thread, and NSMap is not
threadsafe, we'd better lock around all access to the map (this fix already
necessitates get & remove to be locked, so this just adds 'add', too).
* bindings/objc/WebScriptObject.mm:
(WebCore::createJSWrapper):
- lock around getJSWrapper, retain.
(-[WebScriptObject _setImp:originRootObject:rootObject:]):
- lock around addJSWrapper.
(-[WebScriptObject release]):
- Added; removeJSWrapper for last release, lock & synchronized vs. getJSWrapper.
(-[WebScriptObject dealloc]):
- removeJSWrapper call hoisted into release.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (122197 => 122198)
--- trunk/Source/WebCore/ChangeLog 2012-07-10 06:42:40 UTC (rev 122197)
+++ trunk/Source/WebCore/ChangeLog 2012-07-10 07:08:53 UTC (rev 122198)
@@ -1,3 +1,31 @@
+2012-07-09 Gavin Barraclough <[email protected]>
+
+ Threadsafety issues in WebScriptObject
+ https://bugs.webkit.org/show_bug.cgi?id=90849
+
+ Reviewed by Filip Pizlo.
+
+ WebScriptObject maintains a NSMap of wrapper objects. A race condition exists
+ between a wrapper being retrieved from the map, and being released - if the
+ final release on an object is called between a call to getJSWrapper and the
+ subsequent retain, we may end up with a stale object reference.
+
+ We can make this safe by hoisting the removal from the map from delloc up into
+ release (if the retainCount is 1), and locking release against retrieval from
+ the map. Since release may be called from another thread, and NSMap is not
+ threadsafe, we'd better lock around all access to the map (this fix already
+ necessitates get & remove to be locked, so this just adds 'add', too).
+
+ * bindings/objc/WebScriptObject.mm:
+ (WebCore::createJSWrapper):
+ - lock around getJSWrapper, retain.
+ (-[WebScriptObject _setImp:originRootObject:rootObject:]):
+ - lock around addJSWrapper.
+ (-[WebScriptObject release]):
+ - Added; removeJSWrapper for last release, lock & synchronized vs. getJSWrapper.
+ (-[WebScriptObject dealloc]):
+ - removeJSWrapper call hoisted into release.
+
2012-07-09 Christophe Dumez <[email protected]>
[EFL] Battery status code needs refactoring to be reused in WebKit2
Modified: trunk/Source/WebCore/bindings/objc/WebScriptObject.mm (122197 => 122198)
--- trunk/Source/WebCore/bindings/objc/WebScriptObject.mm 2012-07-10 06:42:40 UTC (rev 122197)
+++ trunk/Source/WebCore/bindings/objc/WebScriptObject.mm 2012-07-10 07:08:53 UTC (rev 122198)
@@ -84,6 +84,9 @@
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 [[[WebScriptObject alloc] _initWithJSObject:object originRootObject:origin rootObject:root] autorelease];
@@ -145,6 +148,9 @@
_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)
@@ -223,14 +229,26 @@
return jsCast<JSDOMWindowBase*>(root->globalObject())->allowsAccessFrom(_private->originRootObject->globalObject());
}
+- (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);
+ }
+
+ [super release];
+}
+
- (void)dealloc
{
if (WebCoreObjCScheduleDeallocateOnMainThread([WebScriptObject class], self))
return;
- if (_private->imp)
- WebCore::removeJSWrapper(_private->imp);
-
if (_private->rootObject && _private->rootObject->isValid())
_private->rootObject->gcUnprotect(_private->imp);
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes
