Don't detach elements from the render tree when entering fullscreen mode
Created attachment 104448 [details] Patch
Comment on attachment 104448 [details] Patch Attachment 104448 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9433030
Created attachment 104452 [details] Patch
Comment on attachment 104452 [details] Patch Attachment 104452 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9437044
Created attachment 104453 [details] Patch
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.
Created attachment 104645 [details] Patch
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 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 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.
Created attachment 104782 [details] Patch
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.
Created attachment 104784 [details] Patch
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.
Created attachment 104918 [details] Patch
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.
Ping- what more needs to be done here?
(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 on attachment 104918 [details] Patch R=me based on Jer's unofficial review.
Comment on attachment 104918 [details] Patch Clearing flags on attachment: 104918 Committed r94510: <http://trac.webkit.org/changeset/94510>
All reviewed patches have been landed. Closing bug.
(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.
Reverted r94510 for reason: Causes Committed r94513: <http://trac.webkit.org/changeset/94513>
Created attachment 106371 [details] Patch
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.
Ping. Is anybody able to take a look at this?
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?
Created attachment 107720 [details] Patch
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?
Created attachment 107725 [details] Patch
Comment on attachment 107725 [details] Patch R=me
Comment on attachment 107725 [details] Patch Clearing flags on attachment: 107725 Committed r95371: <http://trac.webkit.org/changeset/95371>
Created attachment 108060 [details] Patch
Using firstChild() is not correct in the case of inline elements containing block elements. since the inline elements get split into parts.
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?
(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 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).