239294 - Both mozilla 1.4.1 and firefox 0.8 crash [@ ProcessPseudoFrame] when trying to render small sample page.

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[William Shubert]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040211 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040211 Firefox/0.8

I was writing a web page, and found that firefox 0.8 crashes when it tries to
render the page! Testing showed mozilla 1.4.1 does the same thing, so I kept
ripping things out until I got a small (38 line) HTML file that crashes both
every time. The problem seems to be in rendering a table that is
"display:inline" and contains a colgroup.

Reproducible: Always
Steps to Reproduce:
1. Visit page (I'll add the page as an attachment)
2. Watch mozilla or firefox go boom
3. Send bug to mozilla.org

Actual Results:  
Browser completely crashes - all windows close, application exits.

Expected Results:  
Should have simple rendered the page.

Comment 1

[William Shubert]

Attachment: Example XHTML file that makes browser crash

The HTML comments in the page explain how to make it stop crashing. As the file
stands, it crashes whether loaded from a web server or just from a file,
doesn't matter at all.

Comment 2

[Marius Strumyla]

both firefox0.8 and mozilla1.7b crashes on win2k. talkback id: TB9331E

Comment 3

[WD]

Attachment: Stack Trace

Comment 4

[Hermann Schwab]

validator says: This page is not Valid XHTML 1.0 Strict!

Line 19, column 21:  document type does not allow element "colgroup" here  

    <colgroup span="12"></colgroup>
                       ^

http://validator.w3.org/check?uri=http%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D145197%26action%3Dview

Part of the attachments source code:
<!--
  To make mozilla render this properly, take out the "display: inline"
  style, or take out the colgroup - getting rid of either makes
  everything work fine!!!
-->
<table style="display: inline">
  <col></col>
  <colgroup span="12"></colgroup>
  <tr>
    <th>Year</th>
    <th colspan="12">Month</th>
  </tr><tr>
    <td>2001</td>

Comment 5

[William Shubert]

Odd - if the initial "<col></col>" is changed to "<colgroup
span="1"></colgroup>", which makes the page valid, the mozilla no longer
crashes. So this is less severe than I thought at first, since mozilla only
crashes when trying to render an invalid page.

Comment 6

[Anders Pedersen]

Confirmed crash launching the testcase.

talkbackid: TB9533G
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040331
Microsoft Windows 2000 Professional 5.00.2195 SP4

Comment 7

[Bernd]

And I thought my interest for col frame creation (bug 238999) was more academic.
I will fix this in the week starting from 2004-04-11 as I will be away from
03rd-10th of April. If somebody would like to have this fixed for 1.7, feel free
to take it.

Boris thats probably one of those places where the colframe creation causes
asserts in the LastChild patch 

Comment 8

[Bernd]

Attachment: hack that makes us render the page

Comment 9

[Bernd]

There is only one way to fix this bug and Chris described the way in 1999
at
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/html/table/src&command=DIFF&root=/cvsroot&file=nsTableFrame.cpp&rev1=3.346&rev2=3.347#3

Currently the colgroup and rowgroup go on the main childlist inside the frame
constructor and are only split up at nsTableFrame::SetInitialChildList.

The pseudocolgroup handling is problem.In the testcase the display:inline on the
table converts the table into one inline. So all childs of the table become
wrapped by pseudo table. The first <col> tag can't be direct children of the
table so we wrap it with an anonymous colgroup. Then we encounter an real
colgroup and process the previously created anonymous colgroup. As we did not
mark the lowest frame to be a colgroup frame (the lowestframe mechanism assumes
basically the main child list only: outer table - table - row group - row -
outer cell - inner cell) at this point the lowestframe is the table, which
causes a complete processing of the table on the next step the rowgroup is
created and as the pseudotable should  already be created it crashes because the
table pseudo has been processed already.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146931 [details] [diff] [review]
hack that makes us render the page

I don't do lone srs for now, and I'm certainly not able to do an r here any
time soon.  In any case, I didn't get the impression that bernd considered this
patch acceptable for checkin.

Comment 11

[Bernd]

As I wrote the patch is a hack. We had this crash since karnaze wrote this in
2000 I believe. This is a result of a architectural problem that I tried to
outline in my previous comment. I can't hack this, I don't see a reasonable
workaround, this needs to be fixed once and for all. This requires a larger
change also in nsTableFrame.cpp. I need then somebody who critical reviews this
(aka boris or david). Currently we have one crash report since 2000 about this
place with a pretty seldom structure. This will probably not ready before 1.7
but before ff 1.0.

Comment 12

[Bernd]

Attachment: patch

Comment 13

[Bernd]

Attachment: patch rev2

Comment 14

[Bernd]

Comment on attachment 151219 [details] [diff] [review]
patch rev2

the patch fixes the crash and the limits the assertion that boris added to the
really critical cases

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 151219 [details] [diff] [review]
patch rev2

>+// PseudoFrame are necessary when the childframe cannot be the direct
>+// ancestor of the gemoetrical parent frame. The amount of necessary pseudo

Maybe refer to CSS?  Also, what's a geometrical parent frame?  It seems like
you really want to refer to content tree relationships.

>+#ifdef DEBUG
>+  // there are two situations where table related frames will wrap around
>+  // foreign frames
>+  // a) inner table cell, which is a pseudo frame
>+  // b) caption frame which will be always a real frame.
>+  nsIAtom* parentFrameType = parentFrame->GetType();
>+  if (nsLayoutAtoms::tableCaptionFrame != parentFrameType) {
>+    NS_ASSERTION(parentFrame == aState.mPseudoFrames.mCellInner.mFrame,
>                "Weird parent in ConstructTableForeignFrame");
>-
>+  }
>+#endif

You don't need ifdefs -- you can just use boolean logic:

  NS_ASSERTION(nsLayoutAtoms::tableCaptionFrame != parentFrameType ||
	       parentFrame == aState.mPseudoFrames.mCellInner.mFrame,
	       "Weird parent in...

Comment 16

[Bernd]

Comment on attachment 151219 [details] [diff] [review]
patch rev2

once this has baken on trunk it should go into branch

Comment 17

[Bernd]

fix checked in

Comment 18

[Mike Kaply [:mkaply]]

Any chance of getting a tiny patch that just has the fix and a quick comment for
the branches?

Comment 19

[Bernd]

That would be
http://bugzilla.mozilla.org/attachment.cgi?id=151219&action=diff#mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp_sec2

Comment 20

[Bernd]

Attachment: shorter patch

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 155675 [details] [diff] [review]
shorter patch

a=mkaply for 1.7.3

Comment 22

[Bernd]

Comment on attachment 155675 [details] [diff] [review]
shorter patch

the patch landed on 1.7.3 aviary might need that too

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 155675 [details] [diff] [review]
shorter patch

a=[e-mail]

Comment 24

[Bernd]

could somebody please check in the approved patch, I don't have a aviary branch
build. Thanks

Comment 25

[Jay Patel [:jay]]

This is definitely still crashing the latest Firefox10 Aviary builds.  Here's my
crash with the 8/17 build:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=587296

If Bernd can't check this into the Aviary, who can?

Comment 26

[Bernd]

fix checked in into the avairy

Comment 27

[Bernd]

.

Comment 28

[Bob Clary [:bc] (inactive)]

layout/tables/crashtests/239294-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3