Bug 12373

Summary: ASSERTION FAILURE: !child->needsLayout() on paypal.com
Product: WebKit Reporter: Matt Lilek <dev+webkit>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, ddkilzer, mitz
Priority: P1 Keywords: HasReduction, InRadar, Regression
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://paypal.com
Attachments:
Description Flags
Crashlog
none
Test case (needs reduction)
none
Reduction (will trip the ASSERT)
none
Option A: Don't create renderers for forms in tables, table sections and table rows in HTML
none
Option B: Alternatively, change the assertion to ignore inlines. They're harmless! darin: review+

Description Matt Lilek 2007-01-22 21:32:41 PST
1. Login to paypal.com
2. In the recent activity list of the overview page, click the detail link
3. WebKit will crash with the following assertion failure in r19037:

ASSERTION FAILED: !child->needsLayout()
(/Users/matt/Code/WebKit/WebCore/rendering/RenderContainer.cpp:479 virtual void WebCore::RenderContainer::layout())
Comment 1 Matt Lilek 2007-01-22 21:34:42 PST
Created attachment 12617 [details]
Crashlog
Comment 2 Matt Lilek 2007-01-22 21:37:41 PST
If you don't have any items in recent activity, a detail page from My Account -> History triggers this too.
Comment 3 Matt Lilek 2007-01-22 21:57:23 PST
<rdar://problem/4947450>
Comment 4 David Kilzer (:ddkilzer) 2007-01-23 05:52:28 PST
Created attachment 12624 [details]
Test case (needs reduction)

Test case saved as "Web Page, Complete" from Firefox with transaction-specific info replaced with our friend, Lorem Ipsum.  Still needs a reduction, but now you don't need a PayPal account to reproduce it.
Comment 5 mitz 2007-01-23 08:03:45 PST
Created attachment 12626 [details]
Reduction (will trip the ASSERT)
Comment 6 Beth Dakin 2007-01-27 18:16:57 PST
I talked with Maciej a bit about this bug. We are asserting because child is a RenderInline and RenderInline::layout() does nothing. (It is worth noting that this does not crash without the assertion, not does it cause any layout glitches as far as I can tell.) After talking with Maciej, it seems like we are incorrectly constructing the RenderTree. It seems like there should be an anonymous block wrapping the RenderInline so that it would get updated appropriately if there was anything to update. If this isn't the case, then it seems like the assertion is just wrong. 
Comment 7 mitz 2007-01-27 22:35:52 PST
Firefox has a UA stylesheet rule that sets such forms to display:none. The children of the form element are pushed out anyway, so it shouldn't matter. I tried to override createRenderer() to return 0 if the form element has one of the forbidden parents (table, tbody, tfoot, thead, tr) but it broke some of the fast/dom tests where the form is created dynamically inside the table. Maybe other rules apply there.
Comment 8 mitz 2007-01-27 22:37:46 PST
How about having RenderInline::layout() just call setNeedsLayout(false)? The ASSERT in RenderContainer is new, but it is nice to have.
Comment 9 Maciej Stachowiak 2007-01-28 19:45:24 PST
1) Which tests break?

2) Would the UA rule work for us too?
Comment 10 mitz 2007-01-28 23:06:05 PST
(In reply to comment #9)
> 1) Which tests break?

Sorry, they were not fast/dom but rather
fast/table/form-in-tbody-before-misnested-text-crash.xhtml
fast/table/form-in-row-before-misnested-text-crash.xhtml
fast/table/form-in-table-before-misnested-text-crash.xhtml

These are all XHTML, and I remember that the Firefox UA rule uses a "-moz-is-html" pseudoclass, so maybe I should have done something similar in my createRenderer() override.

> 2) Would the UA rule work for us too?

Here's the Firefox rule from its html.css:

tr > form:-moz-is-html, tbody > form:-moz-is-html,
thead > form:-moz-is-html, tfoot > form:-moz-is-html,
table > form:-moz-is-html {
  /* Important: don't show these forms in HTML */
  display: none !important;
}

I don't know if there's a WebKit equivalent of -moz-is-html.
Comment 11 mitz 2007-01-29 14:22:55 PST
Created attachment 12759 [details]
Option A: Don't create renderers for forms in tables, table sections and table rows in HTML

Wait, there's another patch!
Comment 12 mitz 2007-01-29 14:24:11 PST
Created attachment 12760 [details]
Option B: Alternatively, change the assertion to ignore inlines. They're harmless!

Also fixes the problem.
Comment 13 Darin Adler 2007-01-29 17:19:21 PST
Comment on attachment 12759 [details]
Option A: Don't create renderers for forms in tables, table sections and table rows in HTML

I think this change will break any use of a counter CSS rule on one of these "form elements inside tables", because counters are based on the render tree hierarchy.

I'm not sure if that's an important issue, though.
Comment 14 Darin Adler 2007-01-30 14:04:52 PST
Comment on attachment 12760 [details]
Option B: Alternatively, change the assertion to ignore inlines. They're harmless!

Hyatt told me this change is a good idea. So r=me.
Comment 15 Sam Weinig 2007-01-30 14:56:20 PST
Landed in r19270.