Bug 73972 – Tighten our checks for needsSectionRecalc in RenderTable

• Component: Tables
• Status: REOPENED

[Julien Chaffraix]

While investigating a bug, I discovered that we really don't implement much checks for needsSectionRecalc() inside RenderTable and could happily query a dirtied section pointer without knowing it.

Let's add more checks to catch such bugs earlier rather than later!

[Julien Chaffraix]

Created attachment 118160 [details]
Proposed change 1: Add some ASSERT to the sections getters, make us use the getters more and rename some field for coherency.

[Julien Chaffraix]

Reviewer ping! This is an important - mechanical - change that wil help us detect more badness that could happen in the table rendering code.

[Adam Barth]

Comment on attachment 118160 [details]
Proposed change 1: Add some ASSERT to the sections getters, make us use the getters more and rename some field for coherency.

Normally I don't review patches in Rendering, but this one looks fine to me.

[WebKit Review Bot]

Comment on attachment 118160 [details]
Proposed change 1: Add some ASSERT to the sections getters, make us use the getters more and rename some field for coherency.

Clearing flags on attachment: 118160

Committed r103794: <http://trac.webkit.org/changeset/103794>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It made the following tests assert on Qt in debug mode:
tables/mozilla/marvin/backgr_simple-table.html
tables/mozilla/marvin/backgr_position-table.html
tables/mozilla_expected_failures/marvin/backgr_border-table.html

ASSERTION FAILED: !needsSectionRecalc()
../../../../Source/WebCore/rendering/RenderTable.h(139) : WebCore::RenderTableSection* WebCore::RenderTable::header() const

Could you check it?

[Julien Chaffraix]

(In reply to comment #6)
> It made the following tests assert on Qt in debug mode:
> tables/mozilla/marvin/backgr_simple-table.html
> tables/mozilla/marvin/backgr_position-table.html
> tables/mozilla_expected_failures/marvin/backgr_border-table.html
> 
> ASSERTION FAILED: !needsSectionRecalc()
> ../../../../Source/WebCore/rendering/RenderTable.h(139) : WebCore::RenderTableSection* WebCore::RenderTable::header() const

I can reproduce it on Mac ToT. Strangely DRT does not crash but Safari does :-(

I will roll out the change to give me enough time to investigate what is wrong here. Sorry for the redness.

[Julien Chaffraix]

Rolled the change in http://trac.webkit.org/changeset/103838

[Ahmad Saleem]

I can see this bug patch was pushed in following commit:

https://github.com/WebKit/WebKit/commit/25f6e886e2abb48a556a2ded24bd668dd1035c00

But it got rolled back as Comment 08 mentioned.

Is this required now? I can see in Webkit Github source that it is still referring as m_head rather than m_header, which this patch was trying to do (renaming etc.):

https://github.com/WebKit/WebKit/blob/96f929df42d353179530e799be3b53eee9424a70/Source/WebCore/rendering/RenderTable.cpp

https://github.com/WebKit/WebKit/blob/96f929df42d353179530e799be3b53eee9424a70/Source/WebCore/rendering/RenderTable.h

Appreciate if someone can have a look and decide accordingly. Thanks!