[Webkit-unassigned] [Bug 118410] Segmentation fault occurred when ICU data library doesn't embed the expected encoding

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=118410





--- Comment #16 from Benjamin Dupont <bdupont at nds.com>  2013-07-10 09:53:28 PST ---
(In reply to comment #15)
> (From update of attachment 206390 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206390&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> The patch cannot be landed with this line, a pre-commit hook will block it.
Yes, I saw I forgot this line just after I have uploaded the patch...

> > Source/WebCore/platform/text/TextCodecICU.cpp:-72
> > -    registrar("ISO-8859-8-I", "ISO-8859-8-I");
> 
> I don't see how moving this line down can work. addToTextEncodingNameMap() gets an atomic name for encoding name, so if ISO-8859-8-I is registered after enumerating ICU aliases, it will just map to ISO-8859-8. That is exactly what we are trying to avoid.
I didn't understand the existing comment and I can't match your explanation with the current implementation.
e.g If we add first 
1) ISO-8859-8:
registrar("ISO-8859-8", "ISO-8859-8") -> addToTextEncodingNameMap
a) isUndesiredAlias -> no
b) textEncodingNameMap->get("ISO-8859-8") -> 0 -> atomicName = "ISO-8859-8"
c) textEncodingNameMap->add("ISO-8859-8", "ISO-8859-8")

2) ISO-8859-8-I:
registrar("ISO-8859-8-I", "ISO-8859-8-I") -> addToTextEncodingNameMap
a) isUndesiredAlias -> no
b) textEncodingNameMap->get("ISO-8859-8-I") -> 0 -> atomicName = "ISO-8859-8-I"
c) textEncodingNameMap->add("ISO-8859-8-I", "ISO-8859-8-I")

Thus after these 2 iterations my understanding is that the hash map should contain both encodings.

Thanks in advance for your explanations.

> 
> > Source/WebCore/platform/text/TextCodecICU.cpp:69
> > +    WTF::HashMap<const char*, char>knownEncodings;
> 
> There shouldn't be a WTF prefix when using WTF names. Also, a missing space before knownEncodings.
> 
> Didn't you mean to use HashSet?
> 
Oops, yes I used HashSet but It seems I had a problem during my merge. :)

> > Source/WebCore/platform/text/TextCodecICU.cpp:138
> > +    if (knownEncodings.contains("macintosh")) {
> 
> This is not the right check. Since registrar uses an atomic name for standard name, it was fine to call it with any alias as second argument. But this check will fail.
I think with the previous point explanation it will be clearer for this one also.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.