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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: 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

Comment 1

[Robert Longson [:longsonr]]

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

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch

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.

Comment 6

[Robert Longson [: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

Comment 7

[Robert Longson [:longsonr]]

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

Comment 8

[Cameron McCormack (:heycam)]

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.

Comment 9

[Robert Longson [:longsonr]]

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.

Comment 10

[Robert Longson [:longsonr]]

s/sue/sure/

Comment 11

[Cameron McCormack (:heycam)]

I think it's code, as it's causing a crash in some different tests.

Comment 12

[Cameron McCormack (:heycam)]

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.

Comment 13

[Robert Longson [:longsonr]]

I don't think it's that critical since this is a preffed off feature.

Comment 14

[Cameron McCormack (:heycam)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c36791017011