Bug 89817 – webkitCancelFullScreen() is not working for fullscreen elements in iframes

• Component: New Bugs
• Status: CLOSED WONTFIX

[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

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.

I also found 3 lines of dead code that I propose removing to reduce confusion, and a missing "else" statement.

[Max Feil]

Created attachment 149182 [details]
Patch

[Antonio Gomes]

Comment on attachment 149182 [details]
Patch

It needs a test.

[Max Feil]

(In reply to comment #2)
> It needs a test.

I know it needs a test. That's why I said a test is pending.

I would like Jer to review the patch first, so that I don't waste time writing a test for code that needs to change.

[Antonio Gomes]

(In reply to comment #3)
> (In reply to comment #2)
> > It needs a test.
> 
> I know it needs a test. That's why I said a test is pending.
> 
> I would like Jer to review the patch first, so that I don't waste time writing a test for code that needs to change.

you will need the test regardless if the code need to change. I could review it, but if you want him and only him, please ping him on freenode / #webkit.

[Max Feil]

Created attachment 149640 [details]
Patch

[Max Feil]

(In reply to comment #4)
> you will need the test regardless if the code need to change. I could review it, but if you want him and only him, please ping him on freenode / #webkit.

I added a test. Jer is on parental leave until July 13, so I think we should go ahead with this and he can fine tune it when he gets back, if necessary.

[Jer Noble]

(In reply to comment #0)
> 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
> 
> 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.

It seems your patch is rather heavy handed.

Why doesn't your port just null-check the parameter passed to exitFullScreenForElement()?  Or if that's unreasonable, why doesn't Document::webkitCancelFullScreen() pass 'this' or the (previous) top of the full screen element stack instead of 'm_fullScreenElement' if 'm_fullScreenElement' is null?

[WebKit Review Bot]

Comment on attachment 149640 [details]
Patch

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

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

[WebKit Review Bot]

Created attachment 149681 [details]
Archive of layout-test-results from ec2-cr-linux-04

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

[Max Feil]

(In reply to comment #7)
> It seems your patch is rather heavy handed.
> 
> Why doesn't your port just null-check the parameter passed to exitFullScreenForElement()?  Or if that's unreasonable, why doesn't Document::webkitCancelFullScreen() pass 'this' or the (previous) top of the full screen element stack instead of 'm_fullScreenElement' if 'm_fullScreenElement' is null?

Null checking the parameter passed to ChromeClient*::exitFullScreenForElement() is not good because it is important for the chrome client function to be able to determine the correct document. With a null element it cannot do this. And if the document cannot be determined, 
Document::webkitDidExitFullScreenForElement() cannot be called, which means that the m_fullScreenElement for that document will remain set. This has a host of bad implications, including code continuing to think that fullscreen mode is active. Look at how the boolean function Document::webkitIsFullScreen() is defined to return true whenever m_fullScreenElement is set for that document.

Similarly, passing 'this' gives the wrong document so the m_fullScreenElement of the iframe's document will never be reset back to 0. Same goes for the (previous) top of the fullscreen element stack because we are at the topDocument.

From my study of the code, the only proper way to fix this bug is to pass an element to ChromeClient*::exitFullScreenForElement() which is associated with the actual document that has m_fullScreenElement set (there will be only one).

[Max Feil]

Created attachment 149806 [details]
Patch

[Jer Noble]

(In reply to comment #10)
> (In reply to comment #7)
> > It seems your patch is rather heavy handed.
> > 
> > Why doesn't your port just null-check the parameter passed to exitFullScreenForElement()?  Or if that's unreasonable, why doesn't Document::webkitCancelFullScreen() pass 'this' or the (previous) top of the full screen element stack instead of 'm_fullScreenElement' if 'm_fullScreenElement' is null?
> 
> Null checking the parameter passed to ChromeClient*::exitFullScreenForElement() is not good because it is important for the chrome client function to be able to determine the correct document. With a null element it cannot do this. And if the document cannot be determined, 
> Document::webkitDidExitFullScreenForElement() cannot be called, which means that the m_fullScreenElement for that document will remain set. This has a host of bad implications, including code continuing to think that fullscreen mode is active. Look at how the boolean function Document::webkitIsFullScreen() is defined to return true whenever m_fullScreenElement is set for that document.
> 
> Similarly, passing 'this' gives the wrong document so the m_fullScreenElement of the iframe's document will never be reset back to 0. Same goes for the (previous) top of the fullscreen element stack because we are at the topDocument.
> 
> From my study of the code, the only proper way to fix this bug is to pass an element to ChromeClient*::exitFullScreenForElement() which is associated with the actual document that has m_fullScreenElement set (there will be only one).

Your port has already been notified of the full screen element in ChromeClient::enterFullscreenForElement().  Just store that value, which is what other ports do.  Honestly, the "element" parameter in the exit-category full screen functions is unnecessary and can be determined through other means if necessary. I'd rather remove it if it's causing problems rather than add more complexity.

[WebKit Review Bot]

Comment on attachment 149806 [details]
Patch

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

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

[WebKit Review Bot]

Created attachment 149852 [details]
Archive of layout-test-results from ec2-cr-linux-04

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

[Max Feil]

(In reply to comment #12)
> Your port has already been notified of the full screen element in ChromeClient::enterFullscreenForElement().  Just store that value, which is what other ports do.  Honestly, the "element" parameter in the exit-category full screen functions is unnecessary and can be determined through other means if necessary. I'd rather remove it if it's causing problems rather than add more complexity.

I can see your point. I checked the other ports and Chromium & Windows store either the fullscreen frame or fullscreen element. The ports that use the element directly and (likely) crash are GTK, EFL and BlackBerry. The mac port I couldn't tell exactly because I'm not familiar with mm files (but there is at least a simple null check on the element before webkit{Will,Did}ExitFullScreenForElement() are called on the document). And the Qt port doesn't look like it has implemented enter/exitFullScreenForElement at all yet.

I will gladly store a RefPtr to the element in the BlackBerry port. But I also think that the element parameter of exitFullScreenForElement is best removed to halt the misleading impression that enter/exitFullScreenForElement() are two nice symmetrical calls.

[Max Feil]

Created bug 90327 to track BlackBerry-specific fix for this problem.

[Max Feil]

BlackBerry specific fix has been in place for a long time now. Closing.