Closed Bug 337124 Opened 15 years ago Closed 14 years ago

Crash [@ nsTableRowGroupFrame::GetNumLines] with table-row-group/table-column-group [@ nsTableRowGroupFrame::IncrementalReflow]

Categories

(Core :: Layout: Tables, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: bernd_mozilla)

References

Details

(5 keywords)

Crash Data

Attachments

(6 files, 1 obsolete file)

See upcoming testcase, which crashes on load.
This also crashes Firefox1.5.0.3.
Doesn't happen in 2006-01-18 build, happens in 2006-01-21 build.
Very likely this regressed with the fix from bug 317275, since the patch in that bug was also checked into the branch.

Talkback ID: TB18433284Z
nsTableRowGroupFrame::IncrementalReflow   TableBackgroundPainter::PaintTable   nsTableFrame::PaintTableBorderBackground
Attached file testcase
Attached file original mangled file
Hmm, problem is that my original mangled file also crashes in older builds, like 2005-09-01 build or Mozilla1.7, for instance.
I tried to get a stack with the 2005-09-1, but I only get garbage with Talkback.
>but I only get garbage with Talkback.
I would trust a trace from your debug build ;-) .
cgFrame = NS_STATIC_CAST(nsTableColGroupFrame*, cgFrame->GetNextSibling()
returns some garbage if a rowgroup is a sibling of a colgroup and on the secondary childlist....
Attached file backtrace
This is a backtrace from a debug build with the original mangled file.
For some reason, I don't get a crash with the testcase in my debug build.
I do get a crash, no need to waste time on it further.
Assignee: nobody → bernd_mozilla
removing the style attribute in the first testcase from <fieldset style="display: table-column-group;"> or replacing the fieldset with a div makes the thing not crash. Fieldset is mentioned in IsSpecialContent.

Now comes the quick question which function does not honour IsSpecialContent and uses display based frame creation where tag based should be used.... everybodies darling... nsCSSFrameConstructor::TableProcessChild, somewhere I have a r-'ed patch for this.
 

somewhere is bug 243159
Flags: blocking1.9a2?
I used this bug as a testcase to verify bug 350212, and the stack trace on Windows was garbled just like Martijn's, but the stacks on Mac and Linux had this at the top:

    	 nsTableRowGroupFrame::GetNumLines()   	[/builds/tinderbox/Fx-Mozilla1.8-release/Linux_2.4.21-27.0.4.EL_Depend/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1856]
    	TableBackgroundPainter::PaintTable() 	[/builds/tinderbox/Fx-Mozilla1.8-release/Linux_2.4.21-27.0.4.EL_Depend/mozilla/layout/tables/nsTablePainter.cpp, line 389]
    	nsTableFrame::Paint() 	[/builds/tinderbox/Fx-Mozilla1.8-release/Linux_2.4.21-27.0.4.EL_Depend/mozilla/layout/tables/nsTableFrame.cpp, line 1466]
    	nsContainerFrame::PaintChild() 	[/builds/tinderbox/Fx-Mozilla1.8-release/Linux_2.4.21-27.0.4.EL_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 329]
    	nsTableOuterFrame::Paint() 	[/builds/tinderbox/Fx-Mozilla1.8-release/Linux_2.4.21-27.0.4.EL_Depend/mozilla/layout/tables/nsTableOuterFrame.cpp, line 693]

...

See TB22514961 (Windows), TB22515374 (Linux), TB22515426 (Mac).
Summary: Crash [@ nsTableRowGroupFrame::IncrementalReflow] with table-row-group/table-column-group → Crash [@ nsTableRowGroupFrame::GetNumLines] with table-row-group/table-column-group [@ nsTableRowGroupFrame::IncrementalReflow]
comment 7 and 8 are bogus. I wrote patch for bug 243159 which removes TableProcessChild but the crash still remains. Its my favorite function (see
https://bugzilla.mozilla.org/show_bug.cgi?id=341858#c21 ) IsValidSibling that returns a bogus sibling. 

 <span style="display: table-row-group;"><input type="text"></span>
  <div style="display: table-column-group;"><script>document.body.offsetHeight;</script></div>
  <span style="display: table-row-group;"><input type="text"></span>
  <fieldset style="display: table-column-group;"></fieldset>

We iterate starting from the second div back toward the start inside FindPreviousSibling , to find that the div is a good sibling for the fieldset ... overwrite the the correct parent frame (the fieldsetframe) with the parent of the div and create a completely bogus frame structure.
Attached patch patchSplinter Review
Flags: blocking1.9a2? → blocking1.9+
Comment on attachment 241200 [details] [diff] [review]
patch

This is in depth defense. The core problem is that rowgroups and colgroups are siblings content wise but the colgroups live on a secondary childlist in the frame tree. So we need to handle the case when the two things that are not really siblings are expected to be sibling from the frame constructor side. See the above SetInitialChildlist how we do this initially.
Attachment #241200 - Flags: superreview?(roc)
Attachment #241200 - Flags: review?(roc)
+    const nsStyleDisplay* prevDisplay = aPrevFrame->GetStyleDisplay();
+    if (NS_STYLE_DISPLAY_TABLE_COLUMN_GROUP == prevDisplay->mDisplay) {
+      mColGroups.InsertFrame(nsnull, aPrevFrame, aFrameList);
+    }
+    else {
+      NS_ERROR("Wrong sibling frame");
+      mColGroups.AppendFrame(nsnull, aFrameList);
+    }

Is this actually correct? When aPrevFrame is not valid, shouldn't we search mColGroups to find the frame for the last colgroup element that's before aFrameList's first frame's element?
Comment on attachment 241200 [details] [diff] [review]
patch

Roc, oh yes, that is brilliant. I had given up the hope that one do it correctly but you are right. We can simply loop over the frame list if the aPrevFrame is wrong we search for decent one.
Attachment #241200 - Flags: superreview?(roc)
Attachment #241200 - Flags: review?(roc)
Attached patch revised patch (obsolete) — Splinter Review
This includes now the full "pseudo awareness" for digging up content.
Attachment #244753 - Flags: superreview?(roc)
Attachment #244753 - Flags: review?(roc)
+    if (display != prevDisplay) {

Shouldn't this be "display->mDisplay != prevDisplay->mDisplay"?

Maybe we don't really need all this code. For this special case, could we just append the frame to either mColGroups or mFrames and then call nsFrameList::SortByContentOrder() on the list?
Attached patch revised patchSplinter Review
yes you are right the comparison worked only because of sharing/not sharing. CompareByContentOrder will probably fail for table pseudos 

<foo display: block>
  <bar display row>  
 body.offsetHeight
  <baz display: row>

the rowgroups for bar and baz will both return foo as content is causes a rowgroup around bar and another one around baz. Thats the drilling part of the patch.
Attachment #244753 - Attachment is obsolete: true
Attachment #245236 - Flags: superreview?(roc)
Attachment #245236 - Flags: review?(roc)
Attachment #244753 - Flags: superreview?(roc)
Attachment #244753 - Flags: review?(roc)
Attachment #245236 - Flags: superreview?(roc)
Attachment #245236 - Flags: superreview+
Attachment #245236 - Flags: review?(roc)
Attachment #245236 - Flags: review+
fix + test case checked in 

/cvsroot/mozilla/testing/mochitest/tests/test_bug337124.html,v  <--  test_bug337
124.html
initial revision: 1.1
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
cool test. please mark in-testsuite+ when you land them :)
Flags: in-testsuite+
Flags: in-testsuite+ → in-testsuite?
Boris why did you mark the flag as 'in&#8209;testsuite?'. "Mochito" is mentioned at http://developer.mozilla.org/en/docs/Mozilla_automated_testing and I did a checkin.
Ah, sorry.  I just saw the "please mark in-testsuite+ when you land them" comment and assumed sayrer had mis-clicked.
Flags: in-testsuite? → in-testsuite+
Comment on attachment 245236 [details] [diff] [review]
revised patch

I think this is branch worth
Attachment #245236 - Flags: approval1.8.1.2?
Attachment #245236 - Flags: approval1.8.0.10?
Comment on attachment 245236 [details] [diff] [review]
revised patch

Approved for both branches, a=jay for drivers.
Attachment #245236 - Flags: approval1.8.1.2?
Attachment #245236 - Flags: approval1.8.1.2+
Attachment #245236 - Flags: approval1.8.0.10?
Attachment #245236 - Flags: approval1.8.0.10+
argh,  I landed the fix on branches without citing the bug number
Verified fixed for 1.8.1.2 and 1.8.0.10 

using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/2007011603 BonEcho/2.0.0.2pre

and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.10pre) Gecko/20070116 Firefox/1.5.0.10pre
Status: RESOLVED → VERIFIED
Depends on: 371925
Crash Signature: [@ nsTableRowGroupFrame::GetNumLines] [@ nsTableRowGroupFrame::IncrementalReflow]
You need to log in before you can comment on or make changes to this bug.