Bug 76154 - A Frame with frame flattening can be stuck in a state in which performPostLayoutTasks() is never executed
Summary: A Frame with frame flattening can be stuck in a state in which performPostLay...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-01-12 00:23 PST by Benjamin Poulain
Modified: 2012-01-12 16:19 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.26 KB, patch)
2012-01-12 00:45 PST, Benjamin Poulain
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-01-12 00:23:39 PST
For <rdar://problem/10363387>.

With Frame Flattening, performPostLayoutTasks() is always executed asynchronously.

If FrameView::unscheduleRelayout() is executed when a pending post layout task timer exists, performPostLayoutTasks() will never be executed for this frame.

Here is the sequence of events
1) there is a layout, and since inSubframeLayoutWithFrameFlattening == true, m_hasPendingPostLayoutTasks becomes true and a post layout is scheduled
2) there is a FrameView::unscheduleRelayout(), which kill the timer, and leave m_hasPendingPostLayoutTasks == true
so m_hasPendingPostLayoutTasks == true, and in the common case that would get processed in the next layout() but here we only execute the post layout on timer
3) all the next layouts skip the postLayout() tasks because m_hasPendingPostLayoutTasks == true
Comment 1 Benjamin Poulain 2012-01-12 00:45:32 PST
Created attachment 122183 [details]
Patch
Comment 2 Benjamin Poulain 2012-01-12 00:48:57 PST
I have trouble making a test for this. I do not have yet a reliable way to trigger FrameView::unscheduleRelayout() after a layout() but before the timer.

I would appreciate if you could already check if this looks correct.
Comment 3 Simon Fraser (smfr) 2012-01-12 09:59:31 PST
Comment on attachment 122183 [details]
Patch

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

I think Beth should look at this.

> Source/WebCore/ChangeLog:14
> +        This patch revert the handling of the postLayoutTasks to its state prior to r66552.

Presumably without regressing the bug that r66552 fixed? Can you say more here about how you managed that?
Comment 4 Benjamin Poulain 2012-01-12 11:42:38 PST
> > Source/WebCore/ChangeLog:14
> > +        This patch revert the handling of the postLayoutTasks to its state prior to r66552.
> 
> Presumably without regressing the bug that r66552 fixed? Can you say more here about how you managed that?

This do not revert r66552, just revert a detail of the patch: the change from the timer to a bool to manage the state.
Comment 5 Benjamin Poulain 2012-01-12 11:48:29 PST
Some more context: r66552 changed the postLayoutTasks from the timer to a bool, and stopped the timer on unschedudeRelayout. That change makes sense.

But then, r77988 change the way the postLayoutTasks are performed, and that is when everything went to hell.
Comment 6 Antti Koivisto 2012-01-12 13:00:16 PST
Comment on attachment 122183 [details]
Patch

Looks good to me and reducing complexity here a bit is nice (assuming the earlier problematic cases are still handled somehow).
Comment 7 Beth Dakin 2012-01-12 14:24:17 PST
Comment on attachment 122183 [details]
Patch

Well if Antti's on board, so am I ;-)

Looks good. And since I desperately can't remember why I added m_hasPendingPostLayoutTasks, I can't really object to its removal. I still think you should test the original reproducibility steps of that bug that I was fixing when I added it, but r=me.
Comment 8 Benjamin Poulain 2012-01-12 16:19:39 PST
Committed r104874: <http://trac.webkit.org/changeset/104874>