REGRESSION (r212929): WKSnapshotConfiguration may leak an NSNumber when deallocated

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 330439 [details]
Patch v1

[David Kilzer (:ddkilzer)]

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.

[Joseph Pecoraro]

Comment on attachment 330439 [details]
Patch v1

r=me! Nice

[WebKit Commit Bot]

Comment on attachment 330439 [details]
Patch v1

Clearing flags on attachment: 330439

Committed r226391: <https://trac.webkit.org/changeset/226391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36290876>

[Jonathan Bedard]

Did our leak checker not identify this?

[David Kilzer (:ddkilzer)]

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

[Joseph Pecoraro]

(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

[David Kilzer (:ddkilzer)]

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

[Jonathan Bedard]

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