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.
Created attachment 150283 [details] Patch
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
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
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?
(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.
(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?
(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?
(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.
Created attachment 151116 [details] Patch
Comment on attachment 151116 [details] Patch Clearing flags on attachment: 151116 Committed r122035: <http://trac.webkit.org/changeset/122035>
All reviewed patches have been landed. Closing bug.
Closing bug for patch that landed a long time ago.