Bug 120792 - WKFullScreenWindowController extends lifetime of WKView, deleting it at a wrong time
Summary: WKFullScreenWindowController extends lifetime of WKView, deleting it at a wro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-05 13:52 PDT by Alexey Proskuryakov
Modified: 2013-09-05 15:06 PDT (History)
2 users (show)

See Also:


Attachments
proposed fix (4.32 KB, patch)
2013-09-05 14:00 PDT, Alexey Proskuryakov
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.

<rdar://problem/14884666>
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>.