Commit 8566d352 authored by ggaren's avatar ggaren

Reviewed by Maciej Stachowiak.

        
        Fixed <rdar://problem/5064964> Repro ASSERT failure in JS Bindings when 
        closing window @ lowtrades.bptrade.com
        
        Unfortunately, the bindings depend on UString and Identifier as string 
        representations. So, they need to acquire the JSLock when doing something
        that will ref/deref their strings.

        Layout tests, the original site, and Java, Flash, and Quicktime on the 
        web work. No leaks reported. No automated test for this because testing 
        the Java bindings, like math, is hard.
        
        * bindings/runtime.h: Made Noncopyable, just to be sure.
        
        * bindings/c/c_class.cpp: 
        (KJS::Bindings::CClass::~CClass): Acquire the JSLock and explicitly clear the keys
        in our hashtable, since they're UString::Reps, and ref/deref aren't thread-safe.
        (KJS::Bindings::CClass::methodsNamed): Also acquire the JSLock when adding
        keys to the table, since the table ref's them.
        (KJS::Bindings::CClass::fieldNamed): ditto.

        * bindings/c/c_utility.cpp: Removed dead function.
        (KJS::Bindings::convertValueToNPVariant): Acquire the JSLock because doing
        it recursively is pretty cheap, and it's just too confusing to tell whether
        all our callers do it for us.
        (KJS::Bindings::convertNPVariantToValue): ditto
        * bindings/c/c_utility.h:

        * bindings/jni/jni_class.cpp: Same deal as c_class.cpp.
        (JavaClass::JavaClass):
        (JavaClass::~JavaClass):

        * bindings/jni/jni_instance.cpp: Same deal as c_utility.cpp.
        (JavaInstance::stringValue):
        * bindings/jni/jni_jsobject.cpp:
        (JavaJSObject::convertValueToJObject):

        * bindings/jni/jni_runtime.cpp:
        (JavaMethod::~JavaMethod): Moved from header, for clarity.
        (appendClassName): Made this static, so the set of callers is known, and
        we can assert that we hold the JSLock. Also changed it to take a UString
        reference, which makes the calling code simpler.
        (JavaMethod::signature): Store the ASCII value we care about instead of
        a UString, since UString is so much more hassle. Hold the JSLock while
        building up the temporary UString.

        * bindings/jni/jni_runtime.h: Nixed dead code in JavaMethod.
        (KJS::Bindings::JavaString::JavaString): Hold a UString::Rep instead of
        a UString, so we can acquire the JSLock and explicitly release it.
        (KJS::Bindings::JavaString::_commonInit):
        (KJS::Bindings::JavaString::~JavaString):
        (KJS::Bindings::JavaString::UTF8String):
        (KJS::Bindings::JavaString::uchars):
        (KJS::Bindings::JavaString::length):
        (KJS::Bindings::JavaString::ustring):

        * bindings/jni/jni_utility.cpp:
        (KJS::Bindings::convertArrayInstanceToJavaArray): Made this static, so 
        the set of callers is known, and we can assert that we hold the JSLock. 
        (KJS::Bindings::convertValueToJValue): Acquire the JSLock because doing
        it recursively is pretty cheap, and it's just too confusing to tell whether
        all our callers do it for us.

        * bindings/objc/objc_runtime.h: Nixed some dead code.
        * bindings/objc/objc_utility.mm:
        (KJS::Bindings::convertNSStringToString): Same drill as above.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@20292 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 6bf2c584
