Changeset 20104 in webkit for trunk/JavaScriptCore/API


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/API
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/JavaScriptCore/API/JSCallbackConstructor.cpp

    r17372 r20104  
    7070        for (int i = 0; i < argumentCount; i++)
    7171            arguments[i] = toRef(args[i]);
     72           
     73        JSLock::DropAllLocks dropAllLocks;
    7274        return toJS(m_callback(ctx, thisRef, argumentCount, arguments, toRef(exec->exceptionSlot())));
    7375    }

  • trunk/JavaScriptCore/API/JSCallbackFunction.cpp

    r17372 r20104  
    5959    for (int i = 0; i < argumentCount; i++)
    6060        arguments[i] = toRef(args[i]);
     61
     62    JSLock::DropAllLocks dropAllLocks;
    6163    return toJS(m_callback(execRef, thisRef, thisObjRef, argumentCount, arguments, toRef(exec->exceptionSlot())));
    6264}

  • trunk/JavaScriptCore/API/JSCallbackObject.cpp

    r17372 r20104  
    6060    // initialize from base to derived
    6161    for (int i = static_cast<int>(initRoutines.size()) - 1; i >= 0; i--) {
     62        JSLock::DropAllLocks dropAllLocks;
    6263        JSObjectInitializeCallback initialize = initRoutines[i];
    6364        initialize(toRef(exec), toRef(this));
    … …  
    7071   
    7172    for (JSClassRef jsClass = m_class; jsClass; jsClass = jsClass->parentClass)
    72         if (JSObjectFinalizeCallback finalize = jsClass->finalize)
     73        if (JSObjectFinalizeCallback finalize = jsClass->finalize) {
     74            JSLock::DropAllLocks dropAllLocks;
    7375            finalize(thisRef);
     76        }
    7477   
    7578    JSClassRelease(m_class);
    … …  
    9396        // optional optimization to bypass getProperty in cases when we only need to know if the property exists
    9497        if (JSObjectHasPropertyCallback hasProperty = jsClass->hasProperty) {
     98            JSLock::DropAllLocks dropAllLocks;
    9599            if (hasProperty(ctx, thisRef, propertyNameRef)) {
    96100                slot.setCustom(this, callbackGetter);
    … …  
    98102            }
    99103        } else if (JSObjectGetPropertyCallback getProperty = jsClass->getProperty) {
     104            JSLock::DropAllLocks dropAllLocks;
    100105            if (JSValueRef value = getProperty(ctx, thisRef, propertyNameRef, toRef(exec->exceptionSlot()))) {
    101106                // cache the value so we don't have to compute it again
    … …  
    139144    for (JSClassRef jsClass = m_class; jsClass; jsClass = jsClass->parentClass) {
    140145        if (JSObjectSetPropertyCallback setProperty = jsClass->setProperty) {
     146            JSLock::DropAllLocks dropAllLocks;
    141147            if (setProperty(ctx, thisRef, propertyNameRef, valueRef, toRef(exec->exceptionSlot())))
    142148                return;
    … …  
    148154                    return;
    149155                if (JSObjectSetPropertyCallback setProperty = entry->setProperty) {
     156                    JSLock::DropAllLocks dropAllLocks;
    150157                    if (setProperty(ctx, thisRef, propertyNameRef, valueRef, toRef(exec->exceptionSlot())))
    151158                        return;
    … …  
    181188    for (JSClassRef jsClass = m_class; jsClass; jsClass = jsClass->parentClass) {
    182189        if (JSObjectDeletePropertyCallback deleteProperty = jsClass->deleteProperty) {
     190            JSLock::DropAllLocks dropAllLocks;
    183191            if (deleteProperty(ctx, thisRef, propertyNameRef, toRef(exec->exceptionSlot())))
    184192                return true;
    … …  
    230238            for (int i = 0; i < argumentCount; i++)
    231239                arguments[i] = toRef(args[i]);
     240            JSLock::DropAllLocks dropAllLocks;
    232241            return toJS(callAsConstructor(execRef, thisRef, argumentCount, arguments, toRef(exec->exceptionSlot())));
    233242        }
    … …  
    253262
    254263    for (JSClassRef jsClass = m_class; jsClass; jsClass = jsClass->parentClass)
    255         if (JSObjectHasInstanceCallback hasInstance = jsClass->hasInstance)
     264        if (JSObjectHasInstanceCallback hasInstance = jsClass->hasInstance) {
     265            JSLock::DropAllLocks dropAllLocks;
    256266            return hasInstance(execRef, thisRef, toRef(value), toRef(exec->exceptionSlot()));
     267        }
    257268
    258269    ASSERT(0); // implementsHasInstance should prevent us from reaching here
    … …  
    282293            for (int i = 0; i < argumentCount; i++)
    283294                arguments[i] = toRef(args[i]);
     295            JSLock::DropAllLocks dropAllLocks;
    284296            return toJS(callAsFunction(execRef, thisRef, thisObjRef, argumentCount, arguments, toRef(exec->exceptionSlot())));
    285297        }
    … …  
    296308
    297309    for (JSClassRef jsClass = m_class; jsClass; jsClass = jsClass->parentClass) {
    298         if (JSObjectGetPropertyNamesCallback getPropertyNames = jsClass->getPropertyNames)
     310        if (JSObjectGetPropertyNamesCallback getPropertyNames = jsClass->getPropertyNames) {
     311            JSLock::DropAllLocks dropAllLocks;
    299312            getPropertyNames(execRef, thisRef, toRef(&propertyNames));
     313        }
    300314
    301315        if (OpaqueJSClass::StaticValuesTable* staticValues = jsClass->staticValues) {
    … …  
    331345
    332346    for (JSClassRef jsClass = m_class; jsClass; jsClass = jsClass->parentClass)
    333         if (JSObjectConvertToTypeCallback convertToType = jsClass->convertToType)
     347        if (JSObjectConvertToTypeCallback convertToType = jsClass->convertToType) {
     348            JSLock::DropAllLocks dropAllLocks;
    334349            if (JSValueRef value = convertToType(ctx, thisRef, kJSTypeNumber, toRef(exec->exceptionSlot())))
    335350                return toJS(value)->getNumber();
     351        }
    336352
    337353    return JSObject::toNumber(exec);
    … …  
    344360
    345361    for (JSClassRef jsClass = m_class; jsClass; jsClass = jsClass->parentClass)
    346         if (JSObjectConvertToTypeCallback convertToType = jsClass->convertToType)
     362        if (JSObjectConvertToTypeCallback convertToType = jsClass->convertToType) {
     363            JSLock::DropAllLocks dropAllLocks;
    347364            if (JSValueRef value = convertToType(ctx, thisRef, kJSTypeString, toRef(exec->exceptionSlot())))
    348365                return toJS(value)->getString();
     366        }
    349367
    350368    return JSObject::toString(exec);
    … …  
    388406        if (OpaqueJSClass::StaticValuesTable* staticValues = jsClass->staticValues)
    389407            if (StaticValueEntry* entry = staticValues->get(propertyName.ustring().rep()))
    390                 if (JSObjectGetPropertyCallback getProperty = entry->getProperty)
     408                if (JSObjectGetPropertyCallback getProperty = entry->getProperty) {
     409                    JSLock::DropAllLocks dropAllLocks;
    391410                    if (JSValueRef value = getProperty(toRef(exec), thisRef, propertyNameRef, toRef(exec->exceptionSlot())))
    392411                        return toJS(value);
     412                }
    393413
    394414    return throwError(exec, ReferenceError, "Static value property defined with NULL getProperty callback.");
    … …  
    427447
    428448    for (JSClassRef jsClass = thisObj->m_class; jsClass; jsClass = jsClass->parentClass)
    429         if (JSObjectGetPropertyCallback getProperty = jsClass->getProperty)
     449        if (JSObjectGetPropertyCallback getProperty = jsClass->getProperty) {
     450            JSLock::DropAllLocks dropAllLocks;
    430451            if (JSValueRef value = getProperty(toRef(exec), thisRef, propertyNameRef, toRef(exec->exceptionSlot())))
    431452                return toJS(value);
     453        }
    432454
    433455    return throwError(exec, ReferenceError, "hasProperty callback returned true for a property that doesn't exist.");
Note: See TracChangeset for help on using the changeset viewer.