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

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
15 years ago
8 years ago

People

(Reporter: wms, Assigned: bernd_mozilla)

Tracking

({crash, testcase})

Trunk
x86
All
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

15 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

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

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

Updated

15 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

15 years ago
Posted file Stack Trace

Comment 4

15 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

15 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

15 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

15 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

15 years ago
Keywords: crash, testcase
(Assignee)

Comment 8

15 years ago
(Assignee)

Comment 9

15 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

15 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

15 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

15 years ago
Posted patch patch (obsolete) — Splinter Review
(Assignee)

Comment 13

15 years ago
Posted patch patch rev2Splinter Review
Attachment #151151 - Attachment is obsolete: true
(Assignee)

Comment 14

15 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

15 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

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

Comment 20

15 years ago
Posted patch shorter patchSplinter Review
Comment on attachment 155675 [details] [diff] [review]
shorter patch

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

Updated

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

Comment 22

15 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

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

Comment 25

15 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

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

Comment 27

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

Comment 28

10 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.