[webkit-reviews] review granted: [Bug 87581] WebKit should be lazy-finalization-safe (esp. the DOM) v2 : [Attachment 144207] Patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 87581: WebKit should be lazy-finalization-safe (esp. the DOM) v2
https://bugs.webkit.org/show_bug.cgi?id=87581

Attachment 144207: Patch
https://bugs.webkit.org/attachment.cgi?id=144207&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=144207&action=review


r+ assuming you change the !ASSERTION bits to GC validation, and can reasonably
answer the static cast questions :D

> Source/JavaScriptCore/API/JSCallbackConstructor.cpp:64
> -   
jsCast<JSCallbackConstructor*>(cell)->JSCallbackConstructor::~JSCallbackConstru
ctor();
> +   
static_cast<JSCallbackConstructor*>(cell)->JSCallbackConstructor::~JSCallbackCo
nstructor();

Why are you making this change?

> Source/JavaScriptCore/API/JSCallbackObject.cpp:57
> -    jsCast<JSCallbackObject*>(cell)->JSCallbackObject::~JSCallbackObject();
> +   
static_cast<JSCallbackObject*>(cell)->JSCallbackObject::~JSCallbackObject();

ditto

> Source/JavaScriptCore/API/JSCallbackObject.cpp:63
> -    JSObjectRef thisRef = toRef(asObject(handle.get()));
> +    JSObjectRef thisRef =
toRef(static_cast<JSObject*>(handle.get().asCell()));

if a static cast is valid, a jsCast should be as well -- why isn't it?

> Source/JavaScriptCore/heap/MarkedBlock.cpp:71
> +#if !ASSERT_DISABLED

Make this conditional on GC validation, not assertions.  There are times where
it's nice to be able to test stuff in release builds.

> Source/JavaScriptCore/heap/WeakSetInlines.h:53
> +#if !ASSERT_DISABLED
> +    weakImpl->jsValue().asCell()->clearStructure();

GC validation rather than assertion based... can you have multiple weak handles
to a single object?  might this break everything?