Make CharacterIterator more robust

RESOLVED FIXED in mozilla2.0b8

Status

()

RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: jwatt, Assigned: longsonr)

Tracking

({perf})

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 487228 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #487228 - Flags: review?(roc)
Attachment #487228 - Flags: approval2.0?
(Assignee)

Comment 2

8 years ago
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.
(Assignee)

Updated

8 years ago
Keywords: perf
(Assignee)

Comment 3

8 years ago
Created attachment 487229 [details] [diff] [review]
without the extra bit from some other bug this time
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?
(Assignee)

Comment 5

8 years ago
(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!
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #487229 - Flags: review?(roc)
Attachment #487229 - Flags: approval2.0?
(Assignee)

Comment 8

8 years ago
Created attachment 487428 [details] [diff] [review]
address review comments
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+
(Assignee)

Comment 9

8 years ago
Created attachment 487525 [details] [diff] [review]
hg changeset patch
Attachment #487428 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: after 2.0b7 freeze]
Target Milestone: --- → mozilla2.0b8
Depends on: 611975
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.