Bug 158716 - False-positive over-release of WebView in destroyWebViewAndOffscreenWindow() in DumpRenderTree.mm
Summary: False-positive over-release of WebView in destroyWebViewAndOffscreenWindow() ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-13 18:16 PDT by David Kilzer (:ddkilzer)
Modified: 2016-06-14 11:00 PDT (History)
9 users (show)

See Also:


Attachments
Patch v1 (2.55 KB, patch)
2016-06-13 18:23 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (2.64 KB, patch)
2016-06-13 18:41 PDT, David Kilzer (:ddkilzer)
aestes: review+
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) 2016-06-13 18:16:44 PDT
The clang static analyzer complains that 'webView' is over-released in destroyWebViewAndOffscreenWindow() in DumpRenderTree.mm.

The current code retains 'webView' in createWebViewAndOffscreenWindow(), returns it, then assumes that [mainFrame webView] returns the exact same WebView in destroyWebViewAndOffscreenWindow().
Comment 1 David Kilzer (:ddkilzer) 2016-06-13 18:23:08 PDT
Created attachment 281220 [details]
Patch v1
Comment 2 Andy Estes 2016-06-13 18:31:38 PDT
Comment on attachment 281220 [details]
Patch v1

Patch looks good, but you seem to be running into problems because DumpRenderTreeMac.h is included in a .cpp file. r=me if you straighten that out.
Comment 3 David Kilzer (:ddkilzer) 2016-06-13 18:41:46 PDT
Created attachment 281222 [details]
Patch v2
Comment 4 Andy Estes 2016-06-13 18:43:28 PDT
Comment on attachment 281222 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=281222&action=review

> Tools/DumpRenderTree/mac/DumpRenderTreeMac.h:71
> +WebView* createWebViewAndOffscreenWindow() NS_RETURNS_RETAINED;

I missed this the first time around, but the space should go before the *, not after.
Comment 5 David Kilzer (:ddkilzer) 2016-06-13 19:26:22 PDT
Committed r202020: <http://trac.webkit.org/changeset/202020>
Comment 6 David Kilzer (:ddkilzer) 2016-06-13 19:27:45 PDT
Comment on attachment 281222 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=281222&action=review

>> Tools/DumpRenderTree/mac/DumpRenderTreeMac.h:71
>> +WebView* createWebViewAndOffscreenWindow() NS_RETURNS_RETAINED;
> 
> I missed this the first time around, but the space should go before the *, not after.

Fixed!
Comment 7 Andy Estes 2016-06-13 21:01:06 PDT
For reasons I don't understand, this broke the El Capitan CMake Debug build: https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/5853

Seems like CMake is somehow ignoring the #if __OBJC__?
Comment 8 Andy Estes 2016-06-13 21:03:08 PDT
Alex says this is a bug with the CMake build system. All files in DumpRenderTree are being built with -ObjC++.
Comment 9 Joseph Pecoraro 2016-06-13 21:09:33 PDT
(In reply to comment #8)
> Alex says this is a bug with the CMake build system. All files in
> DumpRenderTree are being built with -ObjC++.

Source/WebKit/PlatformMac.cmake has what I think would be an example of how to do this:

> foreach (_file ${WebKit_SOURCES})
>     list(FIND C99_FILES ${_file} _c99_index)
>     if (${_c99_index} EQUAL -1)
>         set_source_files_properties(${_file} PROPERTIES COMPILE_FLAGS "-ObjC++ -std=c++14")
>     else ()
>         set_source_files_properties(${_file} PROPERTIES COMPILE_FLAGS -std=c99)
>     endif ()
> endforeach ()
Comment 10 David Kilzer (:ddkilzer) 2016-06-14 11:00:27 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Alex says this is a bug with the CMake build system. All files in
> > DumpRenderTree are being built with -ObjC++.
> 
> Source/WebKit/PlatformMac.cmake has what I think would be an example of how
> to do this:
> 
> > foreach (_file ${WebKit_SOURCES})
> >     list(FIND C99_FILES ${_file} _c99_index)
> >     if (${_c99_index} EQUAL -1)
> >         set_source_files_properties(${_file} PROPERTIES COMPILE_FLAGS "-ObjC++ -std=c++14")
> >     else ()
> >         set_source_files_properties(${_file} PROPERTIES COMPILE_FLAGS -std=c99)
> >     endif ()
> > endforeach ()

I filed Bug 158743: REGRESSION (r202020): El Capitan CMake Debug build broken