From f55539ca87c1506befb6041e2ee8826b5aef6fd9 Mon Sep 17 00:00:00 2001 From: "darin@apple.com" Date: Fri, 18 Sep 2009 17:48:23 +0000 Subject: [PATCH] Each wrapped Objective-C object should use a single RuntimeObjectImp https://bugs.webkit.org/show_bug.cgi?id=29351 rdar://problem/7142294 Patch by Darin Adler on 2009-09-18 Reviewed by Sam Weinig. * WebCore.base.exp: Added a newly-needed exported symbol. * bindings/objc/DOMInternal.h: Eliminated unused createWrapperCacheWithIntegerKeys; it has not been needed since the RGBColor wrappers were reworked. * bindings/objc/DOMInternal.mm: Ditto. * bridge/objc/objc_instance.h: Made the create function non-inline. * bridge/objc/objc_instance.mm: (createInstanceWrapperCache): Added. Creates an appropriate map table. (ObjcInstance::create): Moved here from header. Uses NSMapGet and NSMapInsert to cache the instance in a map table. (ObjcInstance::~ObjcInstance): Added a call to NSMapRemove to remove the instance from the map table. * bridge/qt/qt_instance.cpp: (JSC::Bindings::QtInstance::~QtInstance): Remove unneeded code to remove the instance from cachedObjects, which no longer exists. (JSC::Bindings::QtInstance::newRuntimeObject): Renamed to overload new bottleneck. Caching is now handled by the base class. * bridge/runtime.cpp: (JSC::Bindings::Instance::Instance): Initialize m_runtimeObject to 0. (JSC::Bindings::Instance::~Instance): Assert m_runtimeObject is 0. (JSC::Bindings::Instance::createRuntimeObject): Use m_runtimeObject if it's already set. Set m_runtimeObject and call addRuntimeObject if it's not. (JSC::Bindings::Instance::newRuntimeObject): Added. Virtual function, used only by createRuntimeObject. (JSC::Bindings::Instance::willDestroyRuntimeObject): Added. Calls removeRuntimeObject and then clears m_runtimeObject. (JSC::Bindings::Instance::willInvalidateRuntimeObject): Added. Clears m_runtimeObject. * bridge/runtime.h: Made createRuntimeObject non-virtual. Added willDestroyRuntimeObject, willInvalidateRuntimeObject, newRuntimeObject, and m_runtimeObject. * bridge/runtime_object.cpp: (JSC::RuntimeObjectImp::RuntimeObjectImp): Removed addRuntimeObject call, now handled by caller. (JSC::RuntimeObjectImp::~RuntimeObjectImp): Replaced removeRuntimeObject call with willDestroyRuntimeObject call; the latter nows calls removeRuntimeObject. (JSC::RuntimeObjectImp::invalidate): Added willInvalidateRuntimeObject call. * bridge/runtime_object.h: Made invalidate non-virtual. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@48513 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- WebCore/ChangeLog | 57 ++++++++++++++++++++++++++++ WebCore/WebCore.base.exp | 1 + WebCore/bindings/objc/DOMInternal.h | 1 - WebCore/bindings/objc/DOMInternal.mm | 12 ------ WebCore/bridge/objc/objc_instance.h | 16 +++----- WebCore/bridge/objc/objc_instance.mm | 38 +++++++++++++++++-- WebCore/bridge/qt/qt_instance.cpp | 39 +------------------ WebCore/bridge/runtime.cpp | 30 ++++++++++++++- WebCore/bridge/runtime.h | 13 +++++-- WebCore/bridge/runtime_object.cpp | 9 ++--- WebCore/bridge/runtime_object.h | 8 ++-- 11 files changed, 147 insertions(+), 77 deletions(-) diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog index 946fd13f4ac..72ed58b12b1 100644 --- a/WebCore/ChangeLog +++ b/WebCore/ChangeLog @@ -1,3 +1,60 @@ +2009-09-18 Darin Adler + + Reviewed by Sam Weinig. + + Each wrapped Objective-C object should use a single RuntimeObjectImp + https://bugs.webkit.org/show_bug.cgi?id=29351 + rdar://problem/7142294 + + * WebCore.base.exp: Added a newly-needed exported symbol. + + * bindings/objc/DOMInternal.h: Eliminated unused + createWrapperCacheWithIntegerKeys; it has not been needed since the + RGBColor wrappers were reworked. + * bindings/objc/DOMInternal.mm: Ditto. + + * bridge/objc/objc_instance.h: Made the create function non-inline. + * bridge/objc/objc_instance.mm: + (createInstanceWrapperCache): Added. Creates an appropriate map table. + (ObjcInstance::create): Moved here from header. Uses NSMapGet and + NSMapInsert to cache the instance in a map table. + (ObjcInstance::~ObjcInstance): Added a call to NSMapRemove to remove + the instance from the map table. + + * bridge/qt/qt_instance.cpp: + (JSC::Bindings::QtInstance::~QtInstance): Remove unneeded code to remove + the instance from cachedObjects, which no longer exists. + (JSC::Bindings::QtInstance::newRuntimeObject): Renamed to overload new + bottleneck. Caching is now handled by the base class. + + * bridge/runtime.cpp: + (JSC::Bindings::Instance::Instance): Initialize m_runtimeObject to 0. + (JSC::Bindings::Instance::~Instance): Assert m_runtimeObject is 0. + (JSC::Bindings::Instance::createRuntimeObject): Use m_runtimeObject + if it's already set. Set m_runtimeObject and call addRuntimeObject + if it's not. + (JSC::Bindings::Instance::newRuntimeObject): Added. Virtual function, + used only by createRuntimeObject. + (JSC::Bindings::Instance::willDestroyRuntimeObject): Added. + Calls removeRuntimeObject and then clears m_runtimeObject. + (JSC::Bindings::Instance::willInvalidateRuntimeObject): Added. + Clears m_runtimeObject. + + * bridge/runtime.h: Made createRuntimeObject non-virtual. Added + willDestroyRuntimeObject, willInvalidateRuntimeObject, + newRuntimeObject, and m_runtimeObject. + + * bridge/runtime_object.cpp: + (JSC::RuntimeObjectImp::RuntimeObjectImp): Removed addRuntimeObject + call, now handled by caller. + (JSC::RuntimeObjectImp::~RuntimeObjectImp): Replaced removeRuntimeObject + call with willDestroyRuntimeObject call; the latter nows calls + removeRuntimeObject. + (JSC::RuntimeObjectImp::invalidate): Added willInvalidateRuntimeObject + call. + + * bridge/runtime_object.h: Made invalidate non-virtual. + 2009-09-17 Kenneth Rohde Christiansen Reviewed by Simon Hausmann. diff --git a/WebCore/WebCore.base.exp b/WebCore/WebCore.base.exp index 2f36de8197b..008e799f02d 100644 --- a/WebCore/WebCore.base.exp +++ b/WebCore/WebCore.base.exp @@ -133,6 +133,7 @@ __Z4coreP19DOMDocumentFragment __Z4coreP22DOMCSSStyleDeclaration __Z4coreP7DOMNode __Z4coreP8DOMRange +__ZN3JSC8Bindings8Instance16newRuntimeObjectEPNS_9ExecStateE __ZN7WebCore10MouseEventC1ERKNS_12AtomicStringEbbN3WTF10PassRefPtrINS_9DOMWindowEEEiiiiibbbbtNS5_INS_11EventTargetEEENS5_INS_9ClipboardEEEb __ZN7WebCore10ScrollView17setScrollbarModesENS_13ScrollbarModeES1_ __ZN7WebCore10ScrollView20setCanHaveScrollbarsEb diff --git a/WebCore/bindings/objc/DOMInternal.h b/WebCore/bindings/objc/DOMInternal.h index 48f5d2f8673..72f63d2ca57 100644 --- a/WebCore/bindings/objc/DOMInternal.h +++ b/WebCore/bindings/objc/DOMInternal.h @@ -61,7 +61,6 @@ namespace WebCore { // Create an NSMapTable mapping from pointers to ObjC objects held with zeroing weak references. NSMapTable* createWrapperCache(); -NSMapTable* createWrapperCacheWithIntegerKeys(); // Same, but from integers to ObjC objects. id createDOMWrapper(JSC::JSObject*, PassRefPtr origin, PassRefPtr current); diff --git a/WebCore/bindings/objc/DOMInternal.mm b/WebCore/bindings/objc/DOMInternal.mm index 9b26e5973c5..993a3ade593 100644 --- a/WebCore/bindings/objc/DOMInternal.mm +++ b/WebCore/bindings/objc/DOMInternal.mm @@ -49,18 +49,6 @@ NSMapTable* createWrapperCache() #endif } -NSMapTable* createWrapperCacheWithIntegerKeys() -{ -#ifdef BUILDING_ON_TIGER - return NSCreateMapTable(NSIntMapKeyCallBacks, NSNonRetainedObjectMapValueCallBacks, 0); -#else - // NSMapTable with zeroing weak pointers is the recommended way to build caches like this under garbage collection. - NSPointerFunctionsOptions keyOptions = NSPointerFunctionsOpaqueMemory | NSPointerFunctionsIntegerPersonality; - NSPointerFunctionsOptions valueOptions = NSPointerFunctionsZeroingWeakMemory | NSPointerFunctionsObjectPersonality; - return [[NSMapTable alloc] initWithKeyOptions:keyOptions valueOptions:valueOptions capacity:0]; -#endif -} - NSObject* getDOMWrapper(DOMObjectInternal* impl) { if (!DOMWrapperCache) diff --git a/WebCore/bridge/objc/objc_instance.h b/WebCore/bridge/objc/objc_instance.h index 6f1eb0d3391..64cd4919b01 100644 --- a/WebCore/bridge/objc/objc_instance.h +++ b/WebCore/bridge/objc/objc_instance.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2003 Apple Computer, Inc. All rights reserved. + * Copyright (C) 2003, 2009 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -37,16 +37,12 @@ class ObjcClass; class ObjcInstance : public Instance { public: - static PassRefPtr create(ObjectStructPtr instance, PassRefPtr rootObject) - { - return adoptRef(new ObjcInstance(instance, rootObject)); - } - + static PassRefPtr create(ObjectStructPtr, PassRefPtr); + virtual ~ObjcInstance(); + static void setGlobalException(NSString*, JSGlobalObject* exceptionEnvironment = 0); // A null exceptionEnvironment means the exception should propogate to any execution environment. - ~ObjcInstance(); - - virtual Class *getClass() const; + virtual Class* getClass() const; virtual JSValue valueOf(ExecState*) const; virtual JSValue defaultValue(ExecState*, PreferredPrimitiveType) const; @@ -71,7 +67,7 @@ protected: private: static void moveGlobalExceptionToExecState(ExecState*); - ObjcInstance(ObjectStructPtr instance, PassRefPtr); + ObjcInstance(ObjectStructPtr, PassRefPtr); RetainPtr _instance; mutable ObjcClass *_class; diff --git a/WebCore/bridge/objc/objc_instance.mm b/WebCore/bridge/objc/objc_instance.mm index b84f7bb14c4..f7550e4b23a 100644 --- a/WebCore/bridge/objc/objc_instance.mm +++ b/WebCore/bridge/objc/objc_instance.mm @@ -1,5 +1,5 @@ /* - * Copyright (C) 2004, 2008 Apple Computer, Inc. All rights reserved. + * Copyright (C) 2004, 2008, 2009 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -44,8 +44,21 @@ using namespace JSC::Bindings; using namespace JSC; -static NSString* s_exception; +static NSString *s_exception; static JSGlobalObject* s_exceptionEnvironment; // No need to protect this value, since we just use it for a pointer comparison. +static NSMapTable *s_instanceWrapperCache; + +static NSMapTable *createInstanceWrapperCache() +{ +#ifdef BUILDING_ON_TIGER + return NSCreateMapTable(NSNonRetainedObjectMapKeyCallBacks, NSNonOwnedPointerMapValueCallBacks, 0); +#else + // NSMapTable with zeroing weak pointers is the recommended way to build caches like this under garbage collection. + NSPointerFunctionsOptions keyOptions = NSPointerFunctionsZeroingWeakMemory | NSPointerFunctionsObjectPersonality; + NSPointerFunctionsOptions valueOptions = NSPointerFunctionsOpaqueMemory | NSPointerFunctionsOpaquePersonality; + return [[NSMapTable alloc] initWithKeyOptions:keyOptions valueOptions:valueOptions capacity:0]; +#endif +} void ObjcInstance::setGlobalException(NSString* exception, JSGlobalObject* exceptionEnvironment) { @@ -74,7 +87,7 @@ void ObjcInstance::moveGlobalExceptionToExecState(ExecState* exec) s_exceptionEnvironment = 0; } -ObjcInstance::ObjcInstance(ObjectStructPtr instance, PassRefPtr rootObject) +ObjcInstance::ObjcInstance(id instance, PassRefPtr rootObject) : Instance(rootObject) , _instance(instance) , _class(0) @@ -83,13 +96,30 @@ ObjcInstance::ObjcInstance(ObjectStructPtr instance, PassRefPtr root { } +PassRefPtr ObjcInstance::create(id instance, PassRefPtr rootObject) +{ + if (!s_instanceWrapperCache) + s_instanceWrapperCache = createInstanceWrapperCache(); + if (void* existingWrapper = NSMapGet(s_instanceWrapperCache, instance)) + return static_cast(existingWrapper); + RefPtr wrapper = adoptRef(new ObjcInstance(instance, rootObject)); + NSMapInsert(s_instanceWrapperCache, instance, wrapper.get()); + return wrapper.release(); +} + ObjcInstance::~ObjcInstance() { - // -finalizeForWebScript and -dealloc/-finalize may require autorelease pools. + // Both -finalizeForWebScript and -dealloc/-finalize of _instance may require autorelease pools. NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init]; + + ASSERT(s_instanceWrapperCache); + ASSERT(_instance); + NSMapRemove(s_instanceWrapperCache, _instance.get()); + if ([_instance.get() respondsToSelector:@selector(finalizeForWebScript)]) [_instance.get() performSelector:@selector(finalizeForWebScript)]; _instance = 0; + [pool drain]; } diff --git a/WebCore/bridge/qt/qt_instance.cpp b/WebCore/bridge/qt/qt_instance.cpp index 58280e3deaa..05460140593 100644 --- a/WebCore/bridge/qt/qt_instance.cpp +++ b/WebCore/bridge/qt/qt_instance.cpp @@ -43,16 +43,10 @@ namespace Bindings { typedef QMultiHash QObjectInstanceMap; static QObjectInstanceMap cachedInstances; -// Cache JSObjects -typedef QHash InstanceJSObjectMap; -static InstanceJSObjectMap cachedObjects; - // Derived RuntimeObject class QtRuntimeObjectImp : public RuntimeObjectImp { public: QtRuntimeObjectImp(ExecState*, PassRefPtr); - ~QtRuntimeObjectImp(); - virtual void invalidate(); static const ClassInfo s_info; @@ -64,9 +58,6 @@ public: instance->markAggregate(markStack); } -protected: - void removeFromCache(); - private: virtual const ClassInfo* classInfo() const { return &s_info; } }; @@ -78,25 +69,6 @@ QtRuntimeObjectImp::QtRuntimeObjectImp(ExecState* exec, PassRefPtr ins { } -QtRuntimeObjectImp::~QtRuntimeObjectImp() -{ - removeFromCache(); -} - -void QtRuntimeObjectImp::invalidate() -{ - removeFromCache(); - RuntimeObjectImp::invalidate(); -} - -void QtRuntimeObjectImp::removeFromCache() -{ - JSLock lock(SilenceAssertionsOnly); - QtInstance* key = cachedObjects.key(this); - if (key) - cachedObjects.remove(key); -} - // QtInstance QtInstance::QtInstance(QObject* o, PassRefPtr rootObject, QScriptEngine::ValueOwnership ownership) : Instance(rootObject) @@ -112,7 +84,6 @@ QtInstance::~QtInstance() { JSLock lock(SilenceAssertionsOnly); - cachedObjects.remove(this); cachedInstances.remove(m_hashkey); // clean up (unprotect from gc) the JSValues we've created @@ -190,16 +161,10 @@ Class* QtInstance::getClass() const return m_class; } -RuntimeObjectImp* QtInstance::createRuntimeObject(ExecState* exec) +RuntimeObjectImp* QtInstance::newRuntimeObject(ExecState* exec) { JSLock lock(SilenceAssertionsOnly); - RuntimeObjectImp* ret = static_cast(cachedObjects.value(this)); - if (!ret) { - ret = new (exec) QtRuntimeObjectImp(exec, this); - cachedObjects.insert(this, ret); - ret = static_cast(cachedObjects.value(this)); - } - return ret; + return new (exec) QtRuntimeObjectImp(exec, this); } void QtInstance::markAggregate(MarkStack& markStack) diff --git a/WebCore/bridge/runtime.cpp b/WebCore/bridge/runtime.cpp index 69344066423..eac8586a36e 100644 --- a/WebCore/bridge/runtime.cpp +++ b/WebCore/bridge/runtime.cpp @@ -48,12 +48,14 @@ Array::~Array() Instance::Instance(PassRefPtr rootObject) : _rootObject(rootObject) + , m_runtimeObject(0) { ASSERT(_rootObject); } Instance::~Instance() { + ASSERT(!m_runtimeObject); } static KJSDidExecuteFunctionPtr s_didExecuteFunction; @@ -79,12 +81,38 @@ void Instance::end() } RuntimeObjectImp* Instance::createRuntimeObject(ExecState* exec) +{ + ASSERT(_rootObject); + ASSERT(_rootObject->isValid()); + if (m_runtimeObject) + return m_runtimeObject; + JSLock lock(SilenceAssertionsOnly); + m_runtimeObject = newRuntimeObject(exec); + _rootObject->addRuntimeObject(m_runtimeObject); + return m_runtimeObject; +} + +RuntimeObjectImp* Instance::newRuntimeObject(ExecState* exec) { JSLock lock(SilenceAssertionsOnly); - return new (exec) RuntimeObjectImp(exec, this); } +void Instance::willDestroyRuntimeObject() +{ + ASSERT(_rootObject); + ASSERT(_rootObject->isValid()); + ASSERT(m_runtimeObject); + _rootObject->removeRuntimeObject(m_runtimeObject); + m_runtimeObject = 0; +} + +void Instance::willInvalidateRuntimeObject() +{ + ASSERT(m_runtimeObject); + m_runtimeObject = 0; +} + RootObject* Instance::rootObject() const { return _rootObject && _rootObject->isValid() ? _rootObject.get() : 0; diff --git a/WebCore/bridge/runtime.h b/WebCore/bridge/runtime.h index e0280209160..72ee3877d79 100644 --- a/WebCore/bridge/runtime.h +++ b/WebCore/bridge/runtime.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2003, 2008 Apple Inc. All rights reserved. + * Copyright (C) 2003, 2008, 2009 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -86,8 +86,10 @@ public: void begin(); void end(); - virtual Class *getClass() const = 0; - virtual RuntimeObjectImp* createRuntimeObject(ExecState*); + virtual Class* getClass() const = 0; + RuntimeObjectImp* createRuntimeObject(ExecState*); + void willInvalidateRuntimeObject(); + void willDestroyRuntimeObject(); // Returns false if the value was not set successfully. virtual bool setValueOfUndefinedField(ExecState*, const Identifier&, JSValue) { return false; } @@ -119,6 +121,11 @@ protected: virtual void virtualEnd() { } RefPtr _rootObject; + +private: + virtual RuntimeObjectImp* newRuntimeObject(ExecState*); + + RuntimeObjectImp* m_runtimeObject; }; class Array : public Noncopyable { diff --git a/WebCore/bridge/runtime_object.cpp b/WebCore/bridge/runtime_object.cpp index f731af11728..3fd8024bc68 100644 --- a/WebCore/bridge/runtime_object.cpp +++ b/WebCore/bridge/runtime_object.cpp @@ -1,5 +1,5 @@ /* - * Copyright (C) 2003, 2008 Apple Inc. All rights reserved. + * Copyright (C) 2003, 2008, 2009 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -28,7 +28,6 @@ #include "JSDOMBinding.h" #include "runtime_method.h" -#include "runtime_root.h" #include #include @@ -46,25 +45,25 @@ RuntimeObjectImp::RuntimeObjectImp(ExecState* exec, PassRefPtr instanc : JSObject(deprecatedGetDOMStructure(exec)) , m_instance(instance) { - m_instance->rootObject()->addRuntimeObject(this); } RuntimeObjectImp::RuntimeObjectImp(ExecState*, PassRefPtr structure, PassRefPtr instance) : JSObject(structure) , m_instance(instance) { - m_instance->rootObject()->addRuntimeObject(this); } RuntimeObjectImp::~RuntimeObjectImp() { if (m_instance) - m_instance->rootObject()->removeRuntimeObject(this); + m_instance->willDestroyRuntimeObject(); } void RuntimeObjectImp::invalidate() { ASSERT(m_instance); + if (m_instance) + m_instance->willInvalidateRuntimeObject(); m_instance = 0; } diff --git a/WebCore/bridge/runtime_object.h b/WebCore/bridge/runtime_object.h index 2b29e2e3d26..5aa02eae8cf 100644 --- a/WebCore/bridge/runtime_object.h +++ b/WebCore/bridge/runtime_object.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2003, 2008 Apple Inc. All rights reserved. + * Copyright (C) 2003, 2008, 2009 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -34,13 +34,12 @@ namespace JSC { class RuntimeObjectImp : public JSObject { public: RuntimeObjectImp(ExecState*, PassRefPtr); - virtual ~RuntimeObjectImp(); virtual bool getOwnPropertySlot(ExecState*, const Identifier& propertyName, PropertySlot&); virtual bool getOwnPropertyDescriptor(ExecState*, const Identifier& propertyName, PropertyDescriptor&); virtual void put(ExecState*, const Identifier& propertyName, JSValue, PutPropertySlot&); - virtual bool deleteProperty(ExecState* , const Identifier& propertyName); + virtual bool deleteProperty(ExecState*, const Identifier& propertyName); virtual JSValue defaultValue(ExecState*, PreferredPrimitiveType) const; virtual CallType getCallData(CallData&); virtual ConstructType getConstructData(ConstructData&); @@ -48,7 +47,8 @@ public: virtual void getPropertyNames(ExecState*, PropertyNameArray&); virtual void getOwnPropertyNames(ExecState*, PropertyNameArray&); - virtual void invalidate(); + void invalidate(); + Bindings::Instance* getInternalInstance() const { return m_instance.get(); } static JSObject* throwInvalidAccessError(ExecState*); -- GitLab