Currently, JSCallbackObject<JSGlobalObject> registers weak finalizers via 2 mechanisms: 1. JSCallbackObject<Parent>::init() registers a finalizer for all JSClassRef that a JSCallbackObject references. 2. JSCallbackObject<JSGlobalObject>::create() registers a finalizer via vm.heap.addFinalizer() which destroys the JSCallbackObject. The first finalizer is implemented as a virtual function of a JSCallbackObjectData instance that will be destructed if the 2nd finalizer is called. Hence, if the 2nd finalizer if called first, the later invocation of the 1st finalizer will result in a crash.
Created attachment 248980 [details] the patch.
Attachment 248980 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/JSCallbackObject.cpp:48: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/API/tests/GlobalContextWithFinalizerTest.cpp:48: Use 0 instead of NULL. [readability/null] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 248980 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=248980&action=review > Source/JavaScriptCore/API/JSCallbackObject.cpp:52 > +static void destroyJSGlobalObjectCallbackObject(JSCell* cell) > +{ > + auto callbackObj = static_cast<JSCallbackObject<JSGlobalObject>*>(cell); > + > + JSObjectRef thisRef = toRef(static_cast<JSObject*>(cell)); > + for (JSClassRef jsClass = callbackObj->classRef(); jsClass; jsClass = jsClass->parentClass) > + if (JSObjectFinalizeCallback finalize = jsClass->finalize) > + finalize(thisRef); > + > + callbackObj->destroy(cell); Can this code move into the destructor or into the existing destroy function? No need to have three. > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:117 > +void JSCallbackObject<Parent>::installFinalizer(ExecState*) Let's call this addFinalizerIfNeeded.
Created attachment 249074 [details] patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor.
Comment on attachment 249074 [details] patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor. r=me Please fix Windows if needed.
Created attachment 249089 [details] patch 3: fixed Windows build, and broadened assertions. Patch 3 broadens the assertions (added in HeapInlines.h) to also include cases where the object should not derive from JSDestructibleObject. Previously, they only asserted the case where the object should derive from JSDestructibleObject.
Comment on attachment 249089 [details] patch 3: fixed Windows build, and broadened assertions. r=me
Thanks for the review. Landed in r181806: <http://trac.webkit.org/r181806>.
<rdar://problem/19969183>