Closed Bug 879628 Opened 9 years ago Closed 9 years ago

Ordered list markers of nested ordered lists corrupted when using generated content

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Version:
21 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: lang.support, Assigned: bzbarsky)

Details

Attachments

(4 files)

Screenshot of Firefox rendering
238.31 KB, image/png
Details
How content should render
179.61 KB, image/png
Details
Testcase html page
884 bytes, text/html
Details
Don't screw up the text from earlier counters when using counters() with various list styles.
3.86 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130512194445

Steps to reproduce:

Viewed the following content in firefox:

<!DOCTYPE html>
<html lang="th">
<head>
    <meta charset="utf-8" />
    <title>Thai numbering test page</title>
    <style type="text/css">
        ol { counter-reset: item; }
        li { display: block; }
        li:before {
            content: counters(item, ".", -moz-thai);
            content: counters(item, ".", thai);
            counter-increment: item;
            padding-right: 1.0em;
        }
    </style>
</head>
<body>

<ol>
  <li> รายการ ๑
    <ol>
      <li> รายการ ๑.๑
        <ol>
          <li> รายการ ๑.๑.๑
            <ol>
              <li> รายการ ๑.๑.๑.๑
              <li> รายการ ๑.๑.๑.๒
            </ol>
          <li> รายการ ๑.๑.๒
        </ol>
      <li> รายการ ๑.๒
    </ol>
  <li> รายการ ๒
</ol>

</body>
</html>


Actual results:

List markers in first tier list display correctly. Second and subsequent tiers are corrupted displaying mojibake.

Similar problem exists with -moz-lao, -moz-maynar list types


Expected results:

list markers should be identical to numbers listed in each list item, and should display as rendered in chromes/webkit.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Attached file Testcase html page — 
Thank you for the excellent testcase!
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → NEW
Component: CSS Parsing and Computation → Layout: Block and Inline
Ever confirmed: true
Whiteboard: [need review]
This looks good; there's one thing I don't understand, though -- how would 'result' be nonempty (and 'offset' be nonzero)?

From skimming this file, it looks like this string will always have been passed (unmodified) by nsBulletFrame::AppendCounterText,
...which received it unmodified from nsBulletFrame::GetListItemText,
...which received it from either nsBulletFrame::GetDesiredSize or from nsBulletFrame::PaintBullet,
...which both declare it as "nsAutoString text;" and don't touch it before their (one) call to GetListItemText.
The relevant caller of nsBulletFrame::AppendCounterText here is in nsCounterUseNode::GetText, and it will use the same string over and over as it loops over the counters for a counters().
Um, what ensures that these functions increase the length of the string's buffer, or that they set the string's length data correctly?  I think they both have an exploitable buffer overrun, unless I'm missing something.
I'm not sure I follow...  OtherDecimalToText and TamilToText work by appending the number in decimal to the string and then going through and adding a fixed number to each codepoint value in the result.  This operation doesn't change the length of the string; that's changed by the DecimalToText call.

I guess you could get into trouble in OtherDecimalToText if ordinal < 0 and the AppendASCII in DecimalToText fails to realloc the buffer, but I thought we made strings infallible now.
I don't follow either;  I was going to grant r+, but I'll hold off on that until we better understand what dbaron had in mind with the scenario in comment 7, and how that impacts what this patch should do.
Sorry, never mind; I was misreading.
Comment 7 is private: false
Comment 8 is private: false
Comment 9 is private: false
Attachment #758650 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d27cb123e9de
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/d27cb123e9de
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.