Bug 112856 – Objective-C API: wrapperClass holds a static JSClassRef, which causes JSGlobalObjects to leak

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Through a very convoluted path that involves the caching of prototypes on the JSClassRef, we can leak JSGlobalObjects when inserting an Objective-C object into multiple independent JSContexts.

The solution is simply to divorce JSWrapperMap and JSObjCClassInfo from the C API and use JSAPIWrapperObjects directly (without going through JSCallbackObject).

[Mark Hahnenberg]

Created attachment 194180 [details]
Patch

[Early Warning System Bot]

Comment on attachment 194180 [details]
Patch

Attachment 194180 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17211574

[Early Warning System Bot]

Comment on attachment 194180 [details]
Patch

Attachment 194180 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17213776

[Mark Hahnenberg]

Created attachment 194182 [details]
Patch

[Mark Hahnenberg]

<rdar://problem/13467942>

[Geoffrey Garen]

Comment on attachment 194182 [details]
Patch

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

> Source/JavaScriptCore/API/JSAPIWrapperObject.cpp:55
> +    CFRelease(wrapperObject->wrappedObject());

CFRelease is not always the same as -[NSObject release], so this is a no-no.

Let's compile this file as Objective-C++ and call -release.

> Source/JavaScriptCore/API/JSWrapperMap.mm:360
> +            constructor = objectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sConstructor", className], [m_class retain]);

Why are we calling -retain here? You should only call -retain when storing a reference into a data member.

[Mark Hahnenberg]

Comment on attachment 194182 [details]
Patch

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

>> Source/JavaScriptCore/API/JSAPIWrapperObject.cpp:55
>> +    CFRelease(wrapperObject->wrappedObject());
> 
> CFRelease is not always the same as -[NSObject release], so this is a no-no.
> 
> Let's compile this file as Objective-C++ and call -release.

Okiedokie.

>> Source/JavaScriptCore/API/JSWrapperMap.mm:360
>> +            constructor = objectWithCustomBrand(m_context, [NSString stringWithFormat:@"%sConstructor", className], [m_class retain]);
> 
> Why are we calling -retain here? You should only call -retain when storing a reference into a data member.

That was in the original code. I think we call retain there because we use it as our "wrapped object" in the JSAPIWrapperObject and which is later released during finalization. I guess I could refactor the code so that JSAPIWrapperObject does the retain when storing its wrappedObject data member.

[Mark Hahnenberg]

Created attachment 194266 [details]
Patch

[Early Warning System Bot]

Comment on attachment 194266 [details]
Patch

Attachment 194266 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17197714

[Early Warning System Bot]

Comment on attachment 194266 [details]
Patch

Attachment 194266 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17225485

[Mark Hahnenberg]

Created attachment 194273 [details]
Patch

[Geoffrey Garen]

Comment on attachment 194273 [details]
Patch

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

r=me

> Source/JavaScriptCore/API/JSAPIWrapperObject.mm:85
> +    m_wrappedObject = [static_cast<id>(wrappedObject) retain];

Let's make m_wrappedObject a RetainPtr. This can be a follow-up patch.

[Mark Hahnenberg]

Committed r146494: <http://trac.webkit.org/changeset/146494>