[webkit-reviews] review granted: [Bug 106056] Objective-C objects that are passed to JavaScript leak (until the JSContext is destroyed) : [Attachment 182387] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 106056: Objective-C objects that are passed to JavaScript leak (until the
JSContext is destroyed)
https://bugs.webkit.org/show_bug.cgi?id=106056

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=182387&action=review


> Source/JavaScriptCore/API/JSWrapperMap.mm:379
> -    JSContext *m_context;
> +    JSContext* m_context;

Why move the *? Isn’t JSContext an Objective-C class?

> Source/JavaScriptCore/API/JSWrapperMap.mm:381
> +    JSC::WeakGCMap<id, JSC::JSObject>* m_cachedWrappers;

This should be an OwnPtr. But also, if it works, can’t we just have it be a
WeakGCMap rather than an OwnPtr<WeakGCMap>?

> Source/JavaScriptCore/API/JSWrapperMap.mm:399
> +    delete m_cachedWrappers;

Why not use an OwnPtr?

> Source/JavaScriptCore/API/JSWrapperMap.mm:425
> +    JSValue* wrapper = nil;

This should be:

    JSValue *wrapper;

For Objective-C types we use the other * placement (yuck, I wish we’d change
that rule), and there is no need to initialize this since it’s initialized in
all branches of the code below.