374193 - [FIX]Crash [@ nsCSSFrameConstructor::GetFrameFor] with mtable, th and an xbl binding

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, this usually crashes for me the first time, if not, try reloading a few times.
Talkback ID: TB30287494W
nsCSSFrameConstructor::GetFrameFor  [mozilla/layout/base/nscssframeconstructor.cpp, line 7708]
0x02d373e8

This doesn't crash for me in a 2007-01-02 build, but does crash in a 2007-01-03 build:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-01-02+04&maxdate=2007-01-03+09&cvsroot=%2Fcvsroot
Regression from bug 243159, somehow?

It also contains an xbl binding (conveniently embedded now, because bug 371662 is fixed, the xbl binding is this:
<bindings xmlns="http://www.mozilla.org/xbl">
<binding id="a">
<implementation>
<constructor>
  this.style.position='fixed';
</constructor>
</implementation>
<content><children/></content>
</binding>
</bindings>

Comment 1

[Martijn Wargers (dead)]

I'm also getting this talkback sometimes: TB30287802E
nsPlaceholderFrame::CanContinueTextRun  [mozilla/layout/generic/nsplaceholderframe.cpp, line 159]

Comment 2

[Bernd]

Regression from bug 243159, somehow?
sure before bug 243519 xbl on table display types was completely broken.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

The key is that the patch for bug 347355 is wrong -- the <mtable> doesn't create a table frame, so we need to treat it as special.

The other changes are:

1)  <math> is special but wasn't treated as such.
2)  Once we create pseudos for the <mtable>, we need to be careful not to lose them when we construct the table frame inside it.

We probably want this on branches too...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Non-XBL testcase that also crashes for me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Martijn, does that patch fix things for you?  For me this crash was 100% reproducible, so I want to make sure I'm fixing the thing you're seeing.

Comment 6

[Martijn Wargers (dead)]

Yeah, the patch fixes the crash for me.

Comment 7

[rbs]

Comment on attachment 260026 [details] [diff] [review]
Proposed patch

sr=rbs

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Regression test

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 260026 [details] [diff] [review]
Proposed patch

We should probably take this on branches.  I think this is pretty safe, as far as this code goes.  It's certainly theoretically the "right thing".

Comment 11

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070411 Minefield/3.0a4pre

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 260026 [details] [diff] [review]
Proposed patch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 260026 [details] [diff] [review]
Proposed patch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch patch

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 16

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.4 - tested with the testcases from comment #0 and #4 with Builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4pre) Gecko/2007042803 BonEcho/2.0.0.4pre Fedora FC6

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre

and verified fixed 1.8.0.12 on Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.12pre) Gecko/20070428 Firefox/1.5.0.12pre -> no crash on testcases -> adding verified keyword