943270 - gfxFontUtils should use EncodingUtils::DecoderForEncoding instead of nsICharsetConverterManager::GetUnicodeDecoderRaw

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Henri Sivonen (:hsivonen)]

EncodingUtils::DecoderForEncoding is a deCOMtaminated replacement for nsICharsetConverterManager::GetUnicodeDecoderRaw.

https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontUtils.cpp#1306 should use the new method. However, it's not quite a drop-in replacement, because gfxFontUtils sometimes passes in an empty string, but EncodingUtils::DecoderForEncoding only accepts real Gecko-canonical labels. Hence, gfxFontUtils would need to deal with failure to find a Gecko-canonical label on its own first.

Comment 1

[Henri Sivonen (:hsivonen)]

For clarity, the new method is from bug 919935 and is on inbound at the moment.

Comment 2

[Jonathan Kew [:jfkthame]]

AFAICS, gfxFontUtils::DecodeFontName() *won't* ever pass an empty string to this method. It gets the encoding label from GetCharsetForFontName(), which may indeed return either nullptr or "" in addition to valid labels, but these two "special" values are handled separately without trying to get an actual decoder.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: use EncodingUtils::DecoderForEncoding instead of nsICharsetConverterManager::GetUnicodeDecoderRaw when decoding font names

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8338432 [details] [diff] [review]
use EncodingUtils::DecoderForEncoding instead of nsICharsetConverterManager::GetUnicodeDecoderRaw when decoding font names

>+    nsCOMPtr<nsIUnicodeDecoder> decoder =
>+        mozilla::dom::EncodingUtils::DecoderForEncoding(csName);
>+    if (!decoder) {
>         NS_WARNING("failed to get the decoder for a font name string");
>         return false;

This should never happen if csName is indeed a Gecko-canonical label, since we have infallible malloc. (DecoderForEncoding MOZ_ASSERTs this.)

Comment 5

[Jonathan Kew [:jfkthame]]

I believe the intention is that GetCharsetForFontName() should always return a Gecko-canonical name (or empty/null).

Still, I'd be inclined to leave the extra null check here, as a safeguard against possible future changes (e.g. if we switch Gecko to using some other (more standard) naming system, or remove support for certain encodings, etc., and fail to update everything correctly). Some of the possible results from GetCharsetForFontName() are probably very rare, only occurring if a user happens to have an unusual font installed that has specific localized names, so I'm not confident we have unit test coverage that would immediately detect future breakage.

Comment 6

[Henri Sivonen (:hsivonen)]

OK.

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/068411d02b1b

Comment 8

[Henri Sivonen (:hsivonen)]

Thank you.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/068411d02b1b