Bug 201803 – REGRESSION (r233409): Leak of NSMapTable in -[JSVirtualMachine addManagedReference:withOwner:]

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

-[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>

[Radar WebKit Bug Importer]

<rdar://problem/55377721>

[David Kilzer (:ddkilzer)]

Created attachment 378810 [details]
Patch v1

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 378810 [details]
Patch v1

Clearing flags on attachment: 378810

Committed r249885: <https://trac.webkit.org/changeset/249885>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Seriously, could have just added back the release, but rewrite to use RetainPtr was OK.

[David Kilzer (:ddkilzer)]

(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).