Bug 18830 – Assertion failure (using HashMap empty value) in svg/W3C-SVG-1.1/fonts-glyph-02-t.svg

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

Opening svg/W3C-SVG-1.1/fonts-glyph-02-t.svg in a TOT debug build causes
ASSERTION FAILED: !HashTranslator::equal(KeyTraits::emptyValue(), key)

The problem is that SVGRootInlineBox::buildLayoutInformationForTextBox() calls calculateGlyphWidth() in the RTL case passing and offset and a number of extra characters going past the end of the run. The whole approach of taking logical suffixes -- rather than prefixes -- of RTL text is wrong. It was already fixed in one place but I am not sure what the correct fix is in t his case.

[Robert Blaut]

I don't know why a copy of the test in WebKit trunk are different from the copy available on w3.org.

Compare: 
http://trac.webkit.org/browser/trunk/LayoutTests/svg/W3C-SVG-1.1/fonts-glyph-02-t.svg?rev=28763&format=raw
http://www.w3.org/Graphics/SVG/Test/20061213/svggen/fonts-glyph-02-t.svg

Both have the same revision 1.7.

[Robert Blaut]

*** Bug 19574 has been marked as a duplicate of this bug. ***

[Mark Rowe (bdash)]

*** Bug 20950 has been marked as a duplicate of this bug. ***

[Mark Rowe (bdash)]

Bug 20950 was about this same assertion failure when running svg/custom/acid3-test-77.html in a debug build.  Our debug build bots are hitting this repeatedly, and it would be nice to get the bots into a better state.

[Alexey Proskuryakov]

*** Bug 22321 has been marked as a duplicate of this bug. ***

[Holger Freyther]

Created attachment 25224 [details]
Fix the calculation...

This patch makes the out of bounds string copying go away which is fixing the assertion failure as well.

I doubt it is the correct fix for RTL. Maybe we need to iterate the other way around on the string as well? Currently in the first iteration no extra chars are present/taken into account. But at least I think I have pin pointed the cause of this crash.

[Holger Freyther]

Oops, I only saw mitz comment right now. I have little knowledge of RTL but would be willing to take this bug but would need some guidance. 

[Eric Seidel (no email)]

Comment on attachment 25224 [details]
Fix the calculation...

I don't understand this change.  Maybe mitz would.  It looks right, but I think we could make this clearer using some local variable with clearer names.  Why does all this need to be manual calculation?  Don't we have TextIterators or similar for this kind of thing?

Here I'm going to be a hypocrit and mention there is not ChangeLog in this patch.  :(  I know how much ChangeLogs suck with git... 

My approach here would be to start by cleaning up the code to the point where I understood what it was doing... and then I would fix the bug.

[Holger Freyther]

(In reply to comment #9)
> (From update of attachment 25224 [details] [review])

> My approach here would be to start by cleaning up the code to the point where I
> understood what it was doing...

Well, it is pretty easy. Just do a git/svn blame on the code, you will see Maciej's change for acid3 and you will recognize what is wrong with the whole approach.

1.) One should fix mem corruption as this will crash in the field
2.) As Mitz pointed out on IRC the whole SVG Font stuff should walk the text in logical order and not visual one...but this is a bigger task


This patch is only fixing the crash by making sure that the string is not accessed out of bounds. I can and should write a changelog entry though.

[Holger Freyther]

Created attachment 25524 [details]
Fix the calculation for RTL text to stop reading random memory

Same patch, using the svg test again. Regarding correctness: I'm pretty sure that we stop constructing strings from random parts of memory.
I'm also aware that the SVG text handling needs to be changed to process the text in logical order to be correct and spec complaint.

So this patch will stop an assert, a real world crash, will fix a test case. On the other hand it will not be the last thing that is going to happen in regard to RTL text handling of SVG. So what is our take on real world crashes and fixes for them?

[Darin Adler]

Comment on attachment 25524 [details]
Fix the calculation for RTL text to stop reading random memory

> +            const int extraCharsAvailable = length - textBox->end() + i - 1;

We don't use const for local variables in cases like this, and I don't think it's good to start using it in some places but noth others.

r=me on this change despite all the caveats in the change log

[Holger Freyther]

(In reply to comment #12)

> r=me on this change despite all the caveats in the change log

Let us please talk about it/fix it.

[Alexey Proskuryakov]

Comment on attachment 25524 [details]
Fix the calculation for RTL text to stop reading random memory

Clearing the review flag to get this out of commit queue due to above comment.

[Alexey Proskuryakov]

This doesn't happen any more. Re-enabled the test in r60983.