Bug 66531 - Don't detach elements from the render tree when entering fullscreen mode
Summary: Don't detach elements from the render tree when entering fullscreen mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Kozianski
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-18 20:43 PDT by Jeremy Apthorp
Modified: 2011-10-15 00:09 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.16 KB, patch)
2011-08-18 20:48 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (4.13 KB, patch)
2011-08-18 21:06 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (4.10 KB, patch)
2011-08-18 21:12 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (6.30 KB, patch)
2011-08-21 23:05 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (9.98 KB, patch)
2011-08-22 19:12 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (10.04 KB, patch)
2011-08-22 20:51 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (10.07 KB, patch)
2011-08-23 15:18 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (10.45 KB, patch)
2011-09-05 18:17 PDT, James Kozianski
no flags Details | Formatted Diff | Diff
Patch (9.84 KB, patch)
2011-09-16 14:18 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (9.92 KB, patch)
2011-09-16 14:42 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff
Patch (5.40 KB, patch)
2011-09-20 15:15 PDT, Jeremy Apthorp
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Apthorp 2011-08-18 20:43:22 PDT
Don't detach elements from the render tree when entering fullscreen mode
Comment 1 Jeremy Apthorp 2011-08-18 20:48:20 PDT
Created attachment 104448 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2011-08-18 20:58:35 PDT
Comment on attachment 104448 [details]
Patch

Attachment 104448 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9433030
Comment 3 Jeremy Apthorp 2011-08-18 21:06:12 PDT
Created attachment 104452 [details]
Patch
Comment 4 Gustavo Noronha (kov) 2011-08-18 21:09:31 PDT
Comment on attachment 104452 [details]
Patch

Attachment 104452 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9437044
Comment 5 Jeremy Apthorp 2011-08-18 21:12:28 PDT
Created attachment 104453 [details]
Patch
Comment 6 Jer Noble 2011-08-19 07:47:37 PDT
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.
Comment 7 Jeremy Apthorp 2011-08-21 23:05:40 PDT
Created attachment 104645 [details]
Patch
Comment 8 Jeremy Apthorp 2011-08-21 23:06:22 PDT
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.
Comment 9 Darin Adler 2011-08-22 09:36:41 PDT
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.
Comment 10 Jer Noble 2011-08-22 10:48:44 PDT
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.
Comment 11 Jeremy Apthorp 2011-08-22 19:12:59 PDT
Created attachment 104782 [details]
Patch
Comment 12 Jeremy Apthorp 2011-08-22 19:14:06 PDT
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.
Comment 13 Jeremy Apthorp 2011-08-22 20:51:05 PDT
Created attachment 104784 [details]
Patch
Comment 14 Jer Noble 2011-08-23 10:06:48 PDT
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.
Comment 15 Jeremy Apthorp 2011-08-23 15:18:07 PDT
Created attachment 104918 [details]
Patch
Comment 16 Jeremy Apthorp 2011-08-23 15:18:41 PDT
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.
Comment 17 Jeremy Apthorp 2011-09-02 16:11:44 PDT
Ping- what more needs to be done here?
Comment 18 Jer Noble 2011-09-03 13:28:01 PDT
(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 :-( )
Comment 19 Darin Fisher (:fishd, Google) 2011-09-04 20:46:06 PDT
Comment on attachment 104918 [details]
Patch

R=me based on Jer's unofficial review.
Comment 20 WebKit Review Bot 2011-09-04 21:45:29 PDT
Comment on attachment 104918 [details]
Patch

Clearing flags on attachment: 104918

Committed r94510: <http://trac.webkit.org/changeset/94510>
Comment 21 WebKit Review Bot 2011-09-04 21:45:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Abhishek Arya 2011-09-04 23:19:52 PDT
(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.
Comment 23 James Kozianski 2011-09-04 23:42:54 PDT
Reverted r94510 for reason:

Causes

Committed r94513: <http://trac.webkit.org/changeset/94513>
Comment 24 James Kozianski 2011-09-05 18:17:14 PDT
Created attachment 106371 [details]
Patch
Comment 25 James Kozianski 2011-09-05 18:27:23 PDT
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.
Comment 26 James Kozianski 2011-09-11 22:37:16 PDT
Ping. Is anybody able to take a look at this?
Comment 27 James Robinson 2011-09-13 22:16:12 PDT
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?
Comment 28 Jeremy Apthorp 2011-09-16 14:18:25 PDT
Created attachment 107720 [details]
Patch
Comment 29 Jeremy Apthorp 2011-09-16 14:19:24 PDT
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?
Comment 30 Jeremy Apthorp 2011-09-16 14:42:20 PDT
Created attachment 107725 [details]
Patch
Comment 31 James Robinson 2011-09-16 14:44:25 PDT
Comment on attachment 107725 [details]
Patch

R=me
Comment 32 WebKit Review Bot 2011-09-16 23:06:49 PDT
Comment on attachment 107725 [details]
Patch

Clearing flags on attachment: 107725

Committed r95371: <http://trac.webkit.org/changeset/95371>
Comment 33 WebKit Review Bot 2011-09-16 23:06:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Jeremy Apthorp 2011-09-20 15:15:46 PDT
Created attachment 108060 [details]
Patch
Comment 35 Jeremy Apthorp 2011-09-20 15:17:17 PDT
Using firstChild() is not correct in the case of inline elements containing block elements. since the inline elements get split into parts.
Comment 36 Darin Adler 2011-09-20 16:23:49 PDT
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?
Comment 37 Abhishek Arya 2011-09-20 16:28:45 PDT
(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.
Comment 38 Adam Barth 2011-10-15 00:09:03 PDT
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).