287837 - ###!!! ASSERTION: Unexpected line-height unit

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

Bug 281972 uncovers a couple of bugs in initial values which were previously
hidden under a "#ifdef DEBUG_ComputedDOMStyle".

STEPS TO REPRODUCE:
1. in a debug build where bug 281972 is fixed - 
   load about:blank and invoke the DOM Inspector
2. select the HTML element and then click "Computed Style" in the
   right panel and scroll down.

ACTUAL RESULTS:

###!!! ASSERTION: Unexpected line-height unit: 'Error', file
nsComputedDOMStyle.cpp, line 1689

###!!! ASSERTION: Unexpected outline radius unit: 'Error', file
nsComputedDOMStyle.cpp, line 1503

###!!! ASSERTION: Unexpected border radius unit: 'Error', file
nsComputedDOMStyle.cpp, line 3374

Comment 1

[Boris Zbarsky [:bzbarsky]]

Mats, I can't seem to reproduce this (and the initial value of line-height has
been "normal" ever since ruletree landed, as far as I can tell).  Are you still
seeing this?

Comment 2

[Peter Van der Beken [:peterv]]

I can reproduce this in a recent trunk build (pulled last week). Makes using DOM
Inspector quite painful when you have break-on-assert enabled.
When I hit the first assertion in nsComputedDOMStyle::GetLineHeight,
text->mLineHeight.mUnit is eStyleUnit_Normal so we end up in the NS_ERROR on
line 1799 of nsComputedDOMStyle.cpp
(http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/nsComputedDOMStyle.cpp&rev=1.143&root=/cvsroot&mark=1799#1776).

Comment 3

[Boris Zbarsky [:bzbarsky]]

So I sort of wonder why we're asserting here...  "normal" is a pretty reasonable
value for line-height, no?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

how about this? this uses 0 if no explicit value is specified for
outline-radius and border-radius (and handles normal for line-height).

I'm not sure, though, if that usage of 0 should be done here in nsRuleNode, or
in the cssstruct constructor...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196254 [details] [diff] [review]
patch

>Index: nsComputedDOMStyle.cpp

>       default:
>         NS_ERROR("Unexpected line-height unit");
>+      case eStyleUnit_Normal:
>         val->SetIdent(nsLayoutAtoms::normal);
>         break;

Please put that before default, ok?  No need to set to "normal" in the default
case here, since it really should be notreached with this change.

>Index: nsRuleNode.cpp

This should be done in the struct constructor, in both cases; your change, as
written, will clobber values that should not be clobbered with 0...

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

well, if I do that I get:
###!!! ASSERTION: not initial state: 'val->GetUnit() == eCSSUnit_Null', file
../../../../mozilla/layout/style/nsCSSDataBlock.cpp, line 887

Comment 7

[Boris Zbarsky [:bzbarsky]]

If you do which?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, wait.  I see.  I was talking about the _style_ struct, not the css struct. 
The css struct should default to null, the style struct should default to the
initial value, whatever that is.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

ah, ok, that makes a lot of sense. will do.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

I'm not quite sure that NS_FOR_CSS_SIDES is a good macro to use here, because
it uses PRInt32 as the variable type, while nsStyleSides uses PRUint8, but
other code in this file uses it too.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in layout/style/nsComputedDOMStyle.cpp;
/cvsroot/mozilla/layout/style/nsComputedDOMStyle.cpp,v  <--  nsComputedDOMStyle.cpp
new revision: 1.144; previous revision: 1.143
done
Checking in layout/style/nsStyleStruct.cpp;
/cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.132; previous revision: 3.131
done