Bug 89817 - webkitCancelFullScreen() is not working for fullscreen elements in iframes
Summary: webkitCancelFullScreen() is not working for fullscreen elements in iframes
Status: CLOSED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Max Feil
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-23 14:34 PDT by Max Feil
Modified: 2012-11-02 12:47 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.10 KB, patch)
2012-06-23 14:42 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2012-06-26 17:19 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (867.09 KB, application/zip)
2012-06-26 22:43 PDT, WebKit Review Bot
no flags Details
Patch (6.42 KB, patch)
2012-06-27 15:35 PDT, Max Feil
mfeil: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (533.41 KB, application/zip)
2012-06-27 19:05 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Max Feil 2012-06-23 14:34:33 PDT
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.
Comment 1 Max Feil 2012-06-23 14:42:55 PDT
Created attachment 149182 [details]
Patch
Comment 2 Antonio Gomes 2012-06-25 11:42:41 PDT
Comment on attachment 149182 [details]
Patch

It needs a test.
Comment 3 Max Feil 2012-06-25 14:20:14 PDT
(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.
Comment 4 Antonio Gomes 2012-06-25 14:54:18 PDT
(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.
Comment 5 Max Feil 2012-06-26 17:19:30 PDT
Created attachment 149640 [details]
Patch
Comment 6 Max Feil 2012-06-26 17:37:59 PDT
(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.
Comment 7 Jer Noble 2012-06-26 19:07:27 PDT
(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?
Comment 8 WebKit Review Bot 2012-06-26 22:42:57 PDT
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
Comment 9 WebKit Review Bot 2012-06-26 22:43:00 PDT
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
Comment 10 Max Feil 2012-06-27 14:31:21 PDT
(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).
Comment 11 Max Feil 2012-06-27 15:35:48 PDT
Created attachment 149806 [details]
Patch
Comment 12 Jer Noble 2012-06-27 19:01:01 PDT
(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.
Comment 13 WebKit Review Bot 2012-06-27 19:04:48 PDT
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
Comment 14 WebKit Review Bot 2012-06-27 19:05:08 PDT
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
Comment 15 Max Feil 2012-06-29 16:15:31 PDT
(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.
Comment 16 Max Feil 2012-06-29 17:41:10 PDT
Created bug 90327 to track BlackBerry-specific fix for this problem.
Comment 17 Max Feil 2012-11-02 12:47:04 PDT
BlackBerry specific fix has been in place for a long time now. Closing.