Crash tracing statistics indicate that the 'WebVideoFullScreenManager::didSetupFullscreen' encounters stability issues under certain conditions. We do not have a reproducible test case showing this behavior, but code inspection indicates that the page values in the completion Block were not being referenced consistently with other methods in the same class. It also did not account for the possibility that the page object had been cleared between starting the dispatch and when the block actually runs.
<rdar://problem/27745004>
Created attachment 298649 [details] Patch
Comment on attachment 298649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298649&action=review > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:333 > + if (strongThis->m_page) > + strongThis->m_page->send(Messages::WebVideoFullscreenManagerProxy::EnterFullscreen(contextId), strongThis->m_page->pageID()); Nit: "strongThis" is only really necessary for the "if". Leaving the second line with implied "this" looks cleaner IMO. It would probably be even better to return early if m_page is NULL.
Committed r210619: <http://trac.webkit.org/changeset/210619>
Comment on attachment 298649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298649&action=review >> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:333 >> + strongThis->m_page->send(Messages::WebVideoFullscreenManagerProxy::EnterFullscreen(contextId), strongThis->m_page->pageID()); > > Nit: "strongThis" is only really necessary for the "if". Leaving the second line with implied "this" looks cleaner IMO. It would probably be even better to return early if m_page is NULL. OK!