Changeset 122198 in webkit


Ignore:
Timestamp:
Jul 10, 2012 12:08:53 AM (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.

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.
Location:
trunk/Source/WebCore
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r122197 r122198  
     12012-07-09  Gavin Barraclough  <barraclough@apple.com>
     2
     3        Threadsafety issues in WebScriptObject
     4        https://bugs.webkit.org/show_bug.cgi?id=90849
     5
     6        Reviewed by Filip Pizlo.
     7
     8        WebScriptObject maintains a NSMap of wrapper objects. A race condition exists
     9        between a wrapper being retrieved from the map, and being released - if the
     10        final release on an object is called between a call to getJSWrapper and the
     11        subsequent retain, we may end up with a stale object reference.
     12
     13        We can make this safe by hoisting the removal from the map from delloc up into
     14        release (if the retainCount is 1), and locking release against retrieval from
     15        the map. Since release may be called from another thread, and NSMap is not
     16        threadsafe, we'd better lock around all access to the map (this fix already
     17        necessitates get & remove to be locked, so this just adds 'add', too).
     18
     19        * bindings/objc/WebScriptObject.mm:
     20        (WebCore::createJSWrapper):
     21            - lock around getJSWrapper, retain.
     22        (-[WebScriptObject _setImp:originRootObject:rootObject:]):
     23            - lock around addJSWrapper.
     24        (-[WebScriptObject release]):
     25            - Added; removeJSWrapper for last release, lock & synchronized vs. getJSWrapper.
     26        (-[WebScriptObject dealloc]):
     27            - removeJSWrapper call hoisted into release.
     28
    1292012-07-09  Christophe Dumez  <christophe.dumez@intel.com>
    230
  • trunk/Source/WebCore/bindings/objc/WebScriptObject.mm

    r121381 r122198  
    8585id createJSWrapper(JSC::JSObject* object, PassRefPtr<JSC::Bindings::RootObject> origin, PassRefPtr<JSC::Bindings::RootObject> root)
    8686{
     87    // NSMap is not thread safe, hold the JSC API lock; also synchronize this vs. release.
     88    JSC::JSLockHolder holder(JSDOMWindowBase::commonJSGlobalData());
     89
    8790    if (id wrapper = getJSWrapper(object))
    8891        return [[wrapper retain] autorelease];
     
    146149    _private->originRootObject = originRootObject.leakRef();
    147150
     151    // NSMap is not thread safe, hold the JSC API lock.
     152    JSC::JSLockHolder holder(JSDOMWindowBase::commonJSGlobalData());
     153
    148154    WebCore::addJSWrapper(self, imp);
    149155
     
    224230}
    225231
     232- (oneway void)release
     233{
     234    {
     235        // NSMap is not thread safe, hold the JSC API lock; also synchronize this vs. getJSWrapper.
     236        JSC::JSLockHolder holder(JSDOMWindowBase::commonJSGlobalData());
     237
     238        // If we're releasing the last reference to this object, remove if from the map,
     239        // this will prevent this object from being returned by getJSWrapper.
     240        if (_private->imp && [self retainCount] == 1)
     241            WebCore::removeJSWrapper(_private->imp);
     242    }
     243
     244    [super release];
     245}
     246
    226247- (void)dealloc
    227248{
    228249    if (WebCoreObjCScheduleDeallocateOnMainThread([WebScriptObject class], self))
    229250        return;
    230 
    231     if (_private->imp)
    232         WebCore::removeJSWrapper(_private->imp);
    233251
    234252    if (_private->rootObject && _private->rootObject->isValid())
Note: See TracChangeset for help on using the changeset viewer.