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

RESOLVED FIXED

Status

()

Core
Layout: Tables
--
critical
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: William Shubert, Assigned: Bernd)

Tracking

({crash, testcase})

Trunk
x86
All
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 145197 [details]
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

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

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Summary: Both mozilla 1.4.1 and firefox 0.8 crash when trying to render small sample page. → Both mozilla 1.4.1 and firefox 0.8 crash [@ ProcessPseudoFrame] when trying to render small sample page.

Comment 3

13 years ago
Created attachment 145212 [details]
Stack Trace

Comment 4

13 years ago
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>
(Reporter)

Comment 5

13 years ago
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

13 years ago
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
(Assignee)

Comment 7

13 years ago
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 
Assignee: nobody → bernd_mozilla

Updated

13 years ago
Keywords: crash, testcase
(Assignee)

Comment 8

13 years ago
Created attachment 146931 [details] [diff] [review]
hack that makes us render the page
(Assignee)

Comment 9

13 years ago
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.

Updated

13 years ago
Attachment #146931 - Flags: superreview?(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.
Attachment #146931 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 11

13 years ago
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.
(Assignee)

Comment 12

13 years ago
Created attachment 151151 [details] [diff] [review]
patch
(Assignee)

Comment 13

13 years ago
Created attachment 151219 [details] [diff] [review]
patch rev2
Attachment #151151 - Attachment is obsolete: true
(Assignee)

Comment 14

13 years ago
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
Attachment #151219 - Flags: superreview?(dbaron)
Attachment #151219 - Flags: review?(dbaron)
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...
Attachment #151219 - Flags: superreview?(dbaron)
Attachment #151219 - Flags: superreview+
Attachment #151219 - Flags: review?(dbaron)
Attachment #151219 - Flags: review+
(Assignee)

Comment 16

13 years ago
Comment on attachment 151219 [details] [diff] [review]
patch rev2

once this has baken on trunk it should go into branch
Attachment #151219 - Flags: approval1.7.1?
(Assignee)

Comment 17

13 years ago
fix checked in

Comment 18

13 years ago
Any chance of getting a tiny patch that just has the fix and a quick comment for
the branches?
(Assignee)

Comment 19

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

Comment 20

13 years ago
Created attachment 155675 [details] [diff] [review]
shorter patch

Comment 21

13 years ago
Comment on attachment 155675 [details] [diff] [review]
shorter patch

a=mkaply for 1.7.3
Attachment #155675 - Flags: approval1.7.3+

Updated

13 years ago
Attachment #151219 - Flags: approval1.7.3?
(Assignee)

Comment 22

13 years ago
Comment on attachment 155675 [details] [diff] [review]
shorter patch

the patch landed on 1.7.3 aviary might need that too
Attachment #155675 - Flags: approval-aviary?
Comment on attachment 155675 [details] [diff] [review]
shorter patch

a=ben@mozilla.org
Attachment #155675 - Flags: approval-aviary? → approval-aviary+
(Assignee)

Comment 24

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

Comment 25

13 years ago
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?
(Assignee)

Comment 26

13 years ago
fix checked in into the avairy
(Assignee)

Comment 27

13 years ago
.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 28

8 years ago
layout/tables/crashtests/239294-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3
Flags: in-testsuite+
Crash Signature: [@ ProcessPseudoFrame]
You need to log in before you can comment on or make changes to this bug.