Closed
Bug 337124
Opened 18 years ago
Closed 18 years ago
Crash [@ nsTableRowGroupFrame::GetNumLines] with table-row-group/table-column-group [@ nsTableRowGroupFrame::IncrementalReflow]
Categories
(Core :: Layout: Tables, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: martijn.martijn, Assigned: bernd_mozilla)
References
Details
(5 keywords)
Crash Data
Attachments
(6 files, 1 obsolete file)
576 bytes,
text/html
|
Details | |
19.62 KB,
text/html
|
Details | |
5.54 KB,
text/plain
|
Details | |
2.67 KB,
patch
|
Details | Diff | Splinter Review | |
3.02 KB,
patch
|
roc
:
review+
roc
:
superreview+
jay
:
approval1.8.1.2+
jay
:
approval1.8.0.10+
|
Details | Diff | Splinter Review |
2.84 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Comment 2•18 years ago
|
||
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....
Reporter | ||
Comment 5•18 years ago
|
||
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
Updated•18 years ago
|
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]
Assignee | ||
Comment 10•18 years ago
|
||
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.
Assignee | ||
Comment 11•18 years ago
|
||
Flags: blocking1.9a2? → blocking1.9+
Assignee | ||
Comment 12•18 years ago
|
||
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?
Assignee | ||
Comment 14•18 years ago
|
||
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)
Assignee | ||
Comment 15•18 years ago
|
||
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?
Assignee | ||
Comment 17•18 years ago
|
||
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+
Assignee | ||
Comment 18•18 years ago
|
||
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: 18 years ago
Resolution: --- → FIXED
Comment 19•18 years ago
|
||
cool test. please mark in-testsuite+ when you land them :)
Flags: in-testsuite+
Updated•18 years ago
|
Flags: in-testsuite+ → in-testsuite?
Assignee | ||
Comment 20•18 years ago
|
||
Boris why did you mark the flag as 'in‑testsuite?'. "Mochito" is mentioned at http://developer.mozilla.org/en/docs/Mozilla_automated_testing and I did a checkin.
Comment 21•18 years ago
|
||
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+
Assignee | ||
Comment 22•18 years ago
|
||
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 23•18 years ago
|
||
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+
Assignee | ||
Comment 24•18 years ago
|
||
Assignee | ||
Comment 25•18 years ago
|
||
argh, I landed the fix on branches without citing the bug number
Keywords: fixed1.8.0.10,
fixed1.8.1.2
Comment 26•18 years ago
|
||
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
Updated•13 years ago
|
Crash Signature: [@ nsTableRowGroupFrame::GetNumLines]
[@ nsTableRowGroupFrame::IncrementalReflow]
You need to log in
before you can comment on or make changes to this bug.
Description
•