311661 - Evil xul testcase, using display:table-row causes crash [@ nsTableRowGroupFrame::GetFirstRow]

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase.

When clicking on the button, current trunk builds crash. Mozilla1.7 doesn't
crash, so it seems a regression.

Talkback ID: TB10385242H
0x00000645
nsTableRowGroupFrame::GetFirstRow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/tables/nsTableRowGroupFrame.cpp,
line 439]
nsTableRowGroupFrame::Reflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/tables/nsTableRowGroupFrame.cpp,
line 1262]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp,
line 891]
nsTableFrame::ReflowChildren 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/tables/nsTableFrame.cpp,
line 3199]
nsTableFrame::ReflowTable 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/tables/nsTableFrame.cpp,
line 2139]
nsTableFrame::Reflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/tables/nsTableFrame.cpp,
line 1976]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp,
line 891]
nsTableOuterFrame::OuterReflowChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/tables/nsTableOuterFrame.cpp,
line 1312]
nsTableOuterFrame::Reflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/tables/nsTableOuterFrame.cpp,
line 1965]
nsFrame::BoxReflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsFrame.cpp,
line 5322]
nsFrame::RefreshSizeCache 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsFrame.cpp,
line 4826]
nsFrame::GetAscent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsFrame.cpp,
line 5034]
nsSprocketLayout::GetAscent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 1564]
nsBoxFrame::GetAscent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 987]
nsSprocketLayout::Layout 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 260]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1106]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1106]
nsRootBoxFrame::Reflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp,
line 226]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp,
line 891]
ViewportFrame::Reflow 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsViewportFrame.cpp,
line 239]
IncrementalReflow::Dispatch 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 859]
PresShell::ProcessReflowCommands 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6492]
PresShell::WillPaint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6136]
SHELL32.dll + 0x520c24 (0x778b0c24)

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Doesn't crash with 2004-08-09 build, but crashes with 2004-08-10 build, so
again, I'm thinking of 230170.

Comment 3

[Bernd]

And as usual bug 230170 made it only obvious rathen than it is the cause

Comment 4

[Bernd]

