Changeset 20104 in webkit for trunk/JavaScriptCore/kjs


Ignore:
Timestamp:
Mar 11, 2007, 4:57:11 PM (18 years ago)
Author:
ggaren
Message:

JavaScriptCore:

Reviewed by Darin Adler.

Fixed <rdar://problem/4587763> PAC file: lock inversion between QT and
JSCore causes a hang @ www.panoramas.dk


With a PAC file, run-webkit-tests --threaded passes, the reported site
works, and all the Quicktime/JavaScript and Flash/JavaScript examples
I found through Google work, too.


Any time JavaScript causes arbitrary non-JavaScript code to execute, it
risks deadlock, because that code may block, trying to acquire a lock
owned by a thread that is waiting to execute JavaScript. In this case,
the thread was a networking thread that was waiting to interpret a PAC file.


Because non-JavaScript code may execute in response to, well, anything,
a perfect solution to this problem is impossible. I've implemented an
optimistic solution, instead: JavaScript will drop its lock whenever it
makes a direct call to non-JavaScript code through a bridging/plug-in API,
but will blissfully ignore the indirect ways it may cause non-JavaScript
code to run (resizing a window, for example).


Unfortunately, this solution introduces significant locking overhead in
the bridging APIs. I don't see a way around that.

This patch includes some distinct bug fixes I saw along the way:


  • bindings/objc/objc_instance.mm: Fixed a bug where a nested begin() call would leak its autorelease pool, because it would NULL out _pool without draining it.
  • bindings/runtime_object.cpp: (RuntimeObjectImp::methodGetter): Don't copy an Identifier to ASCII only to turn around and make an Identifier from the ASCII. In an earlier version of this patch, the copy caused an assertion failure. Now it's just unnecessary work. (RuntimeObjectImp::getOwnPropertySlot): ditto
  • bindings/objc/objc_instance.h: Removed overrides of setVAlueOfField and getValueOfField, because they did exactly what the base class versions did. Removed overrides of Noncopyable declarations for the same reason.
  • bindings/runtime.h: Inherit from Noncopyable instead of rolling our own.
  • bindings/c/c_instance.h: ditto

And the actual patch:


  • API/JSCallbackConstructor.cpp: Drop all locks when calling out to C. (KJS::JSCallbackConstructor::construct):
  • API/JSCallbackFunction.cpp: ditto (KJS::JSCallbackFunction::callAsFunction):
  • API/JSCallbackObject.cpp: ditto (KJS::JSCallbackObject::init): (KJS::JSCallbackObject::~JSCallbackObject): (KJS::JSCallbackObject::getOwnPropertySlot): (KJS::JSCallbackObject::put): (KJS::JSCallbackObject::deleteProperty): (KJS::JSCallbackObject::construct): (KJS::JSCallbackObject::hasInstance): (KJS::JSCallbackObject::callAsFunction): (KJS::JSCallbackObject::getPropertyNames): (KJS::JSCallbackObject::toNumber): (KJS::JSCallbackObject::toString): (KJS::JSCallbackObject::staticValueGetter): (KJS::JSCallbackObject::callbackGetter):


  • bindings/c/c_instance.cpp: Drop all locks when calling out to C. (KJS::Bindings::CInstance::invokeMethod): (KJS::Bindings::CInstance::invokeDefaultMethod):
  • bindings/c/c_runtime.cpp: Drop all locks when calling out to C. (KJS::Bindings::CField::valueFromInstance): (KJS::Bindings::CField::setValueToInstance):
  • bindings/jni/jni_objc.mm: (KJS::Bindings::dispatchJNICall): Drop all locks when calling out to Java.
  • bindings/objc/objc_instance.mm: The changes here are to accomodate the fact that C++ unwinding of DropAllLocks goes crazy when you put it inside a @try block. I moved all JavaScript stuff outside of the @try blocks, and then prefixed the whole blocks with DropAllLocks objects. This required some supporting changes in other functions, which now acquire the JSLock for themselves, intead of relying on their callers to do so. (ObjcInstance::end): (ObjcInstance::invokeMethod): (ObjcInstance::invokeDefaultMethod): (ObjcInstance::setValueOfUndefinedField): (ObjcInstance::getValueOfUndefinedField):
  • bindings/objc/objc_runtime.mm: Same as above, except I didn't want to change throwError to acquire the JSLock for itself. (ObjcField::valueFromInstance): (ObjcField::setValueToInstance):
  • bindings/objc/objc_utility.mm: Supporting changes mentioned above. (KJS::Bindings::convertValueToObjcValue): (KJS::Bindings::convertObjcValueToValue):
  • kjs/JSLock.cpp: (1) Fixed DropAllLocks to behave as advertised, and drop the JSLock only if the current thread actually acquired it in the first place. This is important because WebKit needs to ensure that the JSLock has been dropped before it makes a plug-in call, even though it doesn't know if the current thread actually acquired the JSLock. (We don't want WebKit to accidentally drop a lock belonging to *another thread*.) (2) Used the new per-thread code written for (1) to make recursive calls to JSLock very cheap. JSLock now knows to call pthread_mutext_lock/ pthread_mutext_unlock only at nesting level 0. (KJS::createDidLockJSMutex): (KJS::JSLock::lock): (KJS::JSLock::unlock): (KJS::DropAllLocks::DropAllLocks): (KJS::DropAllLocks::~DropAllLocks): (KJS::JSLock::lockCount):
  • kjs/JSLock.h: Don't duplicate Noncopyable. (KJS::JSLock::~JSLock):
  • wtf/Assertions.h: Blind attempt at helping the Windows build.

