Summary: | Objective-C API: wrapperClass holds a static JSClassRef, which causes JSGlobalObjects to leak | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, rakuco, rego+ews, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2013-03-20 16:31:33 PDT
Created attachment 194180 [details]
Patch
Comment on attachment 194180 [details] Patch Attachment 194180 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17211574 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 Created attachment 194182 [details]
Patch
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. 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. Created attachment 194266 [details]
Patch
Comment on attachment 194266 [details] Patch Attachment 194266 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17197714 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 Created attachment 194273 [details]
Patch
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. Committed r146494: <http://trac.webkit.org/changeset/146494> |