2007-03-15 Geoffrey Garen <ggaren@apple.com>
Reviewed by Maciej Stachowiak.
Fixed <rdar://problem/5064964> Repro ASSERT failure in JS Bindings when
closing window @ lowtrades.bptrade.com
Unfortunately, the bindings depend on UString and Identifier as string
representations. So, they need to acquire the JSLock when doing something
that will ref/deref their strings.
Layout tests, the original site, and Java, Flash, and Quicktime on the
web work. No leaks reported. No automated test for this because testing
the Java bindings, like math, is hard.
* bindings/runtime.h: Made Noncopyable, just to be sure.
* bindings/c/c_class.cpp:
(KJS::Bindings::CClass::~CClass): Acquire the JSLock and explicitly clear the keys
in our hashtable, since they're UString::Reps, and ref/deref aren't thread-safe.
(KJS::Bindings::CClass::methodsNamed): Also acquire the JSLock when adding
keys to the table, since the table ref's them.
(KJS::Bindings::CClass::fieldNamed): ditto.
* bindings/c/c_utility.cpp: Removed dead function.
(KJS::Bindings::convertValueToNPVariant): Acquire the JSLock because doing
it recursively is pretty cheap, and it's just too confusing to tell whether
all our callers do it for us.
(KJS::Bindings::convertNPVariantToValue): ditto
* bindings/c/c_utility.h:
* bindings/jni/jni_class.cpp: Same deal as c_class.cpp.
(JavaClass::JavaClass):
(JavaClass::~JavaClass):
* bindings/jni/jni_instance.cpp: Same deal as c_utility.cpp.
(JavaInstance::stringValue):
* bindings/jni/jni_jsobject.cpp:
(JavaJSObject::convertValueToJObject):
* bindings/jni/jni_runtime.cpp:
(JavaMethod::~JavaMethod): Moved from header, for clarity.
(appendClassName): Made this static, so the set of callers is known, and
we can assert that we hold the JSLock. Also changed it to take a UString
reference, which makes the calling code simpler.
(JavaMethod::signature): Store the ASCII value we care about instead of
a UString, since UString is so much more hassle. Hold the JSLock while
building up the temporary UString.
* bindings/jni/jni_runtime.h: Nixed dead code in JavaMethod.
(KJS::Bindings::JavaString::JavaString): Hold a UString::Rep instead of
a UString, so we can acquire the JSLock and explicitly release it.
(KJS::Bindings::JavaString::_commonInit):
(KJS::Bindings::JavaString::~JavaString):
(KJS::Bindings::JavaString::UTF8String):
(KJS::Bindings::JavaString::uchars):
(KJS::Bindings::JavaString::length):
(KJS::Bindings::JavaString::ustring):
* bindings/jni/jni_utility.cpp:
(KJS::Bindings::convertArrayInstanceToJavaArray): Made this static, so
the set of callers is known, and we can assert that we hold the JSLock.
(KJS::Bindings::convertValueToJValue): Acquire the JSLock because doing
it recursively is pretty cheap, and it's just too confusing to tell whether
all our callers do it for us.
* bindings/objc/objc_runtime.h: Nixed some dead code.
* bindings/objc/objc_utility.mm:
(KJS::Bindings::convertNSStringToString): Same drill as above.
2007-03-18 Alexey Proskuryakov <ap@webkit.org>
Reviewed by Geoff.
......
......@@ -28,13 +28,10 @@
#include "c_instance.h"
#include "c_runtime.h"
#include "npruntime_impl.h"
#include "identifier.h"
#include "npruntime_impl.h"
// FIXME: Should use HashMap instead of CFDictionary for better performance and for portability.
namespace KJS {
namespace Bindings {
namespace KJS { namespace Bindings {
CClass::CClass(NPClass* aClass)
{
......@@ -43,10 +40,15 @@ CClass::CClass(NPClass* aClass)
CClass::~CClass()
{
JSLock lock;
deleteAllValues(_methods);
_methods.clear();
deleteAllValues(_fields);
_fields.clear();
}
typedef HashMap<NPClass*, CClass*> ClassesByIsAMap;
static ClassesByIsAMap* classesByIsA = 0;
......@@ -84,14 +86,16 @@ MethodList CClass::methodsNamed(const Identifier& identifier, Instance* instance
NPObject* obj = inst->getObject();
if (_isa->hasMethod && _isa->hasMethod(obj, ident)){
Method* aMethod = new CMethod(ident); // deleted in the CClass destructor
_methods.set(identifier.ustring().rep(), aMethod);
{
JSLock lock;
_methods.set(identifier.ustring().rep(), aMethod);
}
methodList.addMethod(aMethod);
}
return methodList;
}
Field* CClass::fieldNamed(const Identifier& identifier, Instance* instance) const
{
Field* aField = _fields.get(identifier.ustring().rep());
......@@ -103,11 +107,12 @@ Field* CClass::fieldNamed(const Identifier& identifier, Instance* instance) cons
NPObject* obj = inst->getObject();
if (_isa->hasProperty && _isa->hasProperty(obj, ident)){
aField = new CField(ident); // deleted in the CClass destructor
_fields.set(identifier.ustring().rep(), aField);
{
JSLock lock;
_fields.set(identifier.ustring().rep(), aField);
}
}
return aField;
}
}
}
} } // namespace KJS::Bindings
......@@ -89,18 +89,11 @@ void convertUTF8ToUTF16(const NPUTF8 *UTF8Chars, int UTF8Length, NPUTF16 **UTF16
#endif
}
// Variant value must be released with NPReleaseVariantValue()
void coerceValueToNPVariantStringType(ExecState *exec, JSValue *value, NPVariant *result)
{
UString ustring = value->toString(exec);
CString cstring = ustring.UTF8String();
NPString string = { (const NPUTF8 *)cstring.c_str(), static_cast<uint32_t>(cstring.size()) };
NPN_InitializeVariantWithStringCopy(result, &string);
}
// Variant value must be released with NPReleaseVariantValue()
void convertValueToNPVariant(ExecState *exec, JSValue *value, NPVariant *result)
{
JSLock lock;
JSType type = value->type();
VOID_TO_NPVARIANT(*result);
......@@ -149,6 +142,8 @@ void convertValueToNPVariant(ExecState *exec, JSValue *value, NPVariant *result)
JSValue *convertNPVariantToValue(ExecState*, const NPVariant* variant)
{
JSLock lock;
NPVariantType type = variant->type;
if (type == NPVariantType_Bool)
......
......@@ -50,7 +50,6 @@ enum NP_ValueType {
void convertNPStringToUTF16(const NPString*, NPUTF16** UTF16Chars, unsigned int* UTF16Length);
void convertUTF8ToUTF16(const NPUTF8* UTF8Chars, int UTF8Length, NPUTF16** UTF16Chars, unsigned int* UTF16Length);
void coerceValueToNPVariantStringType(ExecState*, JSValue*, NPVariant* result);
void convertValueToNPVariant(ExecState*, JSValue*, NPVariant* result);
JSValue* convertNPVariantToValue(ExecState*, const NPVariant*);
Identifier identifierFromNPIdentifier(const NPUTF8* name);
......
......@@ -54,7 +54,10 @@ JavaClass::JavaClass(jobject anInstance)
for (i = 0; i < numFields; i++) {
jobject aJField = env->GetObjectArrayElement((jobjectArray)fields, i);
Field *aField = new JavaField(env, aJField); // deleted in the JavaClass destructor
_fields.set(Identifier(aField->name()).ustring().rep(), aField);
{
JSLock lock;
_fields.set(Identifier(aField->name()).ustring().rep(), aField);
}
env->DeleteLocalRef(aJField);
}
......@@ -64,10 +67,15 @@ JavaClass::JavaClass(jobject anInstance)
for (i = 0; i < numMethods; i++) {
jobject aJMethod = env->GetObjectArrayElement((jobjectArray)methods, i);
Method *aMethod = new JavaMethod(env, aJMethod); // deleted in the JavaClass destructor
MethodList *methodList = _methods.get(Identifier(aMethod->name()).ustring().rep());
if (!methodList) {
methodList = new MethodList();
_methods.set(Identifier(aMethod->name()).ustring().rep(), methodList);
MethodList* methodList;
{
JSLock lock;
methodList = _methods.get(Identifier(aMethod->name()).ustring().rep());
if (!methodList) {
methodList = new MethodList();
_methods.set(Identifier(aMethod->name()).ustring().rep(), methodList);
}
}
methodList->addMethod(aMethod);
env->DeleteLocalRef(aJMethod);
......@@ -86,8 +94,12 @@ JavaClass::JavaClass(jobject anInstance)
JavaClass::~JavaClass() {
free((void *)_name);
JSLock lock;
deleteAllValues(_fields);
_fields.clear();
MethodListMap::const_iterator end = _methods.end();
for (MethodListMap::const_iterator it = _methods.begin(); it != end; ++it) {
const MethodList* methodList = it->second;
......@@ -96,6 +108,8 @@ JavaClass::~JavaClass() {
delete methodList->methodAt(i);
delete methodList;
}
_methods.clear();
delete [] _constructors;
}
......
......@@ -75,6 +75,8 @@ Class *JavaInstance::getClass() const
JSValue *JavaInstance::stringValue() const
{
JSLock lock;
jstring stringValue = (jstring)callJNIObjectMethod (_instance->_instance, "toString", "()Ljava/lang/String;");
JNIEnv *env = getJNIEnv();
const jchar *c = getUCharactersFromJStringInEnv(env, stringValue);
......
......@@ -341,6 +341,8 @@ jlong JavaJSObject::createNative(jlong nativeHandle)
jobject JavaJSObject::convertValueToJObject (JSValue *value) const
{
JSLock lock;
RootObject* rootObject = this->rootObject();
if (!rootObject)
return 0;
......
......@@ -301,10 +301,18 @@ JavaMethod::JavaMethod (JNIEnv *env, jobject aMethod)
_isStatic = (bool)callJNIStaticBooleanMethod (modifierClass, "isStatic", "(I)Z", modifiers);
}
JavaMethod::~JavaMethod()
{
delete _signature;
delete [] _parameters;
};
// JNI method signatures use '/' between components of a class name, but
// we get '.' between components from the reflection API.
static void appendClassName (UString *aString, const char *className)
static void appendClassName(UString& aString, const char* className)
{
ASSERT(JSLock::lockCount() > 0);
char *result, *cp = strdup(className);
result = cp;
......@@ -314,46 +322,47 @@ static void appendClassName (UString *aString, const char *className)
cp++;
}
aString->append(result);
aString.append(result);
free (result);
}
const char *JavaMethod::signature() const
{
if (_signature == 0){
int i;
_signature = new UString("(");
for (i = 0; i < _numParameters; i++) {
if (!_signature) {
JSLock lock;
UString signatureBuilder("(");
for (int i = 0; i < _numParameters; i++) {
JavaParameter *aParameter = static_cast<JavaParameter *>(parameterAt(i));
JNIType _JNIType = aParameter->getJNIType();
if (_JNIType == array_type)
appendClassName(_signature, aParameter->type());
appendClassName(signatureBuilder, aParameter->type());
else {
_signature->append(signatureFromPrimitiveType (_JNIType));
signatureBuilder.append(signatureFromPrimitiveType(_JNIType));
if (_JNIType == object_type) {
appendClassName (_signature, aParameter->type());
_signature->append(";");
appendClassName(signatureBuilder, aParameter->type());
signatureBuilder.append(";");
}
}
}
_signature->append(")");
signatureBuilder.append(")");
const char *returnType = _returnType.UTF8String();
if (_JNIReturnType == array_type) {
appendClassName (_signature, returnType);
}
else {
_signature->append(signatureFromPrimitiveType (_JNIReturnType));
appendClassName(signatureBuilder, returnType);
} else {
signatureBuilder.append(signatureFromPrimitiveType(_JNIReturnType));
if (_JNIReturnType == object_type) {
appendClassName (_signature, returnType);
_signature->append(";");
appendClassName(signatureBuilder, returnType);
signatureBuilder.append(";");
}
}
_signature = strdup(signatureBuilder.ascii());
}
return _signature->ascii();
return _signature;
}
JNIType JavaMethod::JNIReturnType() const
......
......@@ -38,13 +38,20 @@ namespace Bindings
class JavaString
{
public:
JavaString () {};
JavaString()
{
JSLock lock;
_rep = UString().rep();
}
void _commonInit (JNIEnv *e, jstring s)
{
int _size = e->GetStringLength (s);
const jchar *uc = getUCharactersFromJStringInEnv (e, s);
_ustring = UString((UChar *)uc,_size);
{
JSLock lock;
_rep = UString((UChar *)uc,_size).rep();
}
releaseUCharactersForJStringInEnv (e, s, uc);
}
......@@ -56,17 +63,25 @@ public:
_commonInit (getJNIEnv(), s);
}
~JavaString()
{
JSLock lock;
_rep = 0;
}
const char *UTF8String() const {
if (_utf8String.c_str() == 0)
_utf8String = _ustring.UTF8String();
if (_utf8String.c_str() == 0) {
JSLock lock;
_utf8String = UString(_rep).UTF8String();
}
return _utf8String.c_str();
}
const jchar *uchars() const { return (const jchar *)_ustring.data(); }
int length() const { return _ustring.size(); }
UString ustring() const { return _ustring; }
const jchar *uchars() const { return (const jchar *)_rep->data(); }
int length() const { return _rep->size(); }
UString ustring() const { return UString(_rep); }
private:
UString _ustring;
RefPtr<UString::Rep> _rep;
mutable CString _utf8String;
};
......@@ -199,46 +214,8 @@ private:
class JavaMethod : public Method
{
public:
JavaMethod() : Method(), _signature(0), _methodID(0) {};
JavaMethod (JNIEnv *env, jobject aMethod);
void _commonDelete() {
delete _signature;
delete [] _parameters;
};
~JavaMethod () {
_commonDelete();
};
void _commonCopy(const JavaMethod &other) {
_name = other._name;
_returnType = other._returnType;
_numParameters = other._numParameters;
_parameters = new JavaParameter[_numParameters];
int i;
for (i = 0; i < _numParameters; i++) {
_parameters[i] = other._parameters[i];
}
_signature = other._signature;
};
JavaMethod(const JavaMethod &other) : Method() {
_commonCopy(other);
};
JavaMethod &operator=(const JavaMethod &other)
{
if (this == &other)
return *this;
_commonDelete();
_commonCopy(other);
return *this;
};
JavaMethod(JNIEnv* env, jobject aMethod);
~JavaMethod();
virtual const char *name() const { return _name.UTF8String(); };
RuntimeType returnType() const { return _returnType.UTF8String(); };
......@@ -256,7 +233,7 @@ private:
JavaParameter *_parameters;
int _numParameters;
JavaString _name;
mutable UString *_signature;
mutable char* _signature;
JavaString _returnType;
JNIType _JNIReturnType;
mutable jmethodID _methodID;
......
......@@ -747,8 +747,10 @@ jvalue getJNIField( jobject obj, JNIType type, const char *name, const char *sig
return result;
}
jobject convertArrayInstanceToJavaArray(ExecState *exec, JSValue *value, const char *javaClassName) {
static jobject convertArrayInstanceToJavaArray(ExecState *exec, JSValue *value, const char *javaClassName) {
ASSERT(JSLock::lockCount() > 0);
JNIEnv *env = getJNIEnv();
// As JS Arrays can contain a mixture of objects, assume we can convert to
// the requested Java Array type requested, unless the array type is some object array
......@@ -871,6 +873,8 @@ jobject convertArrayInstanceToJavaArray(ExecState *exec, JSValue *value, const c
jvalue convertValueToJValue (ExecState *exec, JSValue *value, JNIType _JNIType, const char *javaClassName)
{
JSLock lock;
jvalue result;
switch (_JNIType){
......
......@@ -102,21 +102,6 @@ public:
CFRelease(_javaScriptName);
}
ObjcMethod(const ObjcMethod &other) : Method()
{
_objcClass = other._objcClass;
_selector = other._selector;
}
ObjcMethod &operator=(const ObjcMethod &other)
{
if (this == &other)
return *this;
_objcClass = other._objcClass;
_selector = other._selector;
return *this;
}
virtual const char *name() const;
virtual int numParameters() const;
......
......@@ -195,6 +195,8 @@ ObjcValue convertValueToObjcValue(ExecState *exec, JSValue *value, ObjcValueType
JSValue *convertNSStringToString(NSString *nsstring)
{
JSLock lock;
unichar *chars;
unsigned int length = [nsstring length];
chars = (unichar *)malloc(sizeof(unichar)*length);
......
......@@ -96,7 +96,7 @@ private:
unsigned int _length;
};
class Method
class Method : Noncopyable
{
public:
virtual const char *name() const = 0;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment