363696 - Crash [@ nsHTMLReflowState::ComputeContainingBlockRectangle] with file upload control in xul

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[Jesse Ruderman]

Loading the testcase crashes Firefox.

<xul:hbox>
  <input type="file" />
</xul:hbox>

Comment 1

[Jesse Ruderman]

Attachment: testcase (crashes on load)

Comment 2

[Jesse Ruderman]

This regressed between 2006-12-07 and 2006-12-08.  I'm the regression is due to the landing of the reflow branch.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase2

The first testcase doesn't seem to crash anymore for me, using current trunk build.
However, with this testcase, I'm still crashing with this stack.

Comment 6

[Jesse Ruderman]

The first testcase still crashes for me [@ nsHTMLReflowState::ComputeContainingBlockRectangle].  Before the crash, I see:

###!!! ASSERTION: no containing block: 'nsnull != cbrs', file /Users/jruderman/trunk/mozilla/layout/generic/nsHTMLReflowState.cpp, line 1652

Comment 7

[Daniel Holbert [:dholbert]]

I don't get that assertion or any crashes under linux using either testcase. Is this known to be a Mac-only problem?

(I know it's marked as Mac OS X, but not sure if that's just 'cause it was first reported on a mac)

Comment 8

[Jeff Walden [:Waldo]]

I don't crash on either of these on Mac (latest trunk) or Fedora 7 (two-day-old build).  This looks to be WFM now, but we should get some testcases in the tree for this, which means I guess we're waiting on infrastructure in bug 397725 (assuming we're not keeping the tests in a central location).

Comment 9

[Martijn Wargers (dead)]

Attachment: testcase3

It is still crashing for me with this testcase, using latest nightly trunk build.

Comment 10

[Matthew Gregan [:kinetik]]

I can reproduce this on OS X with the testcase from comment #1 (but only if the local filename has the extension .xul, not .xml/.xhtml) and comment #9, but not with the one from comment #5.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 248503 [details]
testcase (crashes on load)

Hm, odd that the XUL content-type wasn't necessary originally, judging by the filename of the uploaded file.  Testing in the mochitest server indicates this *should* make the crash happen with no other changes.

Comment 12

[Matthew Gregan [:kinetik]]

The crash is occuring during the construction of |nsReflowState txtKidReflowState| in nsFileControlFrame::Reflow in a case where the FileControl(input) has a Box as the direct parent.  I suspect we are not seeing this crash on other platforms because we're not entering the conditional code that constructs the txtKidReflowState, but I haven't confirmed this yet.

The XUL content-type wasn't necessary originally because some behaviour closely related to this bug changed in trunk recently (2007-10-25 16:31, according to Bonsai) due to the patch from bug #321402 landing.  With a build containing that change, the FileControl(input) frame is wrapped inside a Block when testcase #1 is loaded as XHTML but not when loaded as XUL.  This is also why testcase #2 does not crash in recent trunk builds, and why testcase #3 only fails if there is no text inside the <span> containing the <input> element.

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: patch

Can we get away with just reflowing the text frame?  The attached patch is an experiment at trying this.  nsFileControlFrame::Reflow has a comment saying that doing so messes up the button rect, but that comment may be out of date because I can't see that happening with any of the tests cases I've tried (but perhaps I'm not hitting the right test case...).

The assert and crash hit by the test cases in this bug no long occur, and the crash reported in bug #317502 (for which an earlier fix changed the construction of |txtKidReflowState| to take |*aReflowState.parentState| rather than |aReflowState|, which resulted in us walking up past the last valid mCBReflowState when the input's direct parent is a box) does not occur.

Not sure if this patch takes the right approach to solving this issue, but posting to get some feedback.

Comment 14

[Matthew Gregan [:kinetik]]

Forgot to note that with this patch applied, the text frame ends up slightly taller if testcase #1 is loaded as content-type XUL when compared to the same file loaded as XHTML.  layout/html/tests/formctls/base/input_file_strict.html and layout/html/tests/formctls/base/input_file_strict2.html both look correct compared to trunk without this patch applied.

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: patch v2

Alternate way to solve this as suggested by Robert O'Callahan.  Drop the seemingly unnecessary second reflow of the area frame (or text frame in my earlier patch) completely.  Since the code performing the second reflow also manually tweaked the text frame's height, removing this code results in the text frame being aligned strangely (the baseline of the text frame and button are aligned, resulting in the text frame overflowing the element area) with the button when the input has a large fixed height (e.g. 100px).  Since we don't use more than one line of the text frame anyway, drop the |height: inherit| from the input[type="file"] so that the text frame remains an appropriate height for a single line of text.

Comment 16

[Matthew Gregan [:kinetik]]

Attachment: patch v3

Slightly cleaned up version of patch v2.

Comment 17

[Jeff Walden [:Waldo]]

It's a good idea to get in the habit of including testcases in your patches when you post them to bugs.  Doing so makes it obvious exactly what scenarios you have and haven't tested.  It also reduces the number of assumptions reviewers have to verify when making their reviews, because if a test anticipates a possible question, that question may not even need to be asked (alternately, its scope might be reduced).

Regarding comment 14, if you're seeing different results with different MIME types you should make sure to include tests for these two situations.  You can force specific MIME types using reftest HTTP support:

http://developer.mozilla.org/en/docs/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3F

You'll have a bit of fun with CVS not letting you add new files without commit access, but cvsdo can work around that problem for you:

http://viper.haque.net/~timeless/redbean/cvsdo

Also, the last and greatest benefit of including tests in your patches is you go on my AWESOME list.  :-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

We really do want to have the textfield end up the same height as the button.  Otherwise the control looks pretty ugly.  If you remove that code, they might easily not end up the same height.  Try something basic like:

  <input type="file" style="font-size: 10px; height: 40px">

I wish we'd had reftests back when I last touched this code; this patch would have just failed the tests I'd have written then.  :(

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 287950 [details] [diff] [review]
patch v3

That would be r-.

Oh, and while the example I gave is somewhat exaggerated (to make it clear what's going on), sites do commonly put a height style on the file input and expect it to be obeyed.  With this patch it wouldn't be.

Comment 20

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

Personally, I think a really tall textbox looks stupid because we only ever use the first line of it, and centering it vertically without resizing it looks just fine.

Comment 21

[Matthew Gregan [:kinetik]]

Attachment: screenshot - Boris's example on trunk

Comment 22

[Matthew Gregan [:kinetik]]

Attachment: screenshot - Boris's example on with proposed patch

Comment 23

[Matthew Gregan [:kinetik]]

Attachment: screenshot - Boris's example with proposed patch

Comment 24

[Matthew Gregan [:kinetik]]

Attachment: PicasaWeb upload forms on trunk

Comment 25

[Matthew Gregan [:kinetik]]

Attachment: PicasaWeb upload forms with proposed patch

Note that the real PicasaWeb form does not style the file upload control.  I added a style of height:40px to make comparison of this control styled in the context of a real site easier.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Personally, I think a really tall textbox looks stupid

Of course.  Usually sites that set a height style on a file input are making the file input shorter, not taller; it's just simpler to see what's going on with the bigger height.  Now we could ignore the height they set and make the file control too tall (as this patch does). But that will break layouts.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Oh, and when sites do increase the height they only increase it a little, as I said.

In any case, if we're consciously making the decision that we want to change behavior here, that's a different issue.  Then I'd like some information on what other UAs (IE, Opera, Safari) do.

Comment 28

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

This patch never makes the file control taller; a CSS 'height' is always honoured.

It can make the textbox overflow below the bottom edge of the file control. However, with the current code the text in the textbox overflows the bottom edge of the file control if the height is set to something small; the patch just makes the textbox border overflow as well.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> It can make the textbox overflow below the bottom edge of the file control.

Right.  Which can make the textbox overlap other content.  The text overflowing that way is not a problem, since the textbox has a scrollframe inside, so it gets clipped if needed.

Comment 30

[georgi - hopefully not receiving bugspam]

what makes the file control so special?

Comment 31

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

Attachment: testcase`

The textbox text actually doesn't get clipped properly on trunk, as shown in this testcase.

Comment 32

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

It seems the 3px height is inherited as the content-height of the textbox (as per forms.css), gets used to set up the scrollframe, and then the existing reflow code in nsFileControlFrame mangles the textbox frame size to get this effect.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Note that on Linux we do clip the text in the testcase from comment 31....

Comment 34

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

Attachment: screenshot of testcase

Comment 35

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

IRC discussion summary: let's try using Matthew's patch and adding code to clip drawing to the file control frame's border-box. We should also compare renderings of small file controls between FF2, IE7 and trunk-with-patch.

Comment 36

[Matthew Gregan [:kinetik]]

Attachment: Firefox 2.0.0.9 screenshot

Composite screenshots of Firefox 2.0.0.9 rendering the testcase from comment #31 and the existing testcases layout/html/tests/formctls/base/input_file_strict.html and layout/html/tests/formctls/base/input_file_strict2.html in the source tree.

Comment 37

[Matthew Gregan [:kinetik]]

Attachment: Internet Explorer 7 screenshot

IE7 rendering the same testcases.

Comment 38

[Matthew Gregan [:kinetik]]

Attachment: Safari 3.0.4 screenshot

Same with Safari 3.0.4.

Comment 39

[Matthew Gregan [:kinetik]]

Attachment: Opera 9.2.4 screenshot

Comment 40

[Matthew Gregan [:kinetik]]

Attachment: Current trunk unpatched

Comment 41

[Matthew Gregan [:kinetik]]

Attachment: Current trunk with proposed clipping patch applied

Comment 42

[Matthew Gregan [:kinetik]]

Attachment: patch v4

Comment 43

[Boris Zbarsky [:bzbarsky]]

Does "current trunk with proposed clipping patch applied" really break the button on that tall file input like that?

Comment 44

[Matthew Gregan [:kinetik]]

In the case where the button text is missing?  No, I'm not sure what happened there.  I can take another screenshot, but it definitely doesn't do that with my current trunk build including that patch.

Comment 45

[Matthew Gregan [:kinetik]]

Comment on attachment 289888 [details] [diff] [review]
patch v4

It turns out that the missing button text wasn't a screenshot anomaly.  It's a bug in the proposed patch where we end up drawing the button foreground but not the button text.

This patch is buggy, marking obsolete.  New patch coming soon.

Comment 46

[Matthew Gregan [:kinetik]]

Attachment: patch v5

Simpler patch.  Use the existing method nsFrame::OverflowClip to clip the nsFileFrameControl's display list to the border box size.  The display behaviour is exactly the same as in the screenshot posted for the earlier patch, except that  this patch does not suffer from the bug where the button text would not be drawn when the mouse pointer passed over it.

It would be nice to have some reftests for this, but I'm not yet sure how to do this... the fact that the native button widget changes shape (on OS X, at least) as it changes size makes it difficult to build a reftest to check the clipping behaviour of an FileInputControl with a height style vs a FileInputControl in a div with a height style and hidden overflow.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> the fact that the native button widget changes shape

"-moz-appearance: none" for the win?

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 290317 [details] [diff] [review]
patch v5

Looks reasonable

Comment 49

[Reed Loden [:reed]]

Checking in layout/forms/nsFileControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsFileControlFrame.cpp,v  <--  nsFileControlFrame.cpp
new revision: 3.221; previous revision: 3.220
done
Checking in layout/style/forms.css;
/cvsroot/mozilla/layout/style/forms.css,v  <--  forms.css
new revision: 3.142; previous revision: 3.141
done

Comment 50

[Marcia Knous [:marcia]]

verified using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007112804 Minefield/3.0b2pre and the same nightly on an Intel Mac running Tiger. No crashes running any of the testcases.

Comment 51

[Jesse Ruderman]

I checked in the first three attachments as crashtests.  Do we need some reftests for the other testcases?

Comment 52

[Boris Zbarsky [:bzbarsky]]

Ideally, yes.

Comment 53

[Matthew Gregan [:kinetik]]

(In reply to comment #47)
> "-moz-appearance: none" for the win?

I tried this, and a bunch of other things (colour styles, etc.) but couldn't find a way to disable native theming for the control.  I guess part of the problem is that you can't access the child button directly because it's anonymous content.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Oh.  That was somewhat tongue-in-cheek, sorry.  I toss that into my forms.css if I really need to change the styling for some reason...

Comment 55

[Mats Palmgren (inactive)]

Crash tests was landed already:

changeset:   9174:d71ebe271a1a
user:        [e-mail]
date:        Sat Dec 15 15:45:02 2007 -0800
summary:     Add crashtests