Bug 142846 - JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData before all its finalizers have been called
Summary: JSCallbackObject<JSGlobalObject> should not destroy its JSCallbackObjectData ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-18 16:18 PDT by Mark Lam
Modified: 2015-03-20 11:11 PDT (History)
8 users (show)

See Also:


Attachments
the patch. (14.48 KB, patch)
2015-03-18 16:25 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
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+
Details | Formatted Diff | Diff
patch 3: fixed Windows build, and broadened assertions. (19.34 KB, patch)
2015-03-19 23:04 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>