[Chromium] Random characters got rendered as empty boxes or with incorrect glyphs even when a font is present

• Component: Text
• Status: RESOLVED FIXED

[Kazuhiro Inaba]

Reported at http://crbug.com/80235.

When WebKit renderers run in the sandbox of Windows Chromium, random characters occasionally got rendered as incorrect glyphs.
Inspection by debugger showed that this likely happens when GetGlyphIndices() Win32 API call failed due to sandboxing.

[Kazuhiro Inaba]

One way to fix the issue is to retry the API call preceded by font loading on host process, when it failed. I'll prepare the patch...

[Hironori Bono]

Greetings,

Can you update http://crbug.com/80235, refer this URL, and change its status to ExternalDependency?

Regards,

Hironori Bono

[Kazuhiro Inaba]

Created attachment 123062 [details]
Patch

[James Robinson]

Do we have any ways to test things like this?

[Kent Tamura]

Comment on attachment 123062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123062&action=review

r+. But there are some style issues.

> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:59
> +static bool getGlyphIndices(HFONT font,
> +                            HDC dc,
> +                            const UChar* buffer,
> +                            unsigned length,
> +                            WORD* glyphBuffer,
> +                            DWORD flag) {

You don't need to wrap these lines.

We had better rename 'buffer' to 'characters', 'length'  to 'charactersLength' because this function has two buffer arguments.

> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:148
> +    if (!getGlyphIndices(fontData->platformData().hfont(),
> +                         dc, buffer, length, localGlyphBuffer, GGI_MARK_NONEXISTING_GLYPHS)) {

You don't need to wrap these lines.

> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:150
> +        SelectObject(dc, oldFont);
> +        ReleaseDC(0, dc);

We had better introduce scoped objects like base/win/scoped_dc.h of Chromium.
You don't need to add them in this patch.

[Kent Tamura]

Comment on attachment 123062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123062&action=review

>> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:150
>> +        ReleaseDC(0, dc);
> 
> We had better introduce scoped objects like base/win/scoped_dc.h of Chromium.
> You don't need to add them in this patch.

We already have WebCore/platform/win/HwnDC.h for HDC.

[Kazuhiro Inaba]

Created attachment 123079 [details]
Patch

[Kazuhiro Inaba]

Comment on attachment 123062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123062&action=review

>> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:59
>> +                            DWORD flag) {
> 
> You don't need to wrap these lines.
> 
> We had better rename 'buffer' to 'characters', 'length'  to 'charactersLength' because this function has two buffer arguments.

Done.

>> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:148
>> +                         dc, buffer, length, localGlyphBuffer, GGI_MARK_NONEXISTING_GLYPHS)) {
> 
> You don't need to wrap these lines.

Done.

>>> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:150
>>> +        ReleaseDC(0, dc);
>> 
>> We had better introduce scoped objects like base/win/scoped_dc.h of Chromium.
>> You don't need to add them in this patch.
> 
> We already have WebCore/platform/win/HwnDC.h for HDC.

Switched to use HWndDC, and,
in preference of avoiding nested GetDC, removed the recursion in fillBMPGlyphs.

[Kent Tamura]

Comment on attachment 123079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123079&action=review

> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:69
> +// Initializes space glyph

nit: The comment is useless. It's equivalent to the function name.

[Kazuhiro Inaba]

(In reply to comment #4)
> Do we have any ways to test things like this?

I'd be happy to integrate it if someone knows a way to do this,
but at least I myself cannot come up with a good idea...

[WebKit Review Bot]

Comment on attachment 123079 [details]
Patch

Clearing flags on attachment: 123079

Committed r105393: <http://trac.webkit.org/changeset/105393>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(In reply to comment #10)
> (In reply to comment #4)
> > Do we have any ways to test things like this?
> 
> I'd be happy to integrate it if someone knows a way to do this,
> but at least I myself cannot come up with a good idea...

It sounds like this doesn't repro without the sandbox, so DRT and test_shell probably won't work.

That said, it'd be fine to add a test case that repros the bug in Chromium Win so at least someone can manually run the test with Chromium.  I think it would be fine as either a manual test or a layout test.  A layout test might be a bit better because people are working on getting content_shell (which uses the sandbox) to run layout tests.

[James Robinson]

What about mocking these interfaces and having a setting to make them fail if ensureFontLoaded() isn't called? This seems to be something that is constantly being broken and fixed and it doesn't sound like a fun thing at all to debug.