WebCore:

Reviewed by Darin Adler.

Fixed <rdar://problem/4587763> PAC file: lock inversion between QT and
JSCore causes a hang @ www.panoramas.dk


See JavaScriptCore ChangeLog for details.


  • bindings/objc/WebScriptObject.mm: (_didExecute): Added helpful ASSERT. (+[WebScriptObject throwException:]): Added missing JSLock.

WebKit:

Reviewed by Darin Adler.

Fixed <rdar://problem/4587763> PAC file: lock inversion between QT and
JSCore causes a hang @ www.panoramas.dk


See JavaScriptCore ChangeLog for details.

Drop the JSLock before making calls through the plug-in API from functions
that may have been called by JavaScript.


  • Plugins/WebBaseNetscapePluginView.mm: (-[WebBaseNetscapePluginView sendEvent:]): (-[WebBaseNetscapePluginView setWindowIfNecessary]): (-[WebBaseNetscapePluginView initWithFrame:pluginPackage:URL:baseURL:MIMEType:attributeKeys:attributeValues:loadManually:DOMElement:]): (-[WebBaseNetscapePluginView createPluginScriptableObject]): (-[WebBaseNetscapePluginView evaluateJavaScriptPluginRequest:]): (-[WebBaseNetscapePluginView webFrame:didFinishLoadWithReason:]): (-[WebBaseNetscapePluginView loadPluginRequest:]): (-[WebBaseNetscapePluginView _printedPluginBitmap]):
  • Plugins/WebPluginController.mm: (+[WebPluginController plugInViewWithArguments:fromPluginPackage:]): (-[WebPluginController startAllPlugins]): (-[WebPluginController stopAllPlugins]): (-[WebPluginController addPlugin:]): (-[WebPluginController destroyPlugin:]): (-[WebPluginController destroyAllPlugins]):
