383887 - "ASSERTION: unexpected frame count" and hang with float, percentage padding, <select>, table

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

About twenty assertions about "bad width" and/or "NS_UNCONSTRAINEDSIZE".

###!!! ASSERTION: unexpected frame count: 'aBand->mNumFrames > 1', file /Users/jruderman/trunk/mozilla/layout/generic/nsSpaceManager.cpp, line 301

Hang in nsSpaceManager::GetBandAvailableSpace

Might be related to bug 381074.

Comment 1

[Eli Friedman]

Attachment: Patch

This patch makes a couple of implementations of GetMinWidth sane.  (It is never correct to use the results of a container's pref width method unchecked from a min width method: NS_UNCONSTRAINEDSIZE is a legal pref width but is not a legal min width.)

This doesn't really make it correct, but sane results are probably a higher priority than correct results.

I suppose another possibility would be to special-case the unconstrained width case... I don't know how much better the results would be, though.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you put the guts in a helper function rather than duplicating the code?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, Boris should probably review this.

Comment 4

[Eli Friedman]

Attachment: Patch v2

I'm not sure if this is really the best approach, but it seems correct to me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This patch is wrong.  We really do want the min width to be the same as the pref width in all cases where is makes sense for comboboxes and listboxes.  I don't see how this patch accomplishes that.

What sorts of sizing testing have you done with this?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note in particular that unless <option> has "white-space: nowrap" or some such (which I doubt it does right now) the sizing will be different with this patch because the min-width of a combobox dropdown will be the longest word in an option, whereas the pref width is the longest option.

Comment 7

[Eli Friedman]

Hmm, it's kind of tricky.  It looks like options do in fact have white-space: nowrap, as I haven't managed to get one to wrap.  There seems to be an incremental reflow issue somewhere, though; selects don't seem to be responding to resizing correctly.  The bigger issue here is that the min-width of the popup is apparently always zero.  Not sure what to do about that.

I suppose we could use the pref width for the min width when it's sane; what do we do when it's not sane, though?

It looks like other browsers don't factor percentages into the pref width for selects.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273802 [details] [diff] [review]
Patch v2

white-space inherits. The select is nowrap, but the page could just style its options to some other value...

And yes, the problem is that min-width on scrollframes is not useful for what we want here.

Perhaps we should manually set white-space on all the stuff here in forms.css (with !important) and then manually dig around inside the scrollframe for the thing to really call GetMinWidth() on?  But then percentage paddings or whatnot on options will just cause us to render weirdly, right?

Comment 9

[Jesse Ruderman]

I think there's an exploitable crash lurking around here.

Comment 10

[Eli Friedman]

A crash is possible.

Nevermind about the whole thing about the select... that doesn't really fix this hang.  You can end up with equally fun results with a simple style rule like:
select {width: 17895697.066666666666666666666667px}.  That has the added benefit of triggering all sorts of great assertions about NS_UNCONSTRAINEDSIZE in unexpected places :(

Apparently, we're ending up with a \float at (PR_INT32_MIN, PR_INT32_MIN), and this makes the spacemanager unhappy somehow.  I'll look at it in a bit more detail.  I doubt the spacemanager problem could cause crashes, though.

Comment 11

[Jesse Ruderman]

Eli, are you going to be able to get to this soon?  If not, please reassign to nobody@ and we'll try to find a new owner.

Comment 12

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

On trunk, I only get
###!!! ASSERTION: nscoord addition will reach or pass nscoord_MAX: '(PRInt64)a + (PRInt64)b < (PRInt64)nscoord_MAX', file ../../dist/include/gfx/nsCoord.h, line 153
WARNING: empty langgroup: file /Users/roc/mozilla-checkin/mozilla/gfx/thebes/src/gfxFont.cpp, line 875
now, probably due to other bugs being fixed. No hang and no potential for memory corruption that I can see.

Downgrading priority.

Comment 13

[Boris Zbarsky [:bzbarsky]]

That assert is coming from NSCoordSaturatingAdd (which seems to handle this case correctly, by the way).

Most of the issues here likely went away with bug 363858.

Comment 14

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

Hmm, that assertion should actually be a warning, since we do handle it correctly. Daniel, can you fix that?

I'll mark this fixed anyway.

Comment 15

[Jesse Ruderman]

Crashtest checked in, even though it currently hits that other assertion.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> Hmm, that assertion should actually be a warning, since we do handle it
> correctly. Daniel, can you fix that?

Sure.  See bug 408754

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> On trunk, I only get
> ###!!! ASSERTION: nscoord addition will reach or pass nscoord_MAX: '(PRInt64)a
> + (PRInt64)b < (PRInt64)nscoord_MAX', file ../../dist/include/gfx/nsCoord.h,
> line 153

In response to Jesse's question in https://bugzilla.mozilla.org/show_bug.cgi?id=408754#c2 , I GDB'd through this bug's testcase a bit to find out why we're triggering this assertion in the first place.  Looks like it's a nscoord_MAX arithmetic problem in nsComboboxControlFrame.cpp. (A place where we should probably be using NSCoordSaturatingSubtract.)

The responsible line is:
  http://mxr.mozilla.org/seamonkey/source/layout/forms/nsComboboxControlFrame.cpp#594

 564 nscoord
 565 nsComboboxControlFrame::GetIntrinsicWidth(nsIRenderingContext* aRenderingContext,
 566                                           nsLayoutUtils::IntrinsicWidthType aType)
 567 {
 ...
 594     dropdownContentWidth -= scrollbarWidth;
 
Here, dropdownContentWidth is nscoord_MAX, and scrollbarWidth is 900.  So we end up returning that the nsComboboxControlFrame's pref-width is [nscoord_MAX - 900].

Later, this value gets passed into NSCoordSaturatingAdd -- we add 1500 to it, which would put us over nscoord_MAX.  And so the warning is triggered.  (But as said above, we cap the arithmetic at nscoord_MAX, so nothing bad happens)

Comment 18

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

OK, let's file a new non-blocking bug on that.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: Tiny patch for nscoord_MAX arithmetic issue

This tiny patch addresses the nscoord_MAX arithmetic issue in nsComboBoxControlFrame, described in my previous comment.

I can also file a separate bug for this patch, if that's cleaner.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> OK, let's file a new non-blocking bug on that.

oops -- missed that comment.  You were too quick for me. :)  Ok, I'll file a new bug.

Also, did you mean to reopen this bug?  My bugmail shows it being reopened on Comment 18.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 293618 [details] [diff] [review]
Tiny patch for nscoord_MAX arithmetic issue

Obsoleting this tiny patch attachment.  

I just filed bug 408772 for this nscoord_MAX arithmetic issue, and I re-posted the patch there.

Comment 22

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

oops

Comment 23

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre - no assertion on the testcase

--> Verified fixed