181274 2018-01-03 20:19:40 -0800 REGRESSION (r212929): WKSnapshotConfiguration may leak an NSNumber when deallocated 2018-01-04 12:29:30 -0800 1 1 1 Unclassified WebKit WebKit2 Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 161450 1 ddkilzer ddkilzer ap commit-queue jbedard joepeck thorton webkit-bug-importer oldest_to_newest 1385669 0 ddkilzer 2018-01-03 20:19:40 -0800 Source/WebKit/UIProcess/API/Cocoa/WKSnapshotConfiguration.mm:31:1: warning: 'WKSnapshotConfiguration' lacks a 'dealloc' instance method but must release '_snapshotWidth' @implementation WKSnapshotConfiguration ^ 1 warning generated. Regressed with: Bug 161450: No reliable way to get a snapshot of WKWebView <https://bugs.webkit.org/show_bug.cgi?id=161450> Found by building WebKit project with clang static analyzer. 1385680 1 330439 ddkilzer 2018-01-03 20:35:31 -0800 Created attachment 330439 Patch v1 1385687 2 ddkilzer 2018-01-03 20:39:22 -0800 If the NSNumber object isn't set, or if it's a tagged pointer, there's probably no leak in practice, but there isn't a guarantee that a tagged pointer will always be used. 1385692 3 330439 joepeck 2018-01-03 21:02:23 -0800 Comment on attachment 330439 Patch v1 r=me! Nice 1385693 4 330439 commit-queue 2018-01-03 21:23:58 -0800 Comment on attachment 330439 Patch v1 Clearing flags on attachment: 330439 Committed r226391: <https://trac.webkit.org/changeset/226391> 1385694 5 commit-queue 2018-01-03 21:23:59 -0800 All reviewed patches have been landed. Closing bug. 1385695 6 webkit-bug-importer 2018-01-03 21:24:54 -0800 <rdar://problem/36290876> 1385803 7 jbedard 2018-01-04 07:43:15 -0800 Did our leak checker not identify this? 1385837 8 ddkilzer 2018-01-04 09:41:36 -0800 (In reply to Jonathan Bedard from comment #7) > Did our leak checker not identify this? This bug was not a leak, it was an over-released object, so the leak checker wouldn't have caught it. Our internal static analysis bot would have caught it, but it's using a build of macOS prior to 10.13.2, so this particular code would not have been compiled or analyzed. 1385846 9 joepeck 2018-01-04 10:39:35 -0800 (In reply to David Kilzer (:ddkilzer) from comment #8) > (In reply to Jonathan Bedard from comment #7) > > Did our leak checker not identify this? > > This bug was not a leak, it was an over-released object, so the leak checker > wouldn't have caught it. Hmm, I think this was a leak and not an over-released object. ddkilzer has been a strong proponent of having the static analyzer catch these kinds of issues, and I think the ability to 'detect leaks of strong/copy/retain properties not released in dealloc' was added to the analyzer a couple years ago: https://reviews.llvm.org/D17511 1385869 10 ddkilzer 2018-01-04 11:49:40 -0800 (In reply to Joseph Pecoraro from comment #9) > (In reply to David Kilzer (:ddkilzer) from comment #8) > > (In reply to Jonathan Bedard from comment #7) > > > Did our leak checker not identify this? > > > > This bug was not a leak, it was an over-released object, so the leak checker > > wouldn't have caught it. > > Hmm, I think this was a leak and not an over-released object. > > ddkilzer has been a strong proponent of having the static analyzer catch > these kinds of issues, and I think the ability to 'detect leaks of > strong/copy/retain properties not released in dealloc' was added to the > analyzer a couple years ago: > https://reviews.llvm.org/D17511 Oops! Sorry, I was thinking of a different bug (Bug 181272) that was an over-release. This was a leak! However, this leak may not have been caught by the leaks bot if this code path was not exercised in our (layout) tests, or if the NSNumber was a tagged pointer (I think). As Joseph mentioned, though, the clang static analyzer should have caught this (assuming it runs the Objective-C dealloc checker). 1385883 11 jbedard 2018-01-04 12:29:30 -0800 (In reply to David Kilzer (:ddkilzer) from comment #10) > .... > > However, this leak may not have been caught by the leaks bot if this code > path was not exercised in our (layout) tests, or if the NSNumber was a > tagged pointer (I think). > > As Joseph mentioned, though, the clang static analyzer should have caught > this (assuming it runs the Objective-C dealloc checker). I have the answer to my own question. We DO use this when running layout tests, added here: <https://trac.webkit.org/changeset/216462/webkit>. But, this code path is only used when running WebKit2 tests (which we don't run leak checking on) and is only used when we have a real IOSURFACE, meaning on-device testing (which we also don't run leak checking on) 330439 2018-01-03 20:35:31 -0800 2018-01-03 21:23:58 -0800 Patch v1 bug-181274-20180103203530.patch text/plain 1325 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjI2MzE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDM1MDhlODhjNzkxZTcwNDM4 N2IyN2Q2N2M2Nzk5MTY2NzcyZjRlNTMuLjRiYjVmMzQ0NzdiNGFhZDA1NThiZjgwMGY3OTM0MWNh ZTgxZDEwYmEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTgtMDEtMDMgIERhdmlkIEtp bHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAgICBSRUdSRVNTSU9OIChyMjEyOTI5 KTogV0tTbmFwc2hvdENvbmZpZ3VyYXRpb24gbWF5IGxlYWsgYW4gTlNOdW1iZXIgd2hlbiBkZWFs bG9jYXRlZAorICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMTgxMjc0PgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogVUlQcm9jZXNzL0FQSS9D b2NvYS9XS1NuYXBzaG90Q29uZmlndXJhdGlvbi5tbToKKyAgICAgICAgKC1bV0tTbmFwc2hvdENv bmZpZ3VyYXRpb24gZGVhbGxvY10pOiBJbXBsZW1lbnQgbWV0aG9kIGFuZAorICAgICAgICByZWxl YXNlIF9zbmFwc2hvdFdpZHRoLgorCiAyMDE4LTAxLTAzICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHpl ckBhcHBsZS5jb20+CiAKICAgICAgICAgRW5hYmxlIC1XY2FzdC1xdWFsIGZvciBXZWJJbnNwZWN0 b3JVSSwgV2ViS2l0TGVnYWN5LCBXZWJLaXQgcHJvamVjdHMKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJLaXQvVUlQcm9jZXNzL0FQSS9Db2NvYS9XS1NuYXBzaG90Q29uZmlndXJhdGlvbi5tbSBiL1Nv dXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9Db2NvYS9XS1NuYXBzaG90Q29uZmlndXJhdGlvbi5t bQppbmRleCA1OTJiZTZmNDA2NjIwNWE3MDViOGJjYzY3NjM3YjA1OTc4MjJlODE0Li41YjUwZDI1 ODZhODdkMGEwNWQ3YTA3OTk0OTc1OGQwZjdmYTk2MWY0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi S2l0L1VJUHJvY2Vzcy9BUEkvQ29jb2EvV0tTbmFwc2hvdENvbmZpZ3VyYXRpb24ubW0KKysrIGIv U291cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL0NvY29hL1dLU25hcHNob3RDb25maWd1cmF0aW9u Lm1tCkBAIC0zOSw2ICszOSwxMiBAQCAtIChpbnN0YW5jZXR5cGUpaW5pdAogICAgIHJldHVybiBz ZWxmOwogfQogCistICh2b2lkKWRlYWxsb2MKK3sKKyAgICBbX3NuYXBzaG90V2lkdGggcmVsZWFz ZV07CisKKyAgICBbc3VwZXIgZGVhbGxvY107Cit9CiAKIC0gKGlkKWNvcHlXaXRoWm9uZTooTlNa b25lICopem9uZQogewo=