Bug 87581 – WebKit should be lazy-finalization-safe (esp. the DOM) v2

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

WebKit should be lazy-finalization-safe (esp. the DOM) v2

[Geoffrey Garen]

Created attachment 144207 [details]
Patch

[Oliver Hunt]

Comment on attachment 144207 [details]
Patch

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::~JSCallbackConstructor();
> +    static_cast<JSCallbackConstructor*>(cell)->JSCallbackConstructor::~JSCallbackConstructor();

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?

[Geoffrey Garen]

> > Source/JavaScriptCore/API/JSCallbackConstructor.cpp:64
> > -    jsCast<JSCallbackConstructor*>(cell)->JSCallbackConstructor::~JSCallbackConstructor();
> > +    static_cast<JSCallbackConstructor*>(cell)->JSCallbackConstructor::~JSCallbackConstructor();
> 
> Why are you making this change?

jsCast does Structure-based validation, and our Structure is not guaranteed to be alive when we get finalized. In particular, if our Structure has been recycled, the jsCast will probably ASSERT, and if our Structure has been unmapped from memory, the jsCast will segfault.

static_cast allows us to access our object just enough to deref / free its C++ pointers.

Perhaps we can clarify this interface in the future. 

> > Source/JavaScriptCore/heap/MarkedBlock.cpp:71
> > +#if !ASSERT_DISABLED
> 
> Make this conditional on GC validation, not assertions.

Added || ENABLE(GC_VALIDATION)

> can you have multiple weak handles to a single object?  might this break everything?

Yes, you can. No, it doesn't break anything. If one handle is dead, they're all dead, so scribbling this structure is correct for them all.

[Geoffrey Garen]

Committed r118616: <http://trac.webkit.org/changeset/118616>