64632 - hr height < 0 - crash in printing, assert on display

Status: VERIFIED DUPLICATE of bug 71210

(Core :: Layout, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

FreeBSD 4.1 20010105xx

Fresh pull; went to this site, registered (so it shows a list of digital
stations (sets a cookie so you don't need to register again), then clicked on
the print button.  Requester came up, told it to print, boom.

The bug appears to be in nsHRFrame.cpp line 210:

    thickness = aReflowState.mComputedHeight;
    // fix up thickness based on noshade and mode.  see bug 53568 and 54980
    if (eCompatibility_NavQuirks == mode)
    {
      nscoord adjustment =  aReflowState.mComputedBorderPadding.top + 
                            aReflowState.mComputedBorderPadding.bottom; 
      thickness += adjustment;  // adjust for -moz-bg-inset 
      PRBool noShadeAttribute = GetNoShade();
      if (thickness != onePixel)
      {
        if (!noShadeAttribute) { // this makes us compatible with Nav4, and one
pixel taller than IE5
          thickness += onePixel;
        }
      }
    }

In this case, the thickness is -14; which is apparently from the moz-bg-inset.
mode is eCompatibility_Standard, which is why this doesn't get corrected.  In
any case, however, the size should not be negative!  I think there's a problem
with moz-bg-inset or it's use for HR's, or perhaps there shouldn't be a mode
test.  See the bugs referenced above (bug 53568 and bug 54980).

Here are comments from the end of 53568.  It appears my worries about it only
being compensated for in quirks mode are valid.

------- Additional Comments From Randell Jesup 2000-10-05 11:33 -------

Appears fixed on trunk FreeBSD 20001004xx

I looked at the patch - it appears to compensate for -moz-bg-inset within the HR
code.  Is that the "correct" fix, or is -moz-bg-inset inherently incorrect?
Also, it looked as if the code only compensated in quirks mode.  Is that right?

There's still a bug in rendering of noshade HR's with the endcaps, but that's
another issue.  I think someone got way too fancy with making the rounded
endcaps....

I'm REALLY glad that it got fixed for the PR3 release - Not fixing it would have
been asking to be pilloried.  I'm also glad I spent the time to track it through
the code.



------- Additional Comments From [e-mail] 2000-10-05 11:41 -------

Randell:  I'm glad you pushed on this one, too!
You are correct, I compensate for the inset computation inside the HR reflow
code in quirks mode.  That's what quirks mode is all about. This is to account
for Nav's bizzare sizing algorithm with HR heights, and Nav/IE's odd treatment
of HR's with respect to floaters.  Basically, legacy behavior is to treat an HR
as a block element unless it's impacted by a floater, in which case it's treated
like an inline element.  That's very tough for a CSS-compliant browser to
emulate.
This isn't necessary in standard mode at all, where HR's are block elements,
period.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Added crash, regression
Severity to major

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

FreeBSD 4.1 20010306xx re-verified bug exists (assertion on display, didn't try
crashing it on print)

Assertions (10 of them for height < 0) can be seen at http://sharkyextreme.com

###!!! ASSERTION: Negative Height Result- very bad: 'mComputedHeight>=0', file
nsHTMLReflowState.cpp, line 2444

(height was -14 again, due to moz-bg-inset).  Note that this page is NOT marked
as standard (though it does use some simple styles).

OS->All
Severity->critical (it's a crasher)
Changed subject

Related issue: on Linux ns4.7, shaded (normal) HR's are all 1 pixel too tall.  I
suspect this code: 

if (!noShadeAttribute) { // this makes us compatible with Nav4, and one pixel
taller than IE5
    thickness += onePixel;

We're one pixel taller than ns4.7 (Linux).  I suspect we're one pixel taller
than ns 4.x and ie5 on all platforms, and that this if should go away.

Again, I suggest that we remove this test:
    // fix up thickness based on noshade and mode.  see bug 53568 and 54980
    if (eCompatibility_NavQuirks == mode)

I think that the border thickness (moz-bg-inset) needs to be compensated for in
all cases.

Comment 3

[karnaze (gone)]

reassigning to attinasi and marking m0.9.

Comment 4

[Chris Petersen]

Reassigning owner

Comment 5

[Marc Attinasi]

I think that bug 71210 covers the remaining issue here (the assert, which has
been commented out for now since the layout code now handels it correctly).
There is no longer a crash printing AFAICT. Please re-open is you are still
seeing the crash on a current build - thanks!

*** This bug has been marked as a duplicate of 71210 ***

Comment 6

[James Green]

Verified dup.