513110 - "ASSERTION: invalid break type" with <br style="clear:both">, large negative word-spacing

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: no page breaks yet: 'NS_STYLE_CLEAR_PAGE != breakType', file /Users/jruderman/central/layout/generic/nsBlockFrame.cpp, line 3796

Comment 1

[Jesse Ruderman]

Still happens on trunk after
http://hg.mozilla.org/mozilla-central/rev/14929e61959d

> -    NS_ASSERTION(NS_STYLE_CLEAR_PAGE != breakType, "no page breaks yet");
> +    NS_ASSERTION(NS_STYLE_CLEAR_LAST_VALUE >= breakType, "invalid break type");

###!!! ASSERTION: invalid break type: 'NS_STYLE_CLEAR_LAST_VALUE >= breakType', file layout/generic/nsBlockFrame.cpp, line 3710

Comment 2

[Jesse Ruderman]

Attachment: testcase 2

Comment 3

[Mats Palmgren (inactive)]

For a <br clear=both> we'll add NS_STYLE_CLEAR_LEFT_AND_RIGHT to aStatus here:
http://hg.mozilla.org/mozilla-central/annotate/8abc76dedec2/layout/generic/nsBRFrame.cpp#l139
http://hg.mozilla.org/mozilla-central/annotate/8abc76dedec2/layout/style/nsStyleConsts.h#l317
then we hit this line:
http://hg.mozilla.org/mozilla-central/annotate/8abc76dedec2/layout/generic/nsLineLayout.cpp#l1030
which adds NS_STYLE_CLEAR_LINE:
http://hg.mozilla.org/mozilla-central/annotate/8abc76dedec2/layout/generic/nsIFrame.h#l292
so now the breakType value is 7.

I think all that's valid, and that the assertion condition is bogus.

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

Comment 5

[Mats Palmgren (inactive)]

Attachment: Jesse's tests as crashtests

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8384041 [details] [diff] [review]
fix

> // See nsStyleDisplay
> #define NS_STYLE_CLEAR_NONE                     0
> #define NS_STYLE_CLEAR_LEFT                     1
> #define NS_STYLE_CLEAR_RIGHT                    2
> #define NS_STYLE_CLEAR_LEFT_AND_RIGHT           3
> #define NS_STYLE_CLEAR_LINE                     4
>-#define NS_STYLE_CLEAR_LAST_VALUE NS_STYLE_CLEAR_LINE
>+// @note NS_STYLE_CLEAR_LINE can be added to one of the other values in layout
>+// so it needs to use a bit value that none of the other values can have.
>+#define NS_STYLE_CLEAR_MAX (NS_STYLE_CLEAR_LINE | NS_STYLE_CLEAR_LEFT_AND_RIGHT)

So these are really flags in a bitfield, and the specific constant values have significance rather than just being incrementing numbers.

Given that, I wonder if it's in fact better to define it as explicit individual bits, with LEFT_AND_RIGHT more clearly being a bitfield:

> #define NS_STYLE_CLEAR_LEFT                     (1 << 0)
> #define NS_STYLE_CLEAR_RIGHT                    (1 << 1)
> #define NS_STYLE_CLEAR_LINE                     (1 << 2)
> #define NS_STYLE_CLEAR_LEFT_AND_RIGHT (NS_STYLE_CLEAR_LEFT | NS_STYLE_CLEAR_RIGHT)

(followed by the similar _MAX one that you added)

Comment 7

[Daniel Holbert [:dholbert]]

(With that setup, I think your @note might be unnecessary)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8384041 [details] [diff] [review]
fix

Anyway, seems reasonable.

One more nit:

>Bug 513110 - The 'ASSERTION: invalid break type' condition is bogus.  r=dholbert

The commit message should describe the change, not the issue. Maybe something like "Adjust the maximum break type (only used in assertions) to reflect reality"?

r=me

Comment 9

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> So these are really flags in a bitfield, and the specific constant values
> have significance rather than just being incrementing numbers.

No, it might appear that way, but that's purely a coincidence.
It's an ordinary list of distinct enumeration property values.
(It's clearer in an earlier version before we removed some unused values:
http://hg.mozilla.org/mozilla-central/diff/14929e61959d/layout/base/nsStyleConsts.h )

The NS_STYLE_CLEAR_LINE is a misnomer though since it's NOT a property
value - it's a bit that's only used in some reflow state or'ed together
with the actual property value.  Having it here together with the list
of property values is convenient though so we don't forget to maintain
that its value use a bit that the property value can't have.

(In reply to Daniel Holbert [:dholbert] from comment #8)
> The commit message should describe the change, not the issue. Maybe
> something like "Adjust the maximum break type (only used in assertions) to
> reflect reality"?

Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7694cf6e7fb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe54c837281

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7694cf6e7fb4
https://hg.mozilla.org/mozilla-central/rev/cbe54c837281