839809 - make counters/lists that would go past our internal limit (int32_t) stay constant rather than wrapping

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Make counter-increments and list counting that would go past our internal (int32_t) limit keep the counter at its current value rather than wrapping.

Per CSS WG resolution regarding counter-styles-3, afternoon of 2013-02-05:
http://krijnhoetmer.nl/irc-logs/css/20130205#l-1590

Note that this patch depends on signed integer overflow behavior in C++,
which I believe is portable despite being unspecified.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 712175 [details] [diff] [review]
Make counter-increments and list counting that would go past our internal (int32_t) limit keep the counter at its current value rather than wrapping.

r=me -- just a few optional suggestions:

>+++ b/layout/base/nsCounterManager.cpp
>@@ -53,17 +53,25 @@ void nsCounterUseNode::Calc(nsCounterLis
>-        mValueAfter = aList->ValueBefore(this) + mChangeValue;
>+        int32_t valueBefore = aList->ValueBefore(this);
>+        int32_t valueAfter = valueBefore + mChangeValue;
>+        // The CSS Working Group resolved that a counter-increment that
>+        // exceeds internal limits should not increment at all.
>+        // http://krijnhoetmer.nl/irc-logs/css/20130205#l-1590
>+        if ((mChangeValue > 0) != (valueAfter > valueBefore)) {
>+          valueAfter = valueBefore;
>+        }
>+        mValueAfter = valueAfter;
[...]
>+++ b/layout/generic/nsBulletFrame.cpp
>+  int32_t nextOrdinal = mOrdinal + aIncrement;
>+
>+  // The CSS Working Group resolved that a counter-increment that
>+  // exceeds internal limits should not increment at all.
>+  // http://krijnhoetmer.nl/irc-logs/css/20130205#l-1590
>+  if ((aIncrement > 0) != (nextOrdinal > mOrdinal)) {
>+    nextOrdinal = mOrdinal;
>+  }
>+
>+  return nextOrdinal;

Rather than duplicating the comment & wraparound-checking-technique here, it might be better to separate out this code into a static helper method, (maybe on nsCounterManager or on nsLayoutUtils) -- e.g. int32_t IncrementCounterWithBoundsCheck(int32_t aCurCounterValue, int32_t aOffset).

Then both of these changes would just be one-liners.

It's also fine as-is, though.

>+++ b/layout/reftests/counters/counter-ua-limits-list-01-ref.html
>+
>+ <!--
>+    Work around our inability to parse -2147483648 as an HTML integer attribute
>+   -->

Probably worth mentioning bug 586761 in this comment. (looks like that covers this issue about parsing -2147483648)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 712175 [details] [diff] [review]
Make counter-increments and list counting that would go past our internal (int32_t) limit keep the counter at its current value rather than wrapping.

Also:
>@@ -6753,16 +6753,18 @@ nsBlockFrame::RenumberLists(nsPresContex
>     for (nsIContent* kid = mContent->GetFirstChild(); kid;
>          kid = kid->GetNextSibling()) {
>       if (kid->IsHTML(nsGkAtoms::li)) {
>+        // FIXME: This isn't right in terms of what CSS says to do for
>+        // overflow of counters.
>         ordinal -= increment;

AFAICT, we could only run into trouble here if we had a list with has more than 2^31 entries, right?  Might be worth mentioning that here, since that makes it significantly less worrisome.

Comment 4

[Daniel Holbert [:dholbert]]

er, s/has more than/has at least/

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> IncrementCounterWithBoundsCheck(int32_t aCurCounterValue, int32_t aOffset).

I plan to take this suggestion, but just call the function IncrementCounter.

Comment 6

[Daniel Holbert [:dholbert]]

Cool, that sounds good.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b968708558b9

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out for reftest failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29e4a8ae748

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Reftest failures were Mac OS X only; tests were fine on Windows, Linux, and Android if we run new reftests there by default.)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

s/module/modulo/ in the new comment.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c46476d3892a with a change to cast to uint32_t around the addition.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Typo fix followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c28fd3fd030

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/c46476d3892a
https://hg.mozilla.org/mozilla-central/rev/5c28fd3fd030