Bug 106056 – Objective-C objects that are passed to JavaScript leak (until the JSContext is destroyed)

• Component: JavaScriptCore
• Status: REOPENED

[Geoffrey Garen]

The JSContext keeps alive all JSValues that reference Objective-C objects, even if the Objective-C objects and the JSValues have otherwise become unreachable. This means that if you have an event loop that regularly sends events to JavaScript code, you have a serious memory use problem.

[Geoffrey Garen]

For now, let's do this:

    (1) JSValue is a strong reference to JSContext

    (2) JSContext holds a WeakGCMap keyed on raw Objective-C pointer, whose value is the JavaScript wrapper. Wrapper creation runs through this map.

[Mark Hahnenberg]

Created attachment 182378 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 182378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182378&action=review

> Source/JavaScriptCore/API/JSWrapperMap.mm:-38
> -static void wrapperFinalize(JSObjectRef object)

Not sure why I did this.

[Mark Hahnenberg]

Created attachment 182387 [details]
Patch

[Darin Adler]

Comment on attachment 182387 [details]
Patch

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.

[Mark Hahnenberg]

Committed r139486: <http://trac.webkit.org/changeset/139486>

[Mark Hahnenberg]

I made one small change, which was to add retain/release for the JSContext * in JSValue since we're not using ARC for this particular file.

[Mark Hahnenberg]

(In reply to comment #7)
> I made one small change, which was to add retain/release for the JSContext * in JSValue since we're not using ARC for this particular file.

(RS'ed by ggaren).

[Gavin Barraclough]

Hey Mark,

There are number more changes that should be made as a part of this change:

(1) The documentation in the API  is now incorrect.  The comments on the JSValue & JSContext classes describe the object lifecycle, these are now utterly wrong.  The second paragraph of the JSValue documentation should probably just be removed, and you need to add text indicating that the context in which the value is created will be retained. 

(2) JSContext m_protectCounts, protect, unprotect are all now unnecessary overhead, and should all be removed.  These exist to handle the context going away before the value does; the context need to be able to unprotect values early.  Since the value is keeping the context alive there is no longer any danger of this happening; instead you should just protect/unprotect the value in JSValue's init/dealloc methods.

(3) m_context is no longer weak, so there is now a lot of dead code in in JSValue.mm, and a wasted message send on every API call.  In the head of just about every method in JSValue.mm we're doing:
    JSContext *context = [self context];
    if (!context)
        return nil;
This is getting a retained copy of the context, which is no longer necessary now m_context is no longer weak.  Just delete all these lines from all functions doing this, and where they were referring to the local variable 'context', instead you can just access m_context directly.

(4) context @property is no longer weak – the context property is declared as:
    @property(readonly, weak) JSContext *context;
This is really only informative (since we're not presently synthesizing the ivar), but it is now misleading, please change this to:
    @property(readonly, retain) JSContext *context;

(5) the JSContext ivar and accessor can be automatically generated.  Since we're no longer doing anything special with m_context, we can just let the compiler handle the ivar for us.  delete:
    JSContext *m_context;
and:
    - (JSContext *)context
    {
        return m_context;
    }
and find&replace "m_context" to "_context" in JSValue.mm.

[Geoffrey Garen]

<rdar://problem/13014028>

[Geoffrey Garen]

Reopening based on Gavin's comments.