850655 - nsSVGTextFrame2's CharIterator::Next crashes instead of returning false if you call it while you're at the end

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

See bug 849688 comment 5

Comment 1

[Daniel Holbert [:dholbert]]

The relevant parts of that comment:

Next() doesn't hold up to its documentation

It's currently documented as follows:
> 1987 class CharIterator
[...]
> 2031   /**
> 2032    * Advances to the next matching character.  Returns true if there was a
> 2033    * character to advance to, and false otherwise.
> 2034    */
> 2035   bool Next();
> 2036 
> 2037   /**
> 2038    * Advances ahead aCount matching characters.  Returns true if there were
> 2039    * enough characters to advance past, and false otherwise.
> 2040    */
> 2041   bool Next(uint32_t aCount);
https://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGTextFrame2.cpp#2031

Both versions say they return false if there aren't enough characters to advance to; however, it actually crashes if we're at the end, at least under some circumstances.  (as was the case in bug 849688, where we had to work around this by checking AtEnd() before calling Next)

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 3

[Robert Longson [:longsonr]]

You should remove all the AtEnd calls that I added in bug 720239.

Also in nsSVGTextFrame2::GetSubStringLength we should probably either 

a) just call stuff we know will work e.g.

   if (!chit.AdvanceToSubtree() ||
       !chit.Next(charnum) ||
       chit.AtEnd()) {
       chit.IsAfterSubtree()) {
     return 0.0f;
   }

becomes

    chit.AdvanceToSubtree();
    chit.Next(charnum);

because we checked the length is ok in SVGTextContainerElement already didn't we?

or alternatively make SVGTextContentElement::GetSubStringLength return an nsresult and take a reference or float pointer which would then allow the removal of the textFrame->GetNumberOfChars(this) call in SVGTextContentElement::GetSubStringLength. This would be more consistent with the other DOM methods that already return an nsresult.

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #3)
> You should remove all the AtEnd calls that I added in bug 720239.

(Bug 849688 I think.)

I think your second suggestion of making nsSVGTextFrame2::GetSubStringLength() etc. be able to report the problem with the charnum/nchars arguments is a good one.  Apart from the consistency, we are also iterating over the characters twice.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch (v1.1)

Comment 6

[Robert Longson [:longsonr]]

In CharIterator::Next(uint32_t aCount), if aCount = 0 then we return true and don't call CharIterator::Next() at all. The DOM methods would now crash if they were called with <text><text> and aCount = 0 I think.

CharIterator::Next(uint32_t aCount) should probably do this at the beginning

    if (aCount == 0 && AtEnd()) {
      return false;
    }

Also the 

+
+  if (nchars == 0) {
+    *aResult = 0.0f;
+    return NS_OK;
+  }
+

in nsSVGTextFrame2::GetSubStringLength should either move to the top of that method for performance or alternatively the check for nchars == 0 in SVGTextContentElement could stay where it is rather than being removed.

r=me with those 2 things addressed.

Comment 7

[Robert Longson [:longsonr]]

Oops, forget everything after

Also the 

that second observation is a rubbish review comment ;-). We need to check that we're not outside the text bounds and fail before checking nchars == 0. Your original change is quite right.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #6)
> CharIterator::Next(uint32_t aCount) should probably do this at the beginning
> 
>     if (aCount == 0 && AtEnd()) {
>       return false;
>     }

Good catch.

Comment 9

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/023023d36160

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/023023d36160