Location:
trunk/JavaScriptCore/kjs
Files:
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/JavaScriptCore/kjs/JSLock.cpp

    r13089 r20104  
    3030#if USE(MULTIPLE_THREADS)
    3131
    32 static pthread_once_t interpreterLockOnce = PTHREAD_ONCE_INIT;
    33 static pthread_mutex_t interpreterLock;
    34 static int interpreterLockCount = 0;
     32// Acquire this mutex before accessing lock-related data.
     33static pthread_mutex_t JSMutex = PTHREAD_MUTEX_INITIALIZER;
    3534
    36 static void initializeJSLock()
     35// Thread-specific key that tells whether a thread holds the JSMutex.
     36pthread_key_t didLockJSMutex;
     37
     38// Lock nesting count.
     39static int JSLockCount;
     40
     41static void createDidLockJSMutex()
    3742{
    38   pthread_mutexattr_t attr;
    39 
    40   pthread_mutexattr_init(&attr);
    41   pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_RECURSIVE);
    42 
    43   pthread_mutex_init(&interpreterLock, &attr);
     43    pthread_key_create(&didLockJSMutex, 0);
    4444}
     45pthread_once_t createDidLockJSMutexOnce = PTHREAD_ONCE_INIT;
    4546
    4647void JSLock::lock()
    4748{
    48   pthread_once(&interpreterLockOnce, initializeJSLock);
    49   pthread_mutex_lock(&interpreterLock);
    50   interpreterLockCount++;
    51   Collector::registerThread();
     49    pthread_once(&createDidLockJSMutexOnce, createDidLockJSMutex);
     50
     51    if (!pthread_getspecific(didLockJSMutex)) {
     52        pthread_mutex_lock(&JSMutex);
     53        pthread_setspecific(didLockJSMutex, &didLockJSMutex);
     54    }
     55    ++JSLockCount;
     56
     57    Collector::registerThread();
    5258}
    5359
    5460void JSLock::unlock()
    5561{
    56   interpreterLockCount--;
    57   pthread_mutex_unlock(&interpreterLock);
     62    ASSERT(JSLockCount);
     63    ASSERT(!!pthread_getspecific(didLockJSMutex));
     64
     65    --JSLockCount;
     66    if (!JSLockCount) {
     67        pthread_setspecific(didLockJSMutex, 0);
     68        pthread_mutex_unlock(&JSMutex);
     69    }
     70}
     71
     72JSLock::DropAllLocks::DropAllLocks()
     73    : m_lockCount(0)
     74{
     75    pthread_once(&createDidLockJSMutexOnce, createDidLockJSMutex);
     76
     77    m_lockCount = !!pthread_getspecific(didLockJSMutex) ? JSLock::lockCount() : 0;
     78    for (int i = 0; i < m_lockCount; i++)
     79        JSLock::unlock();
     80}
     81
     82JSLock::DropAllLocks::~DropAllLocks()
     83{
     84    for (int i = 0; i < m_lockCount; i++)
     85        JSLock::lock();
     86    m_lockCount = 0;
    5887}
    5988
    … …  
    6291// If threading support is off, set the lock count to a constant value of 1 so assertions
    6392// that the lock is held don't fail
    64 const int interpreterLockCount = 1;
     93const int JSLockCount = 1;
    6594
    6695void JSLock::lock()
    … …  
    72101}
    73102
    74 #endif
    75 
    76 int JSLock::lockCount()
    77 {
    78     return interpreterLockCount;
    79 }
    80        
    81103JSLock::DropAllLocks::DropAllLocks()
    82104{
    83     int lockCount = JSLock::lockCount();
    84     for (int i = 0; i < lockCount; i++) {
    85         JSLock::unlock();
    86     }
    87     m_lockCount = lockCount;
    88105}
    89106
    90107JSLock::DropAllLocks::~DropAllLocks()
    91108{
    92     int lockCount = m_lockCount;
    93     for (int i = 0; i < lockCount; i++) {
    94         JSLock::lock();
    95     }
     109}
     110
     111#endif // USE(MULTIPLE_THREADS)
     112
     113int JSLock::lockCount()
     114{
     115    return JSLockCount;
    96116}
    97117

  • trunk/JavaScriptCore/kjs/JSLock.h

    r12301 r20104  
    2424#define KJS_INTERPRETER_LOCK_H
    2525
     26#include <wtf/Assertions.h>
     27#include <wtf/Noncopyable.h>
     28
    2629namespace KJS {
    2730
    28     // to make it safe to use JavaScript on multiple threads, it is
     31    // To make it safe to use JavaScript on multiple threads, it is
    2932    // important to lock before doing anything that allocates a
    3033    // garbage-collected object or which may affect other shared state
    … …  
    3437    // is ok.
    3538
    36     // Sometimes it is necessary to temporarily release the lock -
    37     // since it is recursive you have to actually release all locks
    38     // held by your thread. This is safe to do if you are executing
    39     // code that doesn't require the lock, and reacquire the right
    40     // number of locks at the end. You can do this by constructing a
    41     // locally scoped JSLock::DropAllLocks object.
     39    // To avoid deadlock, sometimes it is necessary to temporarily
     40    // release the lock. Since it is recursive you actually have to
     41    // release all locks held by your thread. This is safe to do if
     42    // you are executing code that doesn't require the lock, and you
     43    // reacquire the right number of locks at the end. You can do this
     44    // by constructing a locally scoped JSLock::DropAllLocks object. The
     45    // DropAllLocks object takes care to release the JSLock only if your
     46    // thread acquired it to begin with.
    4247
    43     class JSLock
    44     {
     48    class JSLock : Noncopyable {
    4549    public:
    4650        JSLock()
    … …  
    4852            lock();
    4953        }
    50         ~JSLock() {
     54
     55        ~JSLock()
     56        {
    5157            unlock();
    5258        }
    … …  
    5561        static void unlock();
    5662        static int lockCount();
    57        
    58         class DropAllLocks {
     63
     64        class DropAllLocks : Noncopyable {
    5965        public:
    6066            DropAllLocks();
    6167            ~DropAllLocks();
     68           
    6269        private:
    6370            int m_lockCount;
    64            
    65             DropAllLocks(const DropAllLocks&);
    66             DropAllLocks& operator=(const DropAllLocks&);
    6771        };
    68        
    69     private:
    70         JSLock(const JSLock&);
    71         JSLock& operator=(const JSLock&);
    7272    };
    7373
Note: See TracChangeset for help on using the changeset viewer.