RESOLVED FIXED 142846
JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData before all its finalizers have been called 
https://bugs.webkit.org/show_bug.cgi?id=142846
Summary JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData ...
Mark Lam
Reported 2015-03-18 16:18:11 PDT
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.
Attachments
the patch. (14.48 KB, patch)
2015-03-18 16:25 PDT, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor. (19.41 KB, patch)
2015-03-19 18:18 PDT, Mark Lam 		ggaren: review+
DetailsFormatted DiffDiff
patch 3: fixed Windows build, and broadened assertions. (19.34 KB, patch)
2015-03-19 23:04 PDT, Mark Lam 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2015-03-18 16:25:58 PDT
Created attachment 248980 [details] the patch.
WebKit Commit Bot
Comment 2 2015-03-18 16:28:14 PDT
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.
Geoffrey Garen
Comment 3 2015-03-18 16:35:49 PDT
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.
Mark Lam
Comment 4 2015-03-19 18:18:35 PDT
Created attachment 249074 [details] patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor.
Geoffrey Garen
Comment 5 2015-03-19 19:21:55 PDT
Comment on attachment 249074 [details] patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor. r=me Please fix Windows if needed.
Mark Lam
Comment 6 2015-03-19 23:04:58 PDT
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.
Geoffrey Garen
Comment 7 2015-03-20 11:04:58 PDT
Comment on attachment 249089 [details] patch 3: fixed Windows build, and broadened assertions. r=me
Mark Lam
Comment 8 2015-03-20 11:10:04 PDT
Thanks for the review. Landed in r181806: <http://trac.webkit.org/r181806>.
Mark Lam
Comment 9 2015-03-20 11:11:41 PDT
<rdar://problem/19969183>
Note You need to log in before you can comment on or make changes to this bug.