-[JSVirtualMachine addManagedReference:withOwner:] leaks an NSMapTable every time a new one is created. - (void)addManagedReference:(id)object withOwner:(id)owner { [...] NSMapTable *ownedObjects = [m_externalObjectGraph objectForKey:owner]; // ownedObjects is retained by m_externalObjectGraph. if (!ownedObjects) { NSPointerFunctionsOptions weakIDOptions = NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPersonality; NSPointerFunctionsOptions integerOptions = NSPointerFunctionsOpaqueMemory | NSPointerFunctionsIntegerPersonality; ownedObjects = [[NSMapTable alloc] initWithKeyOptions:weakIDOptions valueOptions:integerOptions capacity:1]; // ownedObjects is +1 retained by -alloc. [m_externalObjectGraph setObject:ownedObjects forKey:owner]; // ownedObjects is +2 retained by m_externalObjectGraph. } size_t count = reinterpret_cast<size_t>(NSMapGet(ownedObjects, (__bridge void*)object)); NSMapInsert(ownedObjects, (__bridge void*)object, reinterpret_cast<void*>(count + 1)); // FIXME: When `ownedObjects` is created, it leaks one reference count from -alloc when returning from this method. } <https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/API/JSVirtualMachine.mm#L167> Caused by: Bug 186973: [Cocoa] Improve ARC compatibility of more code in JavaScriptCore <https://bugs.webkit.org/show_bug.cgi?id=186973>
<rdar://problem/55377721>
Created attachment 378810 [details] Patch v1
Wow that was a mismerge. I had a patch that actually converted to ARC, and when making the "prepare to use ARC" version I accidentally left in the deletion of the call to release. It would have been simpler to just add the release back in, but it's also OK to use RetainPtr.
Comment on attachment 378810 [details] Patch v1 Clearing flags on attachment: 378810 Committed r249885: <https://trac.webkit.org/changeset/249885>
All reviewed patches have been landed. Closing bug.
Seriously, could have just added back the release, but rewrite to use RetainPtr was OK.
(In reply to Darin Adler from comment #6) > Seriously, could have just added back the release, but rewrite to use > RetainPtr was OK. I didn't add back the -release because: - It would need to be removed to enable ARC again. - Extra insight is required to realize `ownedObjects` isn't being over-released (and is thus still valid) because it was added to m_externalObjectGraph. I'm not sure if the clang static analyzer would have enough context to "know" this. - RetainPtr<> is basically doing the same thing ARC would have done W.r.t. retain counting `ownedObjects`. - I assume that folks will make additional passes through WebKit to remove the use of unnecessary RetainPtr<> for NS objects once the conversion to ARC occurs (and isn't rolled out).