JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData before all its finalizers have been called

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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.

[Mark Lam]

Created attachment 248980 [details]
the patch.

[WebKit Commit Bot]

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 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]

Created attachment 249074 [details]
patch 2: now does the C finalizer callbacks in the JSCallbackObject destructor.

[Geoffrey Garen]

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]

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 on attachment 249089 [details]
patch 3: fixed Windows build, and broadened assertions.

r=me

[Mark Lam]

Thanks for the review.  Landed in r181806: <http://trac.webkit.org/r181806>.

[Mark Lam]

<rdar://problem/19969183>