Bug 73972 - Tighten our checks for needsSectionRecalc in RenderTable
Summary: Tighten our checks for needsSectionRecalc in RenderTable
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on: 75379
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-06 18:44 PST by Julien Chaffraix
Modified: 2022-08-15 15:26 PDT (History)
12 users (show)

See Also:


Attachments
Proposed change 1: Add some ASSERT to the sections getters, make us use the getters more and rename some field for coherency. (10.94 KB, patch)
2011-12-06 19:12 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-12-06 18:44:45 PST
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!
Comment 1 Julien Chaffraix 2011-12-06 19:12:08 PST
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.
Comment 2 Julien Chaffraix 2011-12-28 06:58:57 PST
Reviewer ping! This is an important - mechanical - change that wil help us detect more badness that could happen in the table rendering code.
Comment 3 Adam Barth 2011-12-28 09:48:33 PST
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.
Comment 4 WebKit Review Bot 2011-12-29 03:21:37 PST
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>
Comment 5 WebKit Review Bot 2011-12-29 03:21:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Csaba Osztrogonác 2011-12-29 13:43:55 PST
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?
Comment 7 Julien Chaffraix 2011-12-30 01:05:52 PST
(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.
Comment 8 Julien Chaffraix 2011-12-30 01:24:41 PST
Rolled the change in http://trac.webkit.org/changeset/103838
Comment 9 Ahmad Saleem 2022-08-15 15:26:12 PDT
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!