Closed Bug 842119 Opened 7 years ago Closed 7 years ago

getSubStringLength doesn't work properly with svg.text.css-frames.enabled

Categories

(Core :: SVG, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 --- unaffected
firefox21 --- fixed
firefox-esr17 --- unaffected

People

(Reporter: longsonr, Assigned: heycam)

References

Details

(Keywords: regression, sec-high)

Attachments

(3 files)

Attached image interactive testcase
StartIndex: 0. NumChars: 1. Length:0

StartIndex: 9. NumChars: 1. Length:11.884984016418457

StartIndex: 9. NumChars: 5. Length:11.884984016418457 + various debug assertions
Blocks: svgtext
Without svg.text.css-frames.enabled I get

StartIndex: 0. NumChars: 1. Length:21.086261749267578

StartIndex: 9. NumChars: 1. Length:24.15335464477539

StartIndex: 9. NumChars: 5. Length:98.14696502685547 and no debug assertions
Yes something is quite broken there.  (I do remember trying this test out some time ago and it working, so I must have changed something -- maybe when I switched the positioning attributes to work on post-whitespace-collapsed character indexes.)

Interestingly I get very different behaviours in other browsers.  Chrome, Opera and IE all seem to not handle the bidi text well at all.  Only the old Firefox behaviour seems reasonable.
Attached image simpler test
Here's a simpler test that Chrome and IE get right.  Opera is doing something strange still that I don't understand.

What I guess it shows is that the character indices you pass in to getSubStringLength need to be post-whitespace-collapsed indices, i.e. the same as you use to determine which DOM characters the x="", y="", etc. positioning attribute values correspond to.  The spec says clearly that getNumberOfChars() should return a value only looking at the DOM (so it's like textElement.textContent.length) but it doesn't say how the charnum/nchars parameters of the other methods are interpreted.

All browsers seem to return the number of post-whitespace-collapsed characters from getNumberOfChars(), though, so I think we should just consider the spec to be wrong there and have all of these methods work on the same indices.
Clicking the first "button" in that testcase 5 times or so makes me crash with 100% reproducibility. Crash ids 00ac96d2-12a3-44ae-98d3-f74ca2130217 and 9f33b767-5a59-4de5-81c8-3ae682130217
Attached patch patchSplinter Review
This should make all of the SVG DOM text methods use addressable character indices instead of actual DOM indices (what I've sometimes named "text element char indices" in the code):

* I've replaced CharIterator's eNonSkipped filtering mode with an eAddressable mode, since that's what we want to traverse over for these methods.  I added a few helper functions for advancing along the CharIterator, and gave it the ability to have a subtree for which it tracks whether we're before/within/after, like the other iterator classes have.

* The fix in TextRenderedRunIterator::Next() is to make mTextElementCharIndex on the returned TextRenderedRun correct when there is some trimmed white space as in:

    <text>  abc</text>

where previously this was giving a rendered run with mTextElementCharIndex == 0, mTextFrameContent{Offset,Length} == {2,3} instead of mTextElementCharIndex == 2.  (mTextElementCharIndex should be the DOM character index for the start of the rendered run ("abc" here).)


There are still some problems with GetCharNumAtPosition not returning the right values, which I'll deal with in another bug.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #715037 - Flags: review?(longsonr)
(In reply to Jonas Sicking (:sicking) from comment #4)
> Clicking the first "button" in that testcase 5 times or so makes me crash
> with 100% reproducibility. Crash ids 00ac96d2-12a3-44ae-98d3-f74ca2130217
> and 9f33b767-5a59-4de5-81c8-3ae682130217

I think thats an array out of bounds access. Would be sec-critical but we have a preffed off feature so we're down to sec-high
Group: core-security
Keywords: sec-high
Comment on attachment 715037 [details] [diff] [review]
patch

Looks like you won't need security approval if you can land this before the CSS Text Frames bug gets onto Aurora.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Attachment #715037 - Flags: review?(longsonr) → review+
I found some problems with some reftests; I'll attach a followup patch to this bug tomorrow and get that reviewed before landing the one you just r+ed.
If you are sue the problems are only in the tests then land the code. It's probably best to land the tests afterwards anyway given that this is a security bug now.
s/sue/sure/
I think it's code, as it's causing a crash in some different tests.
If you'd like to neutralise the crash before I write the followup patch tomorrow, you could comment out the guts of nsSVGTextFrame2::GetSubStringLength and just return 0 for now, and disable layout/svg/text/textpath.svg and .../textpath-selection.svg, which are the two pref-forced-on tests that currently use that function.
I don't think it's that critical since this is a preffed off feature.
Turns out I was running the tests last night in the wrong repo.  The patch doesn't cause any further crashes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c36791017011
https://hg.mozilla.org/mozilla-central/rev/c36791017011
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Group: core-security
Keywords: regression
You need to log in before you can comment on or make changes to this bug.