Bug 90327 – [BlackBerry] exitFullScreenForElement() is not working for fullscreen elements in iframes

• Component: WebKit BlackBerry
• Status: CLOSED FIXED

[Max Feil]

This problem was discovered in the BlackBerry port during manual testing of fullscreen video at:
http://w3schools.com/html5/tryit.asp?filename=tryhtml5_video_all
See PR168219.

The video element is in an iframe, and exiting fullscreen resulted in a crash. The exitFullScreenForElement() call was being made on the topDocument (which has a null m_fullScreenElement) instead of the iframe's document.

The Chromium and Windows ports get around this problem by storing either the fullscreen element or its frame during enterFullScreenForElement(), so I will bring the BlackBerry port in line with this. See also bug 89817.

[Max Feil]

Created attachment 150283 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150283 [details]
Patch

Attachment 150283 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13114572

New failing tests:
fullscreen/exit-full-screen-iframe.html

[WebKit Review Bot]

Created attachment 150296 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Antonio Gomes]

Comment on attachment 150283 [details]
Patch

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

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:739
> +    ASSERT(m_fullScreenElement);
> +    if (m_fullScreenElement) {

Max, if it can be null, why do you assert that it is always non-null?

[Max Feil]

(In reply to comment #4)
> > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:739
> > +    ASSERT(m_fullScreenElement);
> > +    if (m_fullScreenElement) {
> 
> Max, if it can be null, why do you assert that it is always non-null?

The element parameter to exitFullScreenForElement() can be null (for reasons I don't fully agree with). But the member variable m_fullScreenElement is set by enterFullScreenForElement() whose element parameter is never null, so the member variable should never be null at this point (except in the pathological case of mismatched enter/exit calls). See bug 89817 for more background info.

[Antonio Gomes]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:739
> > > +    ASSERT(m_fullScreenElement);
> > > +    if (m_fullScreenElement) {
> > 
> > Max, if it can be null, why do you assert that it is always non-null?
> 
> The element parameter to exitFullScreenForElement() can be null (for reasons I don't fully agree with). But the member variable m_fullScreenElement is set by enterFullScreenForElement() whose element parameter is never null, so the member variable should never be null at this point (except in the pathological case of mismatched enter/exit calls). See bug 89817 for more background info.

I understand that part from the other bug. So why not just remove the 'if' and keep the assert?

[Max Feil]

(In reply to comment #6)
> I understand that part from the other bug. So why not just remove the 'if' and keep the assert?

I think it is better in release builds to do nothing rather than crash if exitFullScreenForElement() is called when m_fullScreenElement is empty. In many ways a null m_fullScreenElement means we are not knowingly in fullscreen mode, so there is nothing we should do. I felt this was clear from the code?

[Antonio Gomes]

(In reply to comment #7)
> (In reply to comment #6)
> > I understand that part from the other bug. So why not just remove the 'if' and keep the assert?
> 
> I think it is better in release builds to do nothing rather than crash if exitFullScreenForElement() is called when m_fullScreenElement is empty.

I particularly prefer it to crash, so we fix it if it is a real bug. If we are trying to catch the cases when it can happen, then we can keep both the assert and if check temporarily, but I'd dislike this pattern to live forever.

[Max Feil]

Created attachment 151116 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151116 [details]
Patch

Clearing flags on attachment: 151116

Committed r122035: <http://trac.webkit.org/changeset/122035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Feil]

Closing bug for patch that landed a long time ago.