484953 - Make CharacterIterator more robust

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Followup from bug 483439. We should make CharacterIterator in http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGGlyphFrame.cpp more robust. It should not read past the end of the buffer, and if someone tries to make it, it should assert.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

More performant if we have both a fill and a stroke on text. Includes crashtest from dependent bug. If we break the CharacterIterator expectations we should assert now rather than crash with some bad memory access.

Comment 3

[Robert Longson [:longsonr]]

Attachment: without the extra bit from some other bug this time

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   * Resets the iterator to the beginning of the string.
+   */
+  void Reset() {
+    if (mCurrentChar != -1) {
+      mCurrentChar = -1;
+      mInError = PR_FALSE;
+    }
+  }

Why not just do the assignments unconditionally?

diff --git a/layout/svg/crashtests/605626-1.svg b/layout/svg/crashtests/605626-1.svg
new file mode 100644
--- /dev/null
+++ b/layout/svg/crashtests/605626-1.svg
@@ -0,0 +1,3 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+<image width="10" height="10" xlink:href="data:text/plain,g"/>
+</svg>

You've still got this extra bit!

Is there another bug that 483439-1.svg triggers that causes CharacterIterator to be misused? It seems like there must be; is that filed?

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #4)
> +   * Resets the iterator to the beginning of the string.
> +   */
> +  void Reset() {
> +    if (mCurrentChar != -1) {
> +      mCurrentChar = -1;
> +      mInError = PR_FALSE;
> +    }
> +  }
> 
> Why not just do the assignments unconditionally?

There are two ways that mInError can be set

a) Something goes wrong initially with the CharacterIterator e.g. the initial matrix is singular or EnsureTextRun fails.

b) Everything is OK initially but we run off the end of the text so we can't read any more characters.

These can be distinguished by a) mCurrentChar = -1, b mCurrentChar != -1

In the case of a) we don't want Reset to clear the mInError flag as the CharacterIterator is still unusable. For b) we do as going back to the beginning fixes it. Do you want some more comments in the code?

> 
> You've still got this extra bit!

Oops, I can remove that.

> 
> Is there another bug that 483439-1.svg triggers that causes CharacterIterator
> to be misused? It seems like there must be; is that filed?

No, that was the only one. It comes from bug 483439 comment 18. That's the only function that iterates twice through the text.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, so this only defends against bugs we don't know about?

> Do you want some more comments in the code?

yes please!

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #6)
> OK, so this only defends against bugs we don't know about?

Currently yes, but it is faster too :-) We do want at some point to make the region stuff better wwhich would mean iterating twice but we'd need a cairo API change for that and they don't seem keen. There's a huge jwatt comment in the code about that already.

> 
> > Do you want some more comments in the code?
> 
> yes please!

Will do.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 9

[Robert Longson [:longsonr]]

Attachment: hg changeset patch

Comment 10

[Daniel Holbert [:dholbert]]

I've got this in my bundle of patches to land post-2.7-freeze.

Comment 11

[Daniel Holbert [:dholbert]]

er, s/2.7/2.0b7/ (sorry for bugspam)

Comment 12

[Daniel Holbert [:dholbert]]

Landed (in a push with the other changesets all being few-line-fixes, non-SVG-related, & non-perf-sensitive, since this is a perf-sensitive change):
http://hg.mozilla.org/mozilla-central/rev/2e2ab9de2222