Bug 52095 – REGRESSION (r75277): 2 test cases (<1%) had incorrect layout after fullscreen changes

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

[Jer Noble]

Changes to full screen mode caused two test cases to start generating extra test.  The tests technically pass, but the javascript source code contained is being dumped by DRT.

[Jer Noble]

Failing tests:

fullscreen/full-screen-remove-ancestor.html
fullscreen/full-screen-remove.html

[Jer Noble]

Created attachment 78295 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 78295 [details]
Patch

Rejecting attachment 78295 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
......
fast/xmlhttprequest ................
fast/xpath .....................................
fast/xpath/4XPath/Borrowed ......
fast/xpath/4XPath/Core ..........
fast/xpath/py-dom-xpath ........
fast/xsl .......................................
fonts ......
fullscreen ...
fullscreen/full-screen-remove-ancestor.html -> crashed

Exiting early after 1 failures. 16612 tests run.
325.49s total testing time

16611 test cases (99%) succeeded
1 test case (<1%) crashed
8 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7396070

[Jer Noble]

Created attachment 78344 [details]
Patch

[Eric Seidel (no email)]

Thanks for fixing.  Unfortunately I don't really understand the patch, so best to leave this for someone else to review. :)

[Jer Noble]

Comment on attachment 78344 [details]
Patch

Updated patch to fix the crash while running the full-screen-remove-ancestor test.

[Simon Fraser (smfr)]

Comment on attachment 78344 [details]
Patch

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

r=me, bonus points if you explain why the patch fixes <script> being dumped into the output, in the changelog.

> Source/WebCore/ChangeLog:13
> +        In nodeWillBeRemoved(), match the code in webkitDidExitFullScreen().
> +        Don't detach the documentElement, but rather simply call recalcStyle(Force).

Don't you mean "don't detach the fullscreen element"?

[WebKit Commit Bot]

Comment on attachment 78344 [details]
Patch

Rejecting attachment 78344 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 78344]" exit_code: 1
Last 500 characters of output:
st/html/script-tests/disable-style-element.js
	A	LayoutTests/fast/html/disable-style-element-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/WebCore/dom/StyleElement.h
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/html/HTMLStyleElement.h
	M	Source/WebCore/html/HTMLStyleElement.idl
	M	Source/WebCore/html/HTMLStyleElement.cpp
r75352 = 5b0a15fef122c3d163eb2e34b13253e38202ffea (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/7400083

[Eric Seidel (no email)]

Ah. :)  Yes.  Setting cq+ will land the patch regardless.  We should probably teach it to not try to land patches which are still marked r?:

From the output:
NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.

[Eric Seidel (no email)]

Bug 36638 is about this strange behavior.  I'll look at making a patch.  Sorry for the trouble.

[Maciej Stachowiak]

Comment on attachment 78344 [details]
Patch

I don't think this fix is right. I think the right change is to make RenderFullScreen properly anonymous. That will remove the risk of the document's renderer being destroyed, which is ultimately what goes wrong here. Like so:

Index: rendering/RenderFullScreen.h
===================================================================
--- rendering/RenderFullScreen.h	(revision 75328)
+++ rendering/RenderFullScreen.h	(working copy)
@@ -33,7 +33,7 @@
 
 class RenderFullScreen : public RenderFlexibleBox {
 public:
-    RenderFullScreen(Node* node) : RenderFlexibleBox(node) { setReplaced(false); setIsAnonymous(false); }
+    RenderFullScreen(Node* node) : RenderFlexibleBox(node) { setReplaced(false); }
     virtual bool isRenderFullScreen() const { return true; }
     virtual const char* renderName() const { return "RenderFullScreen"; }


Is there a reason RenderFullScreen is non-anonymous?

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 78344 [details])
>
> Is there a reason RenderFullScreen is non-anonymous?

Yes, I found that, if RenderFullScreen was anonymous and the parent element of the full screen element also had an anonymous renderer, the layout system would collapse the two anonymous blocks and remove the RenderFullScreen renderer from the render tree.

[Jer Noble]

(continuing...)

I believe the function which pruned out the anonymous RenderFullScreen was void RenderBlock::removeLeftoverAnonymousBlock(RenderBlock* child).

[Jer Noble]

(continuing...)

However, it looks like that was before RenderFullScreen became a flex-box; the code which calls removeLeftoverAnonymousBlock will first check that the anonymous block's display style is BLOCK, and the RenderFullScreen will have a display style of BOX; the collapsing behavior described above may no longer be an issue.

[Jer Noble]

(In reply to comment #14)
> (continuing...)
> 
> However, it looks like that was before RenderFullScreen became a flex-box; the code which calls removeLeftoverAnonymousBlock will first check that the anonymous block's display style is BLOCK, and the RenderFullScreen will have a display style of BOX; the collapsing behavior described above may no longer be an issue.

After making the change Maciej suggested, it appears the RenderFullScreen render is still being pruned in the majority of my test cases, which breaks the animation aspect of the full screen feature.

[Jer Noble]

(In reply to comment #7)
> (From update of attachment 78344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78344&action=review
> 
> r=me, bonus points if you explain why the patch fixes <script> being dumped into the output, in the changelog.

I haven't debugged into DRT yet, but I believe the render tree was being left in an inconsistent state when an ancestor was removed.

> > Source/WebCore/ChangeLog:13
> > +        In nodeWillBeRemoved(), match the code in webkitDidExitFullScreen().
> > +        Don't detach the documentElement, but rather simply call recalcStyle(Force).
> 
> Don't you mean "don't detach the fullscreen element"?

In this case, the full screen element is the documentElement.  When an ancestor of the current full screen element is removed, the documentElement becomes the new full screen element.

[Jer Noble]

(In reply to comment #15)
> After making the change Maciej suggested, it appears the RenderFullScreen render is still being pruned in the majority of my test cases, which breaks the animation aspect of the full screen feature.

Upon further testing, it appears the renderer isn't being pruned, but rather the RenderLayer tree will not use hardware-compositing for an anonymous renderer.

[Jer Noble]

Created attachment 78446 [details]
Patch

[Jer Noble]

(In reply to comment #18)
> Created an attachment (id=78446) [details]
> Patch

Thanks for the r+ Simon; but I wanted to address Maciej's comment about anonymity.  This new patch no longer marks RenderFullScreen as a non-anonymous renderer.  The lack of accelerated compositing on the RenderFullScreen object has been fixed in this patch as well.

[WebKit Commit Bot]

Comment on attachment 78446 [details]
Patch

Clearing flags on attachment: 78446

Committed r75450: <http://trac.webkit.org/changeset/75450>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.