Bug 66531 – Don't detach elements from the render tree when entering fullscreen mode

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Apthorp]

Don't detach elements from the render tree when entering fullscreen mode

[Jeremy Apthorp]

Created attachment 104448 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 104448 [details]
Patch

Attachment 104448 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9433030

[Jeremy Apthorp]

Created attachment 104452 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 104452 [details]
Patch

Attachment 104452 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9437044

[Jeremy Apthorp]

Created attachment 104453 [details]
Patch

[Jer Noble]

Comment on attachment 104453 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4820
> +    if (m_fullScreenElement != documentElement()) {
> +        RenderFullScreen* fullscreenRenderer = new (renderArena()) RenderFullScreen(this);
> +        fullscreenRenderer->setStyle(RenderFullScreen::createFullScreenStyle());
> +        fullscreenRenderer->wrapRenderer(renderer);
> +        setFullScreenRenderer(fullscreenRenderer);
> +    }

This is mostly duplicating existing code in NodeRenderingContext.cpp: wrapWithRenderFullScreen(...).

> Source/WebCore/rendering/RenderFullScreen.cpp:114
> +void RenderFullScreen::wrapRenderer(RenderObject* renderer)
> +{
> +    RenderObject* parent = renderer->parent();
> +    parent->addChild(this, renderer);
> +    renderer->remove();
> +    addChild(renderer);
> +}

As is this.  Perhaps that function could be promoted to a public class method or moved into RenderFullScreen.

[Jeremy Apthorp]

Created attachment 104645 [details]
Patch

[Jeremy Apthorp]

Comment on attachment 104453 [details]
Patch

I've refactored the wrap/unwrapRenderer functions. wrapRenderer is now a static method that creates and styles the RenderFullScreen object, and unwrapRenderer removes the wrappings.  RFS::createFullScreenStyle was only being called from RenderFullScreen.cpp, so I've made that method private.

[Darin Adler]

Comment on attachment 104645 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        This prevents plugin instances from being destroyed and reinstantiated
> +        when entering fullscreen mode.

Then there should be a test demonstrating this.

[Jer Noble]

Comment on attachment 104645 [details]
Patch

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

