Bug 120792

Summary: WKFullScreenWindowController extends lifetime of WKView, deleting it at a wrong time
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: MediaAssignee: Alexey Proskuryakov <ap>
Severity: Normal CC: eric.carlson, jer.noble
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
proposed fix jer.noble: review+

Description Alexey Proskuryakov 2013-09-05 13:52:09 PDT
WKFullScreenWindowController has a strong reference to WKView, so when it is instantiated, the view is only deleted when the controller is deleted.

This finally happens in WebPageProxy::resetStateAfterProcessExited() when it resets m_fullScreenManager. But further calls in this function rely on WKView and its associated clients still being there. So, we crash.

I don't see why WKFullScreenWindowController needs to retain the view - when the view is deleted, there is no need for the controller anyway, everything is just closed normally.

Comment 1 Alexey Proskuryakov 2013-09-05 14:00:44 PDT
Created attachment 210659 [details]
proposed fix
Comment 2 Jer Noble 2013-09-05 14:54:46 PDT
Comment on attachment 210659 [details]
proposed fix

LGTM, r=me with nit.

The only thing I worry about is the lifetime of the WKFullScreenWindow being extended (due to being retained by something other than the WKView, or by a -performSelector:afterDelay:) somehow, and thus holding onto an invalid _webView pointer.

Perhaps the _webView could stay unretained, but the -setWebView: method could also stay, and would be cleared out by WKView in its own -dealloc method?  We could even add a "@property (assign) WKView* webView" declaration to make the semantic explicit.
Comment 3 Alexey Proskuryakov 2013-09-05 15:06:56 PDT
That's a great nit.

I added an assignment to nil to -close, as this function is inevitably called when WKView is destroyed.

Committed <http://trac.webkit.org/r155153>.