Bug 12373 – ASSERTION FAILURE: !child->needsLayout() on paypal.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matt Lilek]

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())

[Matt Lilek]

Created attachment 12617 [details]
Crashlog

[Matt Lilek]

If you don't have any items in recent activity, a detail page from My Account -> History triggers this too.

[Matt Lilek]

<rdar://problem/4947450>

[David Kilzer (:ddkilzer)]

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.

Created attachment 12626 [details]
Reduction (will trip the ASSERT)

[Beth Dakin]

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. 

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.

How about having RenderInline::layout() just call setNeedsLayout(false)? The ASSERT in RenderContainer is new, but it is nice to have.

[Maciej Stachowiak]

1) Which tests break?

2) Would the UA rule work for us too?

(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.

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!

Created attachment 12760 [details]
Option B: Alternatively, change the assertion to ignore inlines. They're harmless!

Also fixes the problem.

[Darin Adler]

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.

[Darin Adler]

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.

[Sam Weinig]

Landed in r19270.