Closed Bug 617905 Opened 9 years ago Closed 9 years ago

refactor gfxFont::InitTextRun to make the splitting of long runs universally applicable

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(2 files)

(Split off from bug 569770 comment 38 and following.)

We should refactor gfxFont::InitTextRun so that the handling of huge strings (to split them into reasonable-length chunks for shaping) applies to all platforms; the virtual InitTextRun method, which may be overridden, will never be called with strings > 32K chars.

Currently, platforms that override gfxFont::InitTextRun have to provide their own handling of huge strings (or just pass them on to the shaper, and hope for the best...). With some restructuring, we can handle the splitting of long strings before calling the platform-specific overrides.

This will help to protect lower-level code in the various font-shaping backends from attacks where millions of characters are injected into the document, and then we hit OOM somewhere during the shaping process.
Based on the patch originally posted in bug 569770, but with the problematic optimization of 8-bit text removed. gfxFont::InitTextRun becomes protected, and is only called from gfxFont::SplitAndInitTextRun with moderate-length substrings.

This should make it much harder to hit situations where OOM occurs (and perhaps is not handled adequately) during text-run shaping in gfxFont::InitTextRun implementations, as we'll never attempt to shape megabytes of text in a single operation.
Assignee: nobody → jfkthame
Attachment #496485 - Flags: review?(karlt)
Attachment #496485 - Flags: review?(karlt) → review+
Comment on attachment 496486 [details] [diff] [review]
patch 2, remove CopyItemSplitOversize etc from the Uniscribe backend, as gfxFont::SplitAndInitTextRun protects it from excessively long strings

>     PRUint32 ItemsLength() {
>         return mNumItems;
>     }

This is not used (and wasn't used even before your patch).
Perhaps you can remove it while you are here.
Attachment #496486 - Flags: review?(karlt) → review+
Comment on attachment 496486 [details] [diff] [review]
patch 2, remove CopyItemSplitOversize etc from the Uniscribe backend, as gfxFont::SplitAndInitTextRun protects it from excessively long strings

>-            // See bug 394751 for details.

Can you put this bug reference on "NS_ASSERTION(mMaxGlyphs < 65535" please?
Comment on attachment 496485 [details] [diff] [review]
patch, refactor gfxFont::InitTextRun so that Linux code also benefits from splitting of long runs

Requesting approval to land this for mozilla-2.0. This does not change behavior or fix a specific bug, but improves overall robustness of our Linux font backend by splitting up huge text runs before they are passed to shaping libraries (either in-tree harfbuzz or system pango), thus making it much more difficult for a page/script to trigger OOM-related failures within the text-shaping code.
Attachment #496485 - Flags: approval2.0?
Comment on attachment 496486 [details] [diff] [review]
patch 2, remove CopyItemSplitOversize etc from the Uniscribe backend, as gfxFont::SplitAndInitTextRun protects it from excessively long strings

Requestion approval-2.0. This is a logical followup to the preceding patch, removing redundant code from the Uniscribe backend once it is implemented in the higher-level generic code.
Attachment #496486 - Flags: approval2.0?
Attachment #496485 - Flags: approval2.0? → approval2.0+
Attachment #496486 - Flags: approval2.0? → approval2.0+
Pushed parts 1 & 2:
http://hg.mozilla.org/mozilla-central/rev/d8c5d2150fc7
http://hg.mozilla.org/mozilla-central/rev/4dd9e1b6d029
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.