> Source/WebCore/rendering/RenderFullScreen.cpp:114
> +        RenderObject* parent = object->parent();
> +        if (parent) {

I think this can be written as:

if (RenderObject* parent = object->parent()) {

> Source/WebCore/rendering/RenderFullScreen.h:50
> +    RenderObject* m_wrappedRenderer;

The wrapped renderer will always be the first and only child of the RenderFullScreen, so this extra ivar is probably unnecessary.  All the instances above which reference m_wrappedRenderer can be replaced with firstChild().

> Source/WebCore/rendering/RenderFullScreen.h:51
> +    static PassRefPtr<RenderStyle> createFullScreenStyle();

If this is only ever accessed from RenderFullScreen.cpp now, it can be made a file static.

[Jeremy Apthorp]

Created attachment 104782 [details]
Patch

[Jeremy Apthorp]

Comment on attachment 104645 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +        when entering fullscreen mode.
> 
> Then there should be a test demonstrating this.

Done.

>> Source/WebCore/rendering/RenderFullScreen.cpp:114
>> +        if (parent) {
> 
> I think this can be written as:
> 
> if (RenderObject* parent = object->parent()) {

Done.

>> Source/WebCore/rendering/RenderFullScreen.h:50
>> +    RenderObject* m_wrappedRenderer;
> 
> The wrapped renderer will always be the first and only child of the RenderFullScreen, so this extra ivar is probably unnecessary.  All the instances above which reference m_wrappedRenderer can be replaced with firstChild().

Done.

>> Source/WebCore/rendering/RenderFullScreen.h:51
>> +    static PassRefPtr<RenderStyle> createFullScreenStyle();
> 
> If this is only ever accessed from RenderFullScreen.cpp now, it can be made a file static.

Done.

[Jeremy Apthorp]

Created attachment 104784 [details]
Patch

[Jer Noble]

Comment on attachment 104784 [details]
Patch

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

> Source/WebCore/rendering/RenderFullScreen.cpp:101
> -    fullscreenStyle->setLeft(Length(0, Fixed));
> -    fullscreenStyle->setTop(Length(0, Fixed));
> +    fullscreenStyle->setLeft(Length(0, WebCore::Fixed));
> +    fullscreenStyle->setTop(Length(0, WebCore::Fixed));

I'm curious why this is necessary, given the "using namespace WebCore;" directive above.

> Source/WebCore/rendering/RenderFullScreen.cpp:133
> +    if (wrappedRenderer)
> +        wrappedRenderer->remove();
> +    RenderObject* holder = placeholder() ? placeholder() : this;
> +    RenderObject* parent = holder->parent();
> +    if (parent)
> +        parent->addChild(wrappedRenderer, holder);

You null-check wrappedRenderer before calling remove(), but not before passing wrappedRenderer to parent->addChild().  It appears it's not safe to pass a null value into that function (see RenderObject.cpp), so the second if statement should check wrappedRenderer as well.

[Jeremy Apthorp]

Created attachment 104918 [details]
Patch

[Jeremy Apthorp]

Comment on attachment 104784 [details]
Patch

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

>> Source/WebCore/rendering/RenderFullScreen.cpp:101
>> +    fullscreenStyle->setTop(Length(0, WebCore::Fixed));
> 
> I'm curious why this is necessary, given the "using namespace WebCore;" directive above.

It's ambiguous with 'typedef SInt32 Fixed' in a system header (MacTypes.h) if I don't include the namespace resolution.

>> Source/WebCore/rendering/RenderFullScreen.cpp:133
>> +        parent->addChild(wrappedRenderer, holder);
> 
> You null-check wrappedRenderer before calling remove(), but not before passing wrappedRenderer to parent->addChild().  It appears it's not safe to pass a null value into that function (see RenderObject.cpp), so the second if statement should check wrappedRenderer as well.

Done.

[Jeremy Apthorp]

Ping- what more needs to be done here?

[Jer Noble]

(In reply to comment #17)
> Ping- what more needs to be done here?

Nothing; looks good to me!  (I can't r+ your patch though; sorry :-( )

[Darin Fisher (:fishd, Google)]

Comment on attachment 104918 [details]
Patch

R=me based on Jer's unofficial review.

[WebKit Review Bot]

Comment on attachment 104918 [details]
Patch

Clearing flags on attachment: 104918

Committed r94510: <http://trac.webkit.org/changeset/94510>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Abhishek Arya]

(In reply to comment #21)
> All reviewed patches have been landed.  Closing bug.

Tests that crashed:
+fullscreen/full-screen-twice.html	crash log
+fullscreen/full-screen-css.html	crash log
+fullscreen/full-screen-keyboard-enabled.html	crash log
+fullscreen/full-screen-zIndex-after.html	crash log
+fullscreen/full-screen-placeholder.html	crash log
+fullscreen/full-screen-keyboard-disabled.html	crash log

6 new crashes starting this commit, can you please fix/revert.

[James Kozianski]

Reverted r94510 for reason:

Causes

Committed r94513: <http://trac.webkit.org/changeset/94513>

[James Kozianski]

Created attachment 106371 [details]
Patch

[James Kozianski]

The previous patch crashed when we attempted to enter fullscreen mode when we were already in fullscreen mode. This patch avoids that crash by removing the old fullscreen renderer from the render tree before amending the new one.

The new lines are at the start of Document::webkitWillEnterFullScreenForElement(), namely:

if (m_fullScreenRenderer)
    m_fullScreenRenderer->unwrapRenderer();


Another way to solve this problem is to exit fullscreen mode completely when a page enters fullscreen again, which would be more work, but should leave less room for bugs like this.

[James Kozianski]

Ping. Is anybody able to take a look at this?

[James Robinson]

Comment on attachment 106371 [details]
Patch

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

The C++ changes all look good, but I have some concerns about how the test is working.

> LayoutTests/plugins/fullscreen-plugins-dont-reload-expected.txt:4
> +ALERT: Plugin Loaded!
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600

I can't really tell what this test output is trying to tell me.  What happens if the test fails?  Is the render tree part of the expected test output?

Are you trying to tell if the plugin is loading once or twice?  If so then this should be dumpAsText() and probably have some sort of text in the output indicating what is supposed to be there.

> LayoutTests/plugins/fullscreen-plugins-dont-reload.html:21
> +    }, 100);

why setTimeout(..., 100)? that seems like a recipe for flake - is there something in particular you are waiting for?

[Jeremy Apthorp]

Created attachment 107720 [details]
Patch

[Jeremy Apthorp]

James- I've updated the test so it uses the events instead of timeouts, and updated the expected text to reflect what is actually expected. Could you take another look?

[Jeremy Apthorp]

Created attachment 107725 [details]
Patch

[James Robinson]

Comment on attachment 107725 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 107725 [details]
Patch

Clearing flags on attachment: 107725

Committed r95371: <http://trac.webkit.org/changeset/95371>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jeremy Apthorp]

Created attachment 108060 [details]
Patch

[Jeremy Apthorp]

Using firstChild() is not correct in the case of inline elements containing block elements. since the inline elements get split into parts.

[Darin Adler]

Comment on attachment 108060 [details]
Patch

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

Why no test case?

> Source/WebCore/rendering/RenderFullScreen.cpp:128
> -    RenderObject* wrappedRenderer = firstChild();
> +    RenderObject* wrappedRenderer = m_wrappedRenderer;

What guarantees this renderer is has not been deallocated? Maybe its style was changed to display: none?

> Source/WebCore/rendering/RenderFullScreen.cpp:137
> +    document()->recalcStyle(Node::Force);

This needs a comment. It’s unclear why it’s helpful. The comment in the change log saying “do this here in order to avoid destroying elements that haven't yet followed the wrapped element out of the RenderFullScreen” doesn’t make it clear to me what the issue is. Maybe there’s a clearer way to explain it?

[Abhishek Arya]

(In reply to comment #35)
> Using firstChild() is not correct in the case of inline elements containing block elements. since the inline elements get split into parts.

We should have filed this as a separate security bug with credit: and crbug link. That makes it easy to track for all webkit consumers. Please do attach the testcase as well.

[Adam Barth]

Comment on attachment 108060 [details]
Patch

Cleared review? from attachment 108060 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).