Closed Bug 850655 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: longsonr, Assigned: heycam)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Blocks: svgtext
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)
Summary: Consider making nsTextFrame2::CharIterator::Next more robust → Consider making nsSVGTextFrame2's CharIterator::Next more robust
Summary: Consider making nsSVGTextFrame2's CharIterator::Next more robust → nsSVGTextFrame2's CharIterator::Next crashes instead of returning null if you call it while you're at the end
Summary: nsSVGTextFrame2's CharIterator::Next crashes instead of returning null if you call it while you're at the end → nsSVGTextFrame2's CharIterator::Next crashes instead of returning false if you call it while you're at the end
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #729395 - Flags: review?(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.
(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.
Attached patch patch (v1.1)Splinter Review
Attachment #729395 - Attachment is obsolete: true
Attachment #729395 - Flags: review?(longsonr)
Attachment #729994 - Flags: review?(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.
Attachment #729994 - Flags: review?(longsonr) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/023023d36160
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.