Bug 87581 - WebKit should be lazy-finalization-safe (esp. the DOM) v2
Summary: WebKit should be lazy-finalization-safe (esp. the DOM) v2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on: 87701
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-26 14:57 PDT by Geoffrey Garen
Modified: 2012-05-28 23:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (41.95 KB, patch)
2012-05-26 15:04 PDT, Geoffrey Garen
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2012-05-26 14:57:20 PDT
WebKit should be lazy-finalization-safe (esp. the DOM) v2
Comment 1 Geoffrey Garen 2012-05-26 15:04:28 PDT
Created attachment 144207 [details]
Patch
Comment 2 Oliver Hunt 2012-05-26 15:12:33 PDT
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?
Comment 3 Geoffrey Garen 2012-05-26 15:36:17 PDT
> > 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.
Comment 4 Geoffrey Garen 2012-05-26 15:40:51 PDT
Committed r118616: <http://trac.webkit.org/changeset/118616>