Bug 18830 - Assertion failure (using HashMap empty value) in svg/W3C-SVG-1.1/fonts-glyph-02-t.svg
Summary: Assertion failure (using HashMap empty value) in svg/W3C-SVG-1.1/fonts-glyph-...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: LayoutTestFailure
: 19574 22321 (view as bug list)
Depends on:
Blocks: 19569
  Show dependency treegraph
 
Reported: 2008-05-01 12:41 PDT by mitz
Modified: 2010-06-10 17:46 PDT (History)
5 users (show)

See Also:


Attachments
Fix the calculation... (1.38 KB, patch)
2008-11-17 13:36 PST, Holger Freyther
eric: review-
Details | Formatted Diff | Diff
Fix the calculation for RTL text to stop reading random memory (18.60 KB, patch)
2008-11-26 11:39 PST, Holger Freyther
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2008-05-01 12:41:10 PDT
Opening svg/W3C-SVG-1.1/fonts-glyph-02-t.svg in a TOT debug build causes
ASSERTION FAILED: !HashTranslator::equal(KeyTraits::emptyValue(), key)
Comment 1 mitz 2008-05-01 12:48:44 PDT
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.
Comment 2 Robert Blaut 2008-06-14 12:46:27 PDT
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.
Comment 3 Robert Blaut 2008-06-16 11:22:01 PDT
*** Bug 19574 has been marked as a duplicate of this bug. ***
Comment 4 Mark Rowe (bdash) 2008-09-19 17:43:59 PDT
*** Bug 20950 has been marked as a duplicate of this bug. ***
Comment 5 Mark Rowe (bdash) 2008-09-19 17:45:10 PDT
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.
Comment 6 Alexey Proskuryakov 2008-11-17 12:52:12 PST
*** Bug 22321 has been marked as a duplicate of this bug. ***
Comment 7 Holger Freyther 2008-11-17 13:36:53 PST
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.
Comment 8 Holger Freyther 2008-11-17 13:39:08 PST
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. 
Comment 9 Eric Seidel (no email) 2008-11-25 17:44:18 PST
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.
Comment 10 Holger Freyther 2008-11-26 04:57:58 PST
(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.
Comment 11 Holger Freyther 2008-11-26 11:39:20 PST
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?
Comment 12 Darin Adler 2008-12-05 09:55:27 PST
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
Comment 13 Holger Freyther 2008-12-06 04:15:14 PST
(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.

Comment 14 Alexey Proskuryakov 2009-01-11 01:57:18 PST
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.
Comment 15 Alexey Proskuryakov 2010-06-10 17:46:15 PDT
This doesn't happen any more. Re-enabled the test in r60983.