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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

(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.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: patch, refactor gfxFont::InitTextRun so that Linux code also benefits from splitting of long runs

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.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch 2, remove CopyItemSplitOversize etc from the Uniscribe backend, as gfxFont::SplitAndInitTextRun protects it from excessively long strings

Comment 3

[Karl Tomlinson (:karlt)]

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.

Comment 4

[Karl Tomlinson (:karlt)]

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 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[Jonathan Kew [:jfkthame]]

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.

Comment 7

[Jonathan Kew [:jfkthame]]

Pushed parts 1 & 2:
http://hg.mozilla.org/mozilla-central/rev/d8c5d2150fc7
http://hg.mozilla.org/mozilla-central/rev/4dd9e1b6d029