Bug 56264 – FullScreen: Handle entering full screen security restrictions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jer Noble]

FullScreen: Handle entering full screen security restrictions

[Jer Noble]

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".

[Jer Noble]

Created attachment 85610 [details]
Patch

[Jer Noble]

Somehow, I lost most of my comments in the WebCore/ChangeLog.  I'll update the file and repost the patch.

[Jer Noble]

Created attachment 85611 [details]
Patch

[Eric Carlson]

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?

[Jer Noble]

(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.

[Jer Noble]

Created attachment 85644 [details]
Patch

[Eric Carlson]

(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? ;-)

[Jer Noble]

(In reply to comment #8)
> I assume you have, or will, give this feedback to roc? ;-)

Of course. :-D

[Jer Noble]

Committed r81038: <http://trac.webkit.org/changeset/81038>