Bug 106056 - Objective-C objects that are passed to JavaScript leak (until the JSContext is destroyed)
Summary: Objective-C objects that are passed to JavaScript leak (until the JSContext i...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Mark Hahnenberg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-01-03 15:37 PST by Geoffrey Garen
Modified: 2013-01-14 23:12 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.67 KB, patch)
2013-01-11 11:17 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (5.54 KB, patch)
2013-01-11 11:46 PST, Mark Hahnenberg
darin: 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 2013-01-03 15:37:52 PST
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.
Comment 1 Geoffrey Garen 2013-01-10 12:43:48 PST
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.
Comment 2 Mark Hahnenberg 2013-01-11 11:17:12 PST
Created attachment 182378 [details]
Patch
Comment 3 Mark Hahnenberg 2013-01-11 11:41:31 PST
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.
Comment 4 Mark Hahnenberg 2013-01-11 11:46:27 PST
Created attachment 182387 [details]
Patch
Comment 5 Darin Adler 2013-01-11 12:11:06 PST
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.
Comment 6 Mark Hahnenberg 2013-01-11 12:57:40 PST
Committed r139486: <http://trac.webkit.org/changeset/139486>
Comment 7 Mark Hahnenberg 2013-01-11 12:59:07 PST
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.
Comment 8 Mark Hahnenberg 2013-01-11 13:08:41 PST
(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).
Comment 9 Gavin Barraclough 2013-01-14 21:49:05 PST
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.
Comment 10 Geoffrey Garen 2013-01-14 23:11:51 PST
<rdar://problem/13014028>
Comment 11 Geoffrey Garen 2013-01-14 23:12:47 PST
Reopening based on Gavin's comments.