Closed Bug 383887 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect, P2)

x86
macOS
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, hang, testcase, Whiteboard: [dbaron-1.9:Rs])

Attachments

(2 files, 2 obsolete files)

Attached file 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.
Severity: normal → critical
Keywords: hang
Summary: "ASSERTION: unexpected frame count" with float, percentage padding, <select>, table → "ASSERTION: unexpected frame count" and hang with float, percentage padding, <select>, table
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
Attachment #273726 - Flags: review?(dbaron)
Could you put the guts in a helper function rather than duplicating the code?
That said, Boris should probably review this.
Attached patch Patch v2Splinter Review
I'm not sure if this is really the best approach, but it seems correct to me.
Attachment #273726 - Attachment is obsolete: true
Attachment #273802 - Flags: review?(bzbarsky)
Attachment #273726 - Flags: review?(dbaron)
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?
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.
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 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?
Attachment #273802 - Flags: review?(bzbarsky) → review-
I think there's an exploitable crash lurking around here.
Flags: blocking1.9?
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.
Blocks: 375980
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:Rs]
Priority: P3 → P2
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.
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.
Assignee: sharparrow1 → nobody
Status: ASSIGNED → NEW
Priority: P2 → P5
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.
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Priority: P5 → P2
Crashtest checked in, even though it currently hits that other assertion.
Flags: in-testsuite+
(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
(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)
OK, let's file a new non-blocking bug on that.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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 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.
Attachment #293618 - Attachment is obsolete: true
oops
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.