xul buttons as childs of nsTableRowGroup .... :-(

Comment 5

[Boris Zbarsky [:bzbarsky]]

Do we need a IsSpecialXULContent method?

Or is it time to think about a different approach to deciding when to do
pseudo-frames?

Comment 6

[Bernd]

Attachment: testcase that should crash before bug 230170 was checked in

Comment 7

[Bernd]

Attachment: broken frame hierarchy that shows the problem

Comment 8

[Bernd]

frame hierarchy

webshell=02A99A30 
Viewport(-1)@02B7E464 [view=03AB4608] {0,0,9030,5265} [state=00042010]
[content=00000000] [sc=02B7E368] pst=:-moz-viewport<
  Box(window)(-1)@02B7E4F8 {0,0,9030,5265} [state=81c40080] [content=02B914A0]
[sc=02B7E570] pst=:-moz-canvas<
    DocElementBox(window)(-1)@02B7E81C {0,0,9030,5265} [state=81a40090]
[content=02B914A0] [sc=02B7E650]<
      PopupSet(popupgroup)(-1)@02B7E904 next=02B7EC48 {0,0,9030,0}
[state=80c000a0] [content=03AD8778] [sc=02B7E764]<>
      Placeholder(tooltip)(-1)@02B7EC48 {0,0,9030,0}
outOfFlowFrame=MenuPopup(tooltip)(-1)@02B7EB8C
      TableOuter(window)(-1)@03AFB4F0 {0,0,9030,0} [state=00010000]
[content=02B914A0] [sc=03AFB594] pst=:-moz-table-outer<
        Table(window)(-1)@03AFB654 {0,0,0,0} [state=00010000] [content=02B914A0]
[sc=03AFB420] pst=:-moz-table<
          TableRowGroup(window)(-1)@02B7E8AC {0,0,0,0} [state=00010000]
[content=02B914A0] [sc=03AFB754] pst=:-moz-table-row-group<
            Box(button)(1)@03AFAED0 next=03AFB800 {0,0,0,0} [state=80c004b2]
[content=02B91FB0] [sc=03B092B8]<
              Box(hbox)(-1)@03AFB004 {0,0,0,0} [state=804004a2]
[content=03AFC3A0] [sc=03B0947C]<
                ImageBox(image)(-1)@03AFB250 {0,0,0,0} [state=000004a2]
[content=03AFD4B0]
                Text(label)(-1)[value=view frame hierarchy after pressing the
button]@03AFB2FC {0,0,0,0} [state=010004a2] [content=03AFD5C8]
              >
            >
            TableRow(div)(2)@03AFB800 {0,0,0,0} [state=00010000]
[content=02B92208] [sc=02B7EF64]<>
          >
        >
      >
    >
  >
>

Comment 9

[Bernd]

So what happens here seems like logic issue in ContentInserted

<button onclick="doe()" label="view frame hierarchy after pressing the button"/>
<div style="display:table-row"/>

this will create a pseudo row group, pseudo table and pseudo outer table around
the div.

if then comes a display change on the button, in ContentInserted

we start with the right parentframe: DocElementBox
but then we see that we must insert the frame before the div so we look up the
parent of the frame that corresponds to the div and find the pseudo row group.
This seems to be broken.

But the story continues as Boris suggested, there is no IsSpecialXULContent so
we dont create a pseudo cell frame around the button.

Comment 10

[Bernd]

Attachment: patch

Its not the optimal solution. But it tries to close the hooles now, to get time
to rework the pseudo stuff.

Comment 11

[Bernd]

Comment on attachment 199025 [details] [diff] [review]
patch

Yes, I will fix the indent...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199025 [details] [diff] [review]
patch

>Index: nsCSSFrameConstructor.cpp
>+  PRInt32 nameSpaceID = aContent->GetNameSpaceID();
>+  nsIAtom* tag = aContent->Tag();

I just realized that IsSpecialHTMLContent has this same bug -- those are not
necessarily the tag and ID that will be used for frame construction.  We
probably need to pass those in from ConstructFrameInternal...

>+    return
>+      tag == nsXULAtoms::button ||
>+	  tag == nsXULAtoms::checkbox ||
>+	  tag == nsXULAtoms::radio ||
>+	  tag == nsXULAtoms::autorepeatbutton ||
>+      tag == nsXULAtoms::titlebar ||

Fix indent?  Also, some of these are only special #ifdef MOZ_XUL, no?

>+  if (nameSpaceID == kNameSpaceID_SVG &&

And all of this should be #ifdef MOZ_SVG

>+  if (nameSpaceID == kNameSpaceID_MathML)

#ifdef MOZ_MATHML

Comment 13

[Bernd]

Attachment: revised patch

Comment 14

[Bernd]

The patch differs from the original one by the follwoing:

- whitespace...
- all the special frames are now in one function, so that it will become easier to 
- I reorderd the arguments in adjustaprentframe, so that they follow the
constructfunctions
- hopefully all review comments are addressed.

Comment 15

[Bernd]

Attachment: patch with the changes to the .h file

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199166 [details] [diff] [review]
patch with the changes to the .h file

>Index: nsCSSFrameConstructor.cpp
>+  if (aContent->IsContentOfType(nsIContent::eHTML)) {

  || aNameSpaceID == kNameSpaceID_XHTML

here, just like ConstructHTMLFrame.

>+#ifdef MOZ_XUL
>+  if (aNameSpaceID == kNameSpaceID_XUL)

No, the ifdef should be around some of the tag compares; see below.

>+      aTag == nsXULAtoms::menubar ||

menubar should probably be #ifndef XP_MACOSX with some more "keep in sync"
comments here and where we create them.

>+      aTag == nsXULAtoms::slider ||
>+      aTag == nsXULAtoms::scrollbar ||
>+      aTag == nsXULAtoms::nativescrollbar ||
>+      aTag == nsXULAtoms::scrollbarbutton ||

These four we do special stuff for even if MOZ_XUL is not defined, so they need
to be tested for unconditionally here...

>Index: nsCSSFrameConstructor.h
>+   * @param aaNameSpaceID namespace where that the frame would use

"Namespace that will be used for frame construction"

r+sr=bzbarsky with those fixed.

Comment 17

[Bernd]

Attachment: patch that got checked in

I did run a regression test on it. The benefit is that we close a crash hole.
The risk is that we touch frame construction code.

Comment 18

[Bernd]

fix checked in

Comment 19

[Asa Dotzler [:asa]]

Do we think this is exploitable? I don't see it in the topcrash data and I'm
loathe to take any significant risk for something that's not a topcrash.

Comment 20

[Worcester12345]

I also got a long hang which ended up in it crashing on the branch.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051012
Firefox/1.4.1 ID:2005101205


Has this fix been checked into the branch also? For some reason, my Talkback
will come up, but not send. This has been going on since the betas started or
thereabouts.

Comment 21

[Bernd]

> Do we think this is exploitable?
I don't know. For me it just crashes. But for me bug 308752 also was a pure
crash. Maybe Jesse has an idea. 

Comment 22

[Boris Zbarsky [:bzbarsky]]

The crash (on the first testcase) happens because we have an object of type X
that we think is of type Y.  So we cast it to type Y and then call a virtual
method on the resulting Y*.  Since it's not actually of type Y, we don't end up
jumping to where we should be jumping.

So whether this is exploitable depends on exact vtable memory layouts, etc. 
Chances are, it would be, given the variety of vtables that all of our XUL,
MathML, and SVG frames have...

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED (no crashes) on both testcases using build 2005-10-13-05 of
SeaMonkey trunk Windows XP.

Comment 24

[Bernd]

Attachment: patch ported to branch

Comment 25

[Bernd]

Attachment: revised

Comment 26

[Bernd]

Attachment: revised

Comment 27

[Bernd]

fix checked in on branch

just for the record, this could have been easily fixed in april 2000, 
(/me bows towards the master)

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsCSSFrameConstructor.cpp&branch=&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&rev1=1.408&rev2=1.409

Comment 28

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/83769d48ee58