Bug 201803 - REGRESSION (r233409): Leak of NSMapTable in -[JSVirtualMachine addManagedReference:withOwner:]
Summary: REGRESSION (r233409): Leak of NSMapTable in -[JSVirtualMachine addManagedRefe...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
Keywords: InRadar
Depends on:
Reported: 2019-09-15 06:11 PDT by David Kilzer (:ddkilzer)
Modified: 2019-09-15 10:56 PDT (History)
11 users (show)

See Also:

Patch v1 (2.65 KB, patch)
2019-09-15 06:42 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2019-09-15 06:11:07 PDT
-[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.


Caused by:

Bug 186973: [Cocoa] Improve ARC compatibility of more code in JavaScriptCore
Comment 1 Radar WebKit Bug Importer 2019-09-15 06:12:22 PDT
Comment 2 David Kilzer (:ddkilzer) 2019-09-15 06:42:30 PDT
Created attachment 378810 [details]
Patch v1
Comment 3 Darin Adler 2019-09-15 09:33:43 PDT
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 4 WebKit Commit Bot 2019-09-15 10:17:09 PDT
Comment on attachment 378810 [details]
Patch v1

Clearing flags on attachment: 378810

Committed r249885: <https://trac.webkit.org/changeset/249885>
Comment 5 WebKit Commit Bot 2019-09-15 10:17:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2019-09-15 10:48:23 PDT
Seriously, could have just added back the release, but rewrite to use RetainPtr was OK.
Comment 7 David Kilzer (:ddkilzer) 2019-09-15 10:56:03 PDT
(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).