Bug 61311 – REGRESSION (86992): World leak when all windows closed

• Component: WebKit2
• Status: RESOLVED FIXED

[Jer Noble]

REGRESSION (r86990:86992): World leak when all windows closed

[Jer Noble]

Created attachment 94495 [details]
Patch

[Jer Noble]

<rdar://problem/9486740>

[Jer Noble]

Caused by http://trac.webkit.org/changeset/86992.

[Darin Adler]

Comment on attachment 94495 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:-141
> -    [webView retain];
> -    [_webView release];

Are you sure this is necessary? The way this cycle is broken is by calling setWebView:0. Can we leave out this change please? Does the world leak go away if you leave this in?

[Jer Noble]

Comment on attachment 94495 [details]
Patch

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

>> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:-141
>> -    [_webView release];
> 
> Are you sure this is necessary? The way this cycle is broken is by calling setWebView:0. Can we leave out this change please? Does the world leak go away if you leave this in?

I think either this, or the "_data->_fullScreenWindowController = nullptr;" change in [WKView closeFullScreenWindowController] would be sufficient to fix this leak.  However, the retain-cycle would still be there, and there may still be an unfound leak because of it.

For safety's sake, we could add a call in [WKView dealloc] which explicitly called [_data->_fullScreenWindowController.get() setWebVIew:nil], and that would be enough to ensure that WKFullScreenWindowController was never pointing to a deleted object.

[Jer Noble]

Created attachment 94508 [details]
Patch

[Darin Adler]

(In reply to comment #5)
> For safety's sake, we could add a call in [WKView dealloc] which explicitly called [_data->_fullScreenWindowController.get() setWebVIew:nil], and that would be enough to ensure that WKFullScreenWindowController was never pointing to a deleted object.

Except under GC.

[Darin Adler]

Comment on attachment 94508 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:-141
> -    [webView retain];
> -    [_webView release];

I still don’t think it was best to remove these. A retain cycle is not intrinsically incorrect, as long as something breaks the cycle.

[Jer Noble]

Comment on attachment 94508 [details]
Patch

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

>> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:-141
>> -    [_webView release];
> 
> I still don’t think it was best to remove these. A retain cycle is not intrinsically incorrect, as long as something breaks the cycle.

Okay, I'll upload a new patch that puts these back in.

[Jer Noble]

(In reply to comment #9)
> (From update of attachment 94508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94508&action=review
> 
> >> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:-141
> >> -    [_webView release];
> > 
> > I still don’t think it was best to remove these. A retain cycle is not intrinsically incorrect, as long as something breaks the cycle.
> 
> Okay, I'll upload a new patch that puts these back in.

Oh! r+! In that case, I'll check-in a patch that puts these back in.  Thanks!

[Jer Noble]

Committed r87113: <http://trac.webkit.org/changeset/87113>