Closed Bug 484953 Opened 15 years ago Closed 14 years ago

Make CharacterIterator more robust

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: jwatt, Assigned: longsonr)

References

()

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #487228 - Flags: review?(roc)
Attachment #487228 - Flags: approval2.0?
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.
Keywords: perf
Attachment #487228 - Attachment is obsolete: true
Attachment #487229 - Flags: review?(roc)
Attachment #487229 - Flags: approval2.0?
Attachment #487228 - Flags: review?(roc)
Attachment #487228 - Flags: approval2.0?
+   * 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?
(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.
OK, so this only defends against bugs we don't know about?

> Do you want some more comments in the code?

yes please!
(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.
Attachment #487229 - Flags: review?(roc)
Attachment #487229 - Flags: approval2.0?
Attached patch address review comments (obsolete) — Splinter Review
Attachment #487229 - Attachment is obsolete: true
Attachment #487428 - Flags: review?(roc)
Attachment #487428 - Flags: approval2.0?
Attachment #487428 - Flags: review?(roc)
Attachment #487428 - Flags: review+
Attachment #487428 - Flags: approval2.0?
Attachment #487428 - Flags: approval2.0+
Attachment #487428 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [c-n: after 2.0b7 freeze]
I've got this in my bundle of patches to land post-2.7-freeze.
Version: unspecified → Trunk
er, s/2.7/2.0b7/ (sorry for bugspam)
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
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: after 2.0b7 freeze]
Target Milestone: --- → mozilla2.0b8
Depends on: 611975
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: