Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1218043004:
Add check that Dwrite glyph fits in unicode table. (Closed)
|
Created:
5 years, 5 months ago by Will Harris Modified:
5 years, 5 months ago CC:
reviews_skia.org, bungeman-chromium Base URL:
https://chromium.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd check that Dwrite glyph fits in unicode table.
BUG=470146
Committed: https://skia.googlesource.com/skia/+/f7b54cda0d44cb1e7fd89586ff8ff3b78dfb823b
Patch Set 1 #
Total comments: 1
Patch Set 2 : code review changes #
Total comments: 10
Patch Set 3 : code review 2 #Messages
Total messages: 27 (11 generated)
wfh@chromium.org changed reviewers: + bungeman@chromium.org
PTAL
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/1218043004/diff/1/src/ports/SkTypeface_win_dw... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/1218043004/diff/1/src/ports/SkTypeface_win_dw... src/ports/SkTypeface_win_dw.cpp:296: hr = fontFace->GetGlyphIndices(&c, 1, &glyph); 1. Perhaps we should check for an error code in hr? (i.e. != S_OK) 2. If we are going to check against glyphCount, we should remove the assert
wfh@chromium.org changed reviewers: - bungeman@chromium.org
wfh@chromium.org changed reviewers: + reed@chromium.org - reed@google.com
wfh@chromium.org changed reviewers: + reed@google.com - reed@chromium.org
On 2015/07/07 14:22:52, reed1 wrote: > https://codereview.chromium.org/1218043004/diff/1/src/ports/SkTypeface_win_dw... > File src/ports/SkTypeface_win_dw.cpp (right): > > https://codereview.chromium.org/1218043004/diff/1/src/ports/SkTypeface_win_dw... > src/ports/SkTypeface_win_dw.cpp:296: hr = fontFace->GetGlyphIndices(&c, 1, > &glyph); > 1. Perhaps we should check for an error code in hr? (i.e. != S_OK) > 2. If we are going to check against glyphCount, we should remove the assert Done. PTAL
https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:297: if (SUCCEEDED(hr) && 0 < glyph && glyph < glyphCount) { THanks for the check on hr. Since we are still checking for a "legal" glyph even though GetGlyphIndices is spec'd to never return a bad one, I think we need to... 1. add a unittest that exercises this "bug" in the DW library, and hence the need for a runtime check 2. make the runtime check (glyph < glyphCount) inside a compile-guard saying it is an experiment 3. add a big comment/reference the bug, so future maintainers know *why* we're adding this (seemingly) redundant check.
On 2015/07/07 18:50:52, reed1 wrote: > https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... > File src/ports/SkTypeface_win_dw.cpp (right): > > https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... > src/ports/SkTypeface_win_dw.cpp:297: if (SUCCEEDED(hr) && 0 < glyph && glyph < > glyphCount) { > THanks for the check on hr. > > Since we are still checking for a "legal" glyph even though GetGlyphIndices is > spec'd to never return a bad one, I think we need to... > > 1. add a unittest that exercises this "bug" in the DW library, and hence the > need for a runtime check I cannot reproduce the crashes in the associated bug, so I don't think I will be able to come up with a test. I'm adding this check because it is what I suspect is happening from looking at the memory dumps from the crashes. > 2. make the runtime check (glyph < glyphCount) inside a compile-guard saying it is an experiment I'm happy for this to be an experiment to see if the crashes go away, and then revert the CL if they don't. > 3. add a big comment/reference the bug, so future maintainers know *why* we're adding this (seemingly) redundant check. I can certainly add this. WDYT?
if you can do #2 or #3, upload it and I'll take a look.
bungeman@google.com changed reviewers: + bungeman@google.com
I imagine this has something to do with .CompositeFont or with the other DW timing issues we've been seeing. It would be nice if we didn't need this function (glyph drawing operations could be provided the shaper's glyph to character mappings). https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:296: hr = fontFace->GetGlyphIndices(&c, 1, &glyph); If we're going to check the HRESULT, the above declaration of 'hr' should be removed and this line changed to HRNM(fontFace->GetGlyphIndices(&c, 1, &glyph), "Failed to get glyph index."); When this happens the data is probably hosed anyway, and nothing is assigned until successful completion, so this should work. https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:297: if (SUCCEEDED(hr) && 0 < glyph && glyph < glyphCount) { Similarly the check for glyph < glyphCount check. If this check is failing, then the data is suspect and this function needs to return early without writing to the out param. So in other words, this should go outside this check with a big comment like // Intermittent DW bug... <more details, link to bug> if (glyph < glyphCount) { return; }
I'm just full of copy paste issues today. https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:296: hr = fontFace->GetGlyphIndices(&c, 1, &glyph); On 2015/07/07 20:40:17, bungeman1 wrote: > If we're going to check the HRESULT, the above declaration of 'hr' should be > removed and this line changed to > > HRNM(fontFace->GetGlyphIndices(&c, 1, &glyph), "Failed to get glyph index."); > > When this happens the data is probably hosed anyway, and nothing is assigned > until successful completion, so this should work. That should be HRVM (this function returns void). https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:297: if (SUCCEEDED(hr) && 0 < glyph && glyph < glyphCount) { On 2015/07/07 20:40:17, bungeman1 wrote: > Similarly the check for glyph < glyphCount check. If this check is failing, then > the data is suspect and this function needs to return early without writing to > the out param. So in other words, this should go outside this check with a big > comment like > > // Intermittent DW bug... <more details, link to bug> > if (glyph < glyphCount) { > return; > } I suppose that should be // Intermittent DW bug... <more details, link to bug> if (glyph >= glyphCount) { return; }
I think I did everything. I didn't add a compile time guard, since if this works it sounds like something we might want to keep. I can monitor the crash rates once this hits canary. PTAL https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:296: hr = fontFace->GetGlyphIndices(&c, 1, &glyph); On 2015/07/07 21:02:39, bungeman1 wrote: > On 2015/07/07 20:40:17, bungeman1 wrote: > > If we're going to check the HRESULT, the above declaration of 'hr' should be > > removed and this line changed to > > > > HRNM(fontFace->GetGlyphIndices(&c, 1, &glyph), "Failed to get glyph index."); > > > > When this happens the data is probably hosed anyway, and nothing is assigned > > until successful completion, so this should work. > > That should be HRVM (this function returns void). Done. https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:296: hr = fontFace->GetGlyphIndices(&c, 1, &glyph); On 2015/07/07 20:40:17, bungeman1 wrote: > If we're going to check the HRESULT, the above declaration of 'hr' should be > removed and this line changed to > > HRNM(fontFace->GetGlyphIndices(&c, 1, &glyph), "Failed to get glyph index."); > > When this happens the data is probably hosed anyway, and nothing is assigned > until successful completion, so this should work. Done. https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:297: if (SUCCEEDED(hr) && 0 < glyph && glyph < glyphCount) { On 2015/07/07 21:02:40, bungeman1 wrote: > On 2015/07/07 20:40:17, bungeman1 wrote: > > Similarly the check for glyph < glyphCount check. If this check is failing, > then > > the data is suspect and this function needs to return early without writing to > > the out param. So in other words, this should go outside this check with a big > > comment like > > > > // Intermittent DW bug... <more details, link to bug> > > if (glyph < glyphCount) { > > return; > > } > > I suppose that should be > > // Intermittent DW bug... <more details, link to bug> > if (glyph >= glyphCount) { > return; > } yes I spotted that :) https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:297: if (SUCCEEDED(hr) && 0 < glyph && glyph < glyphCount) { On 2015/07/07 20:40:17, bungeman1 wrote: > Similarly the check for glyph < glyphCount check. If this check is failing, then > the data is suspect and this function needs to return early without writing to > the out param. So in other words, this should go outside this check with a big > comment like > > // Intermittent DW bug... <more details, link to bug> > if (glyph < glyphCount) { > return; > } Done. https://codereview.chromium.org/1218043004/diff/20001/src/ports/SkTypeface_wi... src/ports/SkTypeface_win_dw.cpp:297: if (SUCCEEDED(hr) && 0 < glyph && glyph < glyphCount) { On 2015/07/07 18:50:52, reed1 wrote: > THanks for the check on hr. > > Since we are still checking for a "legal" glyph even though GetGlyphIndices is > spec'd to never return a bad one, I think we need to... > > 1. add a unittest that exercises this "bug" in the DW library, and hence the > need for a runtime check > 2. make the runtime check (glyph < glyphCount) inside a compile-guard saying it > is an experiment > 3. add a big comment/reference the bug, so future maintainers know *why* we're > adding this (seemingly) redundant check. Acknowledged.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218043004/40001
The CQ bit was unchecked by bungeman@google.com
The CQ bit was checked by bungeman@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218043004/40001
The CQ bit was unchecked by rmistry@google.com
On 2015/07/07 21:44:39, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1218043004/40001 not sure what happened here, perhaps this still needs an l-g-t-m from reed? PTAL?
On 2015/07/07 23:20:23, Will Harris wrote: > On 2015/07/07 21:44:39, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1218043004/40001 > > not sure what happened here, perhaps this still needs an l-g-t-m from reed? > PTAL? Sorry, I had unchecked the CQ checkbox because there is an ongoing issue affecting the Commit Queues of all projects. You can follow along here: https://code.google.com/p/chromium/issues/detail?id=507817.
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218043004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/f7b54cda0d44cb1e7fd89586ff8ff3b78dfb823b |
