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

RESOLVED FIXED in mozilla24

Status

()

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Status:
RESOLVED FIXED
Opened:
6 years ago
Updated:
6 years ago

People

(Reporter: lang.support, Assigned: bzbarsky)

Tracking

Version:
21 Branch
Target:
mozilla24
Platform:
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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
Posted 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
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: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.