189751 - mozilla crashes when rendering <input type="browse"> nested inside a table

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[Perry Lorier]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2) Gecko/20021126
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2) Gecko/20021126

code below will crash mozilla:
<form method="post">
 <table><tr><td><input type="browse"></td></tr></table>
</form>

put it in a webpage somewhere, goto that page with mozilla, note that mozilla is
no longer running.  Swear, Curse.

Reproducible: Always

Steps to Reproduce:




Expected Results:  
Something other than a crash, I dunno what <input type="browse"> is supposed to
do, but presumably not crash.

Comment 1

[Vladimir Ermakov]

Attachment: testcase

Comment 2

[Vladimir Ermakov]

Confirming on XP. 

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is a tables bug:

#0  nsTableFrame::IsBorderCollapse (this=0x0)
    at /home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableFrame.h:1088
#1  0x40f08652 in nsTableOuterFrame::InitChildReflowState (this=0x87cded4, 
    aPresContext=@0x86cbca0, aReflowState=@0xbfffc818)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableOuterFrame.cpp:483
#2  0x40f08752 in nsTableOuterFrame::GetMarginPadding (this=0x87cded4, 
    aPresContext=0x86cbca0, aOuterRS=@0xbfffcc04, aChildFrame=0x0,
aAvailWidth=13636, 
    aMargin=@0xbfffc90c, aMarginNoAuto=@0xbfffc9ec, aPadding=@0xbfffca0c)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableOuterFrame.cpp:513
#3  0x40f08cac in nsTableOuterFrame::GetInnerTableAvailWidth (this=0x87cded4, 
    aPresContext=0x86cbca0, aInnerTable=0x0, aOuterRS=@0xbfffcc04, 
    aCaptionWidth=0xbfffc990, aInnerMargin=@0xbfffc9ec, aInnerPadding=@0xbfffca0c)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableOuterFrame.cpp:731
#4  0x40f0b931 in nsTableOuterFrame::Reflow (this=0x87cded4,
aPresContext=0x86cbca0, 
    aDesiredSize=@0xbfffcdc8, aOuterRS=@0xbfffcc04, aStatus=@0xbfffcbf4)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableOuterFrame.cpp:1984
#5  0x40e2f518 in nsBlockReflowContext::ReflowBlock (this=0xbfffcd84, 
    aSpace=@0xbfffccbc, aApplyTopMargin=0, aPrevBottomMargin=@0xbfffd28c, 
    aIsAdjacentWithTop=1, aComputedOffsets=@0xbfffcccc, aFrameRS=@0xbfffcc04, 
    aFrameReflowStatus=@0xbfffcbf4)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsBlockReflowContext.cpp:546
#6  0x40e2821c in nsBlockFrame::ReflowBlockFrame (this=0x87cdc94,
aState=@0xbfffd224, 
    aLine={mCurrent = 0x87ce91c, mListLink = 0x87cdcd0},
aKeepReflowGoing=0xbfffcf48)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsBlockFrame.cpp:3330
#7  0x40e26cef in nsBlockFrame::ReflowLine (this=0x87cdc94, aState=@0xbfffd224,
aLine=
      {mCurrent = 0x87ce91c, mListLink = 0x87cdcd0}, aKeepReflowGoing=0xbfffcf48, 
    aDamageDirtyArea=0)

(gdb) frame 0
#0  nsTableFrame::IsBorderCollapse (this=0x0)
    at /home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableFrame.h:1088
1088      return (PRBool)mBits.mIsBorderCollapse;
(gdb) p this
$4 = (nsTableFrame *) 0x0
(gdb) frame 1
#1  0x40f08652 in nsTableOuterFrame::InitChildReflowState (this=0x87cded4, 
    aPresContext=@0x86cbca0, aReflowState=@0xbfffc818)
    at
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/table/src/nsTableOuterFrame.cpp:483
483       if ((aReflowState.frame == mInnerTableFrame) &&
(mInnerTableFrame->IsBorderCollapse())) {
(gdb) p mInnerTableFrame
$5 = (nsTableFrame *) 0x0

Comment 4

[Bernd]

>I dunno what <input type="browse"> is supposed to do, but presumably not crash.
nsCSSFrameConstructor.cpp also does not know what to do with that and asserts.

Comment 5

[Bernd]

Attachment: patch

defence line for tables

Comment 6

[Bernd]

Attachment: patch revised

this patch only protects the reflow from a failing frame construction, in a
next step two things should happen
a) the input frames should be fixed
b) the table frame construction should be changed with respect to the handling
of the return values, in this case only the input should be not rendered and
not the whole table marked as invalid.

Comment 7

[John Keiser (jkeiser)]

Comment on attachment 112538 [details] [diff] [review]
patch revised

this might look nicer if you did NS_ENSURE_TRUE(!mFrames.IsEmpty() &&
mInnerTableFrame, NS_ERROR_FAILURE); though you don't have to--you'd lose the
(IMO useless) "incomplete children" comment.

r=jkeiser, your call whether you want to do that.

Comment 8

[Bernd]

My intention was to assert here.

Comment 9

[Bernd]

Comment on attachment 112538 [details] [diff] [review]
patch revised

David, could you please sr  if you think we should take this  into 1.3b.?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Um.. how about also removing that browse stuff?  

Comment 11

[Bernd]

I dont undertstand what you mean. Could you be a little bit more verbose. Do you
mean that the frame construction should not verify the return values? I am
somehow reluctant to fiddle with frame construction in a freeze phase. The patch
is wallpaper, but my intention was to work on it in 1.4.

Comment 12

[Boris Zbarsky [:bzbarsky]]

What I meant is that NS_FORM_BROWSE should be removed and any places that use it
fixed to not do that.  It does not correspond to anything useful.

Not for 1.3, of course....

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+ // At this point, we must have an inner table frame, and we might have a
caption    
+ // due to the logic at nsCSSFrameConstructor::ConstructTableFrame we can end 
+ // here without inner table frame

This should be

// At this point, we need an inner table frame, and we might have a caption.  
// Due to the logic in nsCSSFrameConstructor::ConstructTableFrame, we can end 
// here without an inner table frame.

right??

Comment 14

[Bernd]

Robert, yes this is better, I am very gratefull for every language correction.
English is not my first language.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure, no problem. Your English is a whole lot better than my German :-).

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 112538 [details] [diff] [review]
patch revised

with the wording change, sr=roc+moz

Comment 17

[Bernd]

patch checked in, now there are two task remaining:
1.) make table frame construction less vulnerable to bad frames, they should
simply remove the offending frames but return success for their own task and not
propagate endless the error condition. I will do this.
2.) remove the offending frames at all: <input type="browse"> should be simply
ignored, I would like to leave that for John.
 

Comment 18

[Madhur Bhatia]

On winXP today's trunk build, the testcase attachment 112088 [details] does not crash the
browser 

Comment 19

[Boris Zbarsky [:bzbarsky]]

Bug 242873 filed on the removal.  Do we want a separate bug on the frame
construction?

Comment 20

[Bernd]

>Do we want a separate bug on the frame construction?
not really, what would that bug be? "Write nice code in frame constructor" ;-).
The only sensible bug would be "table frame construction should try to handle
NS_FAILED(rv) at the lowest possible level" But I dont see any gain from such a
bug report, if somebody would like to rewrite the frame construction code she/he
will do that anyway.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yeah, the bug there would be "handle failed rv in frame construction in a sane
way"....

We should probably spin that off and mark this fixed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

The frame construction changes discussed happened in bug 237760.  Marking this
fixed.

Comment 23

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1