Bug 56264 - FullScreen: Handle entering full screen security restrictions
Summary: FullScreen: Handle entering full screen security restrictions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-12 23:51 PST by Jer Noble
Modified: 2011-03-14 10:53 PDT (History)
1 user (show)

See Also:


Attachments
Patch (14.77 KB, patch)
2011-03-13 00:19 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (14.96 KB, patch)
2011-03-13 00:26 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (13.72 KB, patch)
2011-03-13 21:52 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-03-12 23:51:10 PST
FullScreen: Handle entering full screen security restrictions
Comment 1 Jer Noble 2011-03-12 23:54:09 PST
Full screen mode should only be activated when handling a user gesture;  Full screen mode should not be allowed within IFRAME elements unless they have been marked as "allowfullscreen".
Comment 2 Jer Noble 2011-03-13 00:19:30 PST
Created attachment 85610 [details]
Patch
Comment 3 Jer Noble 2011-03-13 00:22:39 PST
Somehow, I lost most of my comments in the WebCore/ChangeLog.  I'll update the file and repost the patch.
Comment 4 Jer Noble 2011-03-13 00:26:36 PST
Created attachment 85611 [details]
Patch
Comment 5 Eric Carlson 2011-03-13 17:29:44 PDT
Comment on attachment 85611 [details]
Patch

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

> LayoutTests/fullscreen/full-screen-iframe-allowed.html:22
> +    runWithKeyDown(function() {
> +
> +        setTimeout(function() { 
> +            consoleWrite("FAIL - did not enter full screen!"); 
> +            endTest(); 
> +        }, 50);
> +
> +        frame.contentDocument.documentElement.webkitRequestFullScreen();
> +    });

This makes it look like the 'webkitfullscreenchange' event is posted synchronously, but the Mozilla spec says it should be asynch: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch this event." If the event is being posted aynchronously, using a timeout to detect success/failure will lead to problems as it is timing dependent.

> Source/WebCore/html/HTMLAttributeNames.in:302
>  webkitdirectory
> +webkitallowfullscreen

Should these be sorted?

> Source/WebCore/html/HTMLFrameElementBase.cpp:60
> +#if ENABLE(FULLSCREEN_API)
> +    , m_allowFullScreen(false)
> +#endif

See below.

> Source/WebCore/html/HTMLFrameElementBase.cpp:154
> +#if ENABLE(FULLSCREEN_API)
> +    } else if (attr->name() == webkitallowfullscreenAttr) {
> +        // Boolean argument; the attr is irrelevant.
> +        m_allowFullScreen = true;
> +#endif

See below.

> Source/WebCore/html/HTMLFrameElementBase.cpp:298
> +bool HTMLFrameElementBase::allowFullScreen() const
> +{
> +    return m_allowFullScreen;
> +}

Is it necessary to have the member variable, can't you do something like "return hasAttribute(webkitallowfullscreenAttr)" instead?
Comment 6 Jer Noble 2011-03-13 21:30:50 PDT
(In reply to comment #5)
> (From update of attachment 85611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85611&action=review
> 
> > LayoutTests/fullscreen/full-screen-iframe-allowed.html:22
> > +    runWithKeyDown(function() {
> > +
> > +        setTimeout(function() { 
> > +            consoleWrite("FAIL - did not enter full screen!"); 
> > +            endTest(); 
> > +        }, 50);
> > +
> > +        frame.contentDocument.documentElement.webkitRequestFullScreen();
> > +    });
> 
> This makes it look like the 'webkitfullscreenchange' event is posted synchronously, but the Mozilla spec says it should be asynch: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch this event." If the event is being posted aynchronously, using a timeout to detect success/failure will lead to problems as it is timing dependent.

This is one of the problems with the spec.  To determine whether entering full screen failed, one must wait indefinitely, as there is no error event.  Unfortunately, I don't see another way to determine the failure mode besides a timeout.

But, in the case of DumpRenderTree, we should have pretty dependable, reproducible behavior here.

> 
> > Source/WebCore/html/HTMLAttributeNames.in:302
> >  webkitdirectory
> > +webkitallowfullscreen
> 
> Should these be sorted?

Probably. :)

> > Source/WebCore/html/HTMLFrameElementBase.cpp:60
> > +#if ENABLE(FULLSCREEN_API)
> > +    , m_allowFullScreen(false)
> > +#endif
> 
> See below.
> 
> > Source/WebCore/html/HTMLFrameElementBase.cpp:154
> > +#if ENABLE(FULLSCREEN_API)
> > +    } else if (attr->name() == webkitallowfullscreenAttr) {
> > +        // Boolean argument; the attr is irrelevant.
> > +        m_allowFullScreen = true;
> > +#endif
> 
> See below.
> 
> > Source/WebCore/html/HTMLFrameElementBase.cpp:298
> > +bool HTMLFrameElementBase::allowFullScreen() const
> > +{
> > +    return m_allowFullScreen;
> > +}
> 
> Is it necessary to have the member variable, can't you do something like "return hasAttribute(webkitallowfullscreenAttr)" instead?

Sounds reasonable.  Changed.
Comment 7 Jer Noble 2011-03-13 21:52:29 PDT
Created attachment 85644 [details]
Patch
Comment 8 Eric Carlson 2011-03-13 23:07:22 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 85611 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=85611&action=review
> > 
> > > LayoutTests/fullscreen/full-screen-iframe-allowed.html:22
> > > +    runWithKeyDown(function() {
> > > +
> > > +        setTimeout(function() { 
> > > +            consoleWrite("FAIL - did not enter full screen!"); 
> > > +            endTest(); 
> > > +        }, 50);
> > > +
> > > +        frame.contentDocument.documentElement.webkitRequestFullScreen();
> > > +    });
> > 
> > This makes it look like the 'webkitfullscreenchange' event is posted synchronously, but the Mozilla spec says it should be asynch: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch this event." If the event is being posted aynchronously, using a timeout to detect success/failure will lead to problems as it is timing dependent.
> 
> This is one of the problems with the spec.  To determine whether entering full screen failed, one must wait indefinitely, as there is no error event.  Unfortunately, I don't see another way to determine the failure mode besides a timeout.
> 
I assume you have, or will, give this feedback to roc? ;-)
Comment 9 Jer Noble 2011-03-14 09:30:05 PDT
(In reply to comment #8)
> I assume you have, or will, give this feedback to roc? ;-)

Of course. :-D
Comment 10 Jer Noble 2011-03-14 10:53:34 PDT
Committed r81038: <http://trac.webkit.org/changeset/81038>