201803 2019-09-15 06:11:07 -0700 REGRESSION (r233409): Leak of NSMapTable in -[JSVirtualMachine addManagedReference:withOwner:] 2019-09-15 10:56:03 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=186973 InRadar P2 Normal --- 1 ddkilzer ddkilzer commit-queue darin ews-watchlist joepeck keith_miller mark.lam mitz msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1570927 0 ddkilzer 2019-09-15 06:11:07 -0700 -[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> 1570928 1 webkit-bug-importer 2019-09-15 06:12:22 -0700 <rdar://problem/55377721> 1570931 2 378810 ddkilzer 2019-09-15 06:42:30 -0700 Created attachment 378810 Patch v1 1570942 3 darin 2019-09-15 09:33:43 -0700 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. 1570946 4 378810 commit-queue 2019-09-15 10:17:09 -0700 Comment on attachment 378810 Patch v1 Clearing flags on attachment: 378810 Committed r249885: <https://trac.webkit.org/changeset/249885> 1570947 5 commit-queue 2019-09-15 10:17:10 -0700 All reviewed patches have been landed. Closing bug. 1570950 6 darin 2019-09-15 10:48:23 -0700 Seriously, could have just added back the release, but rewrite to use RetainPtr was OK. 1570952 7 ddkilzer 2019-09-15 10:56:03 -0700 (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). 378810 2019-09-15 06:42:30 -0700 2019-09-15 10:17:09 -0700 Patch v1 bug-201803-20190915064357.patch text/plain 2716 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ5ODUyCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA0 ODhiMTc0YWY2ZDZjMDM1MzFhMzlkYjcyNWY1YTM0OGU1NjYzMWE1Li5mN2U5YTkyNjcyNmQzOTUy ZWQ5ZmQyM2ZhZjRmYjNkMzNlYTQwYjAyIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNCBAQAorMjAxOS0wOS0xNSAgRGF2aWQgS2lsemVyICA8ZGRraWx6ZXJAYXBwbGUuY29t PgorCisgICAgICAgIExlYWsgb2YgTlNNYXBUYWJsZSBpbiAtW0pTVmlydHVhbE1hY2hpbmUgYWRk TWFuYWdlZFJlZmVyZW5jZTp3aXRoT3duZXI6XQorICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3Jn L2IvMjAxODAzPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogQVBJL0pTVmlydHVhbE1hY2hpbmUubW06CisgICAgICAgICgtW0pTVmlydHVhbE1hY2hp bmUgYWRkTWFuYWdlZFJlZmVyZW5jZTp3aXRoT3duZXI6XSk6IFVzZQorICAgICAgICBSZXRhaW5Q dHI8PiB0byBmaXggdGhlIGxlYWsuCisKIDIwMTktMDktMTMgIFl1c3VrZSBTdXp1a2kgIDx5c3V6 dWtpQGFwcGxlLmNvbT4KIAogICAgICAgICBbSlNDXSBNaWNyby1vcHRpbWl6ZSBZYXJySklUJ3Mg c3Vycm9nYXRlIHBhaXIgaGFuZGxpbmcKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9BUEkvSlNWaXJ0dWFsTWFjaGluZS5tbSBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvSlNW aXJ0dWFsTWFjaGluZS5tbQppbmRleCBkMWI2Yjk0OWVkYzMzOGQ1ZGMzOWIyYTgxOWRiMjE5Yjgy ZmUyY2JjLi5iOWFkMmY2Mjk1MjdlNjE5YmZkNzdkMDE1YTI1ODIwMmQzZTBiMjdmIDEwMDY0NAot LS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL0pTVmlydHVhbE1hY2hpbmUubW0KKysrIGIv U291cmNlL0phdmFTY3JpcHRDb3JlL0FQSS9KU1ZpcnR1YWxNYWNoaW5lLm1tCkBAIC00MCw2ICs0 MCw3IEBACiAjaW1wb3J0IDxtdXRleD4KICNpbXBvcnQgPHd0Zi9CbG9ja1B0ci5oPgogI2ltcG9y dCA8d3RmL0xvY2suaD4KKyNpbXBvcnQgPHd0Zi9SZXRhaW5QdHIuaD4KIAogc3RhdGljIE5TTWFw VGFibGUgKmdsb2JhbFdyYXBwZXJDYWNoZSA9IDA7CiAKQEAgLTE4MCwxNyArMTgxLDE3IEBAIC0g KHZvaWQpYWRkTWFuYWdlZFJlZmVyZW5jZTooaWQpb2JqZWN0IHdpdGhPd25lcjooaWQpb3duZXIK ICAgICAgICAgW3NlbGYgYWRkRXh0ZXJuYWxSZW1lbWJlcmVkT2JqZWN0Om93bmVyXTsKICAKICAg ICBhdXRvIGV4dGVybmFsRGF0YU11dGV4TG9ja2VyID0gaG9sZExvY2sobV9leHRlcm5hbERhdGFN dXRleCk7Ci0gICAgTlNNYXBUYWJsZSAqb3duZWRPYmplY3RzID0gW21fZXh0ZXJuYWxPYmplY3RH cmFwaCBvYmplY3RGb3JLZXk6b3duZXJdOworICAgIFJldGFpblB0cjxOU01hcFRhYmxlPiBvd25l ZE9iamVjdHMgPSBbbV9leHRlcm5hbE9iamVjdEdyYXBoIG9iamVjdEZvcktleTpvd25lcl07CiAg ICAgaWYgKCFvd25lZE9iamVjdHMpIHsKICAgICAgICAgTlNQb2ludGVyRnVuY3Rpb25zT3B0aW9u cyB3ZWFrSURPcHRpb25zID0gTlNQb2ludGVyRnVuY3Rpb25zV2Vha01lbW9yeSB8IE5TUG9pbnRl ckZ1bmN0aW9uc09iamVjdFBlcnNvbmFsaXR5OwogICAgICAgICBOU1BvaW50ZXJGdW5jdGlvbnNP cHRpb25zIGludGVnZXJPcHRpb25zID0gTlNQb2ludGVyRnVuY3Rpb25zT3BhcXVlTWVtb3J5IHwg TlNQb2ludGVyRnVuY3Rpb25zSW50ZWdlclBlcnNvbmFsaXR5OwotICAgICAgICBvd25lZE9iamVj dHMgPSBbW05TTWFwVGFibGUgYWxsb2NdIGluaXRXaXRoS2V5T3B0aW9uczp3ZWFrSURPcHRpb25z IHZhbHVlT3B0aW9uczppbnRlZ2VyT3B0aW9ucyBjYXBhY2l0eToxXTsKKyAgICAgICAgb3duZWRP YmplY3RzID0gYWRvcHROUyhbW05TTWFwVGFibGUgYWxsb2NdIGluaXRXaXRoS2V5T3B0aW9uczp3 ZWFrSURPcHRpb25zIHZhbHVlT3B0aW9uczppbnRlZ2VyT3B0aW9ucyBjYXBhY2l0eToxXSk7CiAK LSAgICAgICAgW21fZXh0ZXJuYWxPYmplY3RHcmFwaCBzZXRPYmplY3Q6b3duZWRPYmplY3RzIGZv cktleTpvd25lcl07CisgICAgICAgIFttX2V4dGVybmFsT2JqZWN0R3JhcGggc2V0T2JqZWN0Om93 bmVkT2JqZWN0cy5nZXQoKSBmb3JLZXk6b3duZXJdOwogICAgIH0KIAotICAgIHNpemVfdCBjb3Vu dCA9IHJlaW50ZXJwcmV0X2Nhc3Q8c2l6ZV90PihOU01hcEdldChvd25lZE9iamVjdHMsIChfX2Jy aWRnZSB2b2lkKilvYmplY3QpKTsKLSAgICBOU01hcEluc2VydChvd25lZE9iamVjdHMsIChfX2Jy aWRnZSB2b2lkKilvYmplY3QsIHJlaW50ZXJwcmV0X2Nhc3Q8dm9pZCo+KGNvdW50ICsgMSkpOwor ICAgIHNpemVfdCBjb3VudCA9IHJlaW50ZXJwcmV0X2Nhc3Q8c2l6ZV90PihOU01hcEdldChvd25l ZE9iamVjdHMuZ2V0KCksIChfX2JyaWRnZSB2b2lkKilvYmplY3QpKTsKKyAgICBOU01hcEluc2Vy dChvd25lZE9iamVjdHMuZ2V0KCksIChfX2JyaWRnZSB2b2lkKilvYmplY3QsIHJlaW50ZXJwcmV0 X2Nhc3Q8dm9pZCo+KGNvdW50ICsgMSkpOwogfQogCiAtICh2b2lkKXJlbW92ZU1hbmFnZWRSZWZl cmVuY2U6KGlkKW9iamVjdCB3aXRoT3duZXI6KGlkKW93bmVyCg==