Ensure that Font.charToGlyph won't fail because String.fromCodePoint is given an invalid code point (issue 11768)
#11769
Conversation
Snuffleupagus
force-pushed
the
charToGlyph-fontCharCode-range
branch
from
9651efe to
3e4bb4f
Compare
Snuffleupagus
changed the title
Font.charToGlyph won't fail completely (issue 11768)Font.charToGlyph won't fail because String.fromCodePoint is given an invalid code point (issue 11768)
Snuffleupagus
force-pushed
the
charToGlyph-fontCharCode-range
branch
from
3e4bb4f to
251c187
Compare
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/4d1091cde93c899/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/4d1091cde93c899/output.txt Total script time: 2.44 mins Published |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/1c937e61dc82d6c/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/e2e9e68fdf4f92e/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/e2e9e68fdf4f92e/output.txt Total script time: 1.79 mins
Image differences available at: http://54.215.176.217:8877/e2e9e68fdf4f92e/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/1c937e61dc82d6c/output.txt Total script time: 19.70 mins
Image differences available at: http://54.67.70.0:8877/1c937e61dc82d6c/reftest-analyzer.html#web=eq.log |
| typeof fontCharCode === "number" | ||
| ? String.fromCodePoint(fontCharCode) | ||
| : ""; | ||
| let fontChar = ""; |
There was a problem hiding this comment.
Is this method called often? If so, it may be beneficial to leave it undefined here and only set it to the empty string in the exception cases to avoid unnecessary string allocation in the common case when the code point is valid.
There was a problem hiding this comment.
Is this method called often?
Yes, but comparing the run-time of the tests here to other PRs there really doesn't seem to be any noticeable slowdowns overall (I think browsers may be able to optimize/re-use strings internally).
and only set it to the empty string in the exception cases
While I did consider that, it's not clear to me that it's entirely necessary here (also the code would become a bit "messy" in that case IMHO).
Also, you're still having to allocate the variable (albeit implicitly to undefined) which is probably why it doesn't seem to matter in practice.
src/core/fonts.js
Outdated
| : ""; | ||
| let fontChar = ""; | ||
| if (typeof fontCharCode === "number") { | ||
| if (fontCharCode < 0x110000) { |
There was a problem hiding this comment.
Would it perhaps be better to check for < 0 or > 0x10FFFF just like in the specification, so also negative numbers are handled here? If that is not deemed necessary, can we at least change this to <= 0x10FFFF so it's easier to match with the spec?
There was a problem hiding this comment.
Would it perhaps be better to check for
< 0 or > 0x10FFFFjust like in the specification,
I thought about the < 0 case, but honestly it seems to me that that would indicate much larger underlying problems in the code to the point that I'd not mind a breaking error (also considering that the String.fromCodePoint changes landed ~1.5 years ago and this is the first time they've caused a problem).
can we at least change this to
<= 0x10FFFFso it's easier to match with the spec?
If you think the current formatting is too confusing, maybe because of the amount of digits involved, I suppose that that can be changed :-)
Snuffleupagus
force-pushed
the
charToGlyph-fontCharCode-range
branch
from
251c187 to
14ea345
Compare
…nt` is given an invalid code point (issue 11768) *Please note:* This patch on its own is *not* sufficient to address the underlying problem in the referenced issue, hence why no test-case is included since the *actual* bug still needs to be fixed. As can be seen in the specification, https://tc39.es/ecma262/#sec-string.fromcodepoint, `String.fromCodePoint` will throw a RangeError for invalid code points. In the event that a CMap, in a composite font, contains invalid data and/or we fail to parse it correctly, it's thus possible that the glyph mapping that we build end up with entires that cause `String.fromCodePoint` to throw and thus `Font.charToGlyph` to break. If that happens, as is the case in issue 11768, significant portions of a page/document may fail to render which seems very unfortunate. While this patch doesn't fix the underlying problem, it's hopefully deemed useful not only for the referenced issue but also to prevent similar bugs in the future.
Snuffleupagus
force-pushed
the
charToGlyph-fontCharCode-range
branch
from
14ea345 to
87142a6
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/aaffa554e119241/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/243971614f89fb7/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/aaffa554e119241/output.txt Total script time: 19.74 mins
Image differences available at: http://54.67.70.0:8877/aaffa554e119241/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/243971614f89fb7/output.txt Total script time: 60.00 mins |
|
Thanks! |
Please note: This patch on its own is not sufficient to address the underlying problem in the referenced issue, hence why no test-case is included since the actual bug still needs to be fixed.
As can be seen in the specification, https://tc39.es/ecma262/#sec-string.fromcodepoint,
String.fromCodePointwill throw a RangeError for invalid code points.In the event that a CMap, in a composite font, contains invalid data and/or we fail to parse it correctly, it's thus possible that the glyph mapping that we build end up with entires that cause
String.fromCodePointto throw and thusFont.charToGlyphto break.If that happens, as is the case in issue #11768, significant portions of a page/document may fail to render which seems very unfortunate.
While this patch doesn't fix the underlying problem, it's hopefully deemed useful not only for the referenced issue but also to prevent similar bugs in the future.