Closed Bug 513110 Opened 11 years ago Closed 6 years ago

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

Categories

(Core :: Layout: Floats, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file testcase
###!!! ASSERTION: no page breaks yet: 'NS_STYLE_CLEAR_PAGE != breakType', file /Users/jruderman/central/layout/generic/nsBlockFrame.cpp, line 3796
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
Summary: "ASSERTION: no page breaks yet" with <br style="clear:both">, large negative word-spacing → "ASSERTION: invalid break type" with <br style="clear:both">, large negative word-spacing
Attached file testcase 2
Attached patch fixSplinter Review
Assignee: nobody → matspal
Status: NEW → ASSIGNED
Attachment #8384041 - Flags: review?(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)
(With that setup, I think your @note might be unnecessary)
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
Attachment #8384041 - Flags: review?(dholbert) → review+
(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
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/7694cf6e7fb4
https://hg.mozilla.org/mozilla-central/rev/cbe54c837281
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.