Bug 142846

Summary: JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData before all its finalizers have been called
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, mhahnenb, mmirman, msaboff, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch.
none
patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor.
ggaren: review+
patch 3: fixed Windows build, and broadened assertions. ggaren: review+

Description Mark Lam 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.
Comment 1 Mark Lam 2015-03-18 16:25:58 PDT
Created attachment 248980 [details]
the patch.
Comment 2 WebKit Commit Bot 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.
Comment 3 Geoffrey Garen 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.
Comment 4 Mark Lam 2015-03-19 18:18:35 PDT
Created attachment 249074 [details]
patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor.
Comment 5 Geoffrey Garen 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.
Comment 6 Mark Lam 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.
Comment 7 Geoffrey Garen 2015-03-20 11:04:58 PDT
Comment on attachment 249089 [details]
patch 3: fixed Windows build, and broadened assertions.

r=me
Comment 8 Mark Lam 2015-03-20 11:10:04 PDT
Thanks for the review.  Landed in r181806: <http://trac.webkit.org/r181806>.
Comment 9 Mark Lam 2015-03-20 11:11:41 PDT
<rdar://problem/19969183>