Bug 11672 - REGRESSION (r17068): Repro crash due to painting without layout
Summary: REGRESSION (r17068): Repro crash due to painting without layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://www.mapa.co.il/
Keywords: HasReduction, Regression
Depends on:
Blocks:
 
Reported: 2006-11-21 14:41 PST by mitz
Modified: 2006-12-02 06:14 PST (History)
2 users (show)

See Also:


Attachments
Reduction (crashes when Test is clicked) (514 bytes, text/html)
2006-11-22 05:57 PST, mitz
no flags Details
Change containing block logic in setStyle() to match containingBlock() (34.77 KB, patch)
2006-11-22 07:12 PST, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-11-21 14:41:49 PST
The URL crashes WebKit or causes an assertion failure as it tries to paint a table that is still marked as needing layout. Breaking in gdb reveals that the entire render tree is marked for layout except for the root RenderView (which is why the draw message from AppKit doesn't trigger layout; this logic in -[WebCoreFrameBridge needsLayout] seems to be wrong anyway since there can be a pending subtree relayout, but I don't think this is the case here).
Comment 1 mitz 2006-11-22 00:41:34 PST
RenderBlock::layoutPositionedObjects() calls setChildNeedsLayout() on the positioned objects without passing false for the markParents flag. This is wrong and it leads to the block (in this case, the RenderView) having its child re-marked as needing layout after the block finished laying out its children.

While it's nice that most objects other than tables manage to paint while needing layout, it's a condition that shouldn't occur. Perhaps this can be asserted in some of the more popular paint() methods.

I wonder if it's possible to make a reduction that will crash builds from before r17068. I think it should be possible.
Comment 2 mitz 2006-11-22 05:57:10 PST
Created attachment 11605 [details]
Reduction (crashes when Test is clicked)

Here's the connection to r17068: RenderObject::setStyle() takes care of reassigning positioned objects to their containing block when a container's position property changes. It has logic to determine the current containing block for it absolutely positioned children, which needs to be in sync with the corresponding case in containingBlock(). r17068 changed containingBlock() and container() by eliminating the !isRoot() check, but left it in setStyle():
                    while (cb && (cb->style()->position() == StaticPosition || (cb->isInline() && !cb->isReplaced())) && !cb->isRoot() && !cb->isRenderView()) {

Having said that, the two other issues (marking parents when you shouldn't and -[WebCoreFrameBridge needsLayout] disregarding subtree layout) are still valid in my opinion.
Comment 3 mitz 2006-11-22 06:26:34 PST
(In reply to comment #2)
> the two other issues [...] are still valid
> in my opinion.

Actually, marking parents shouldn't be a problem since there shouldn't be intermediate containers between the caller and the positioned object. It's still a waste of time to mark parents.
Comment 4 mitz 2006-11-22 07:12:16 PST
Created attachment 11607 [details]
Change containing block logic in setStyle() to match containingBlock()

No test regressions.
Comment 5 Dave Hyatt 2006-11-25 03:12:02 PST
Comment on attachment 11607 [details]
Change containing block logic in setStyle() to match containingBlock()

r=me
Comment 6 Alexey Proskuryakov 2006-12-02 06:14:37 PST
Committed revision 17986.