Bug 166957 – [WK2][Cocoa] Avoid null dereference in Fullscreen code.

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/27745004>

[Brent Fulgham]

Created attachment 298649 [details]
Patch

[Eric Carlson]

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.

[Brent Fulgham]

Committed r210619: <http://trac.webkit.org/changeset/210619>

[Brent Fulgham]

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!