Bug 76508 - [Chromium] Random characters got rendered as empty boxes or with incorrect glyphs even when a font is present
Summary: [Chromium] Random characters got rendered as empty boxes or with incorrect gl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-17 18:24 PST by Kazuhiro Inaba
Modified: 2012-01-19 11:04 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.98 KB, patch)
2012-01-18 20:39 PST, Kazuhiro Inaba
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2012-01-18 23:29 PST, Kazuhiro Inaba
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kazuhiro Inaba 2012-01-17 18:24:50 PST
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.
Comment 1 Kazuhiro Inaba 2012-01-17 18:26:21 PST
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...
Comment 2 Hironori Bono 2012-01-17 20:06:13 PST
Greetings,

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

Regards,

Hironori Bono
Comment 3 Kazuhiro Inaba 2012-01-18 20:39:32 PST
Created attachment 123062 [details]
Patch
Comment 4 James Robinson 2012-01-18 20:50:20 PST
Do we have any ways to test things like this?
Comment 5 Kent Tamura 2012-01-18 21:18:08 PST
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.
Comment 6 Kent Tamura 2012-01-18 21:41:43 PST
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.
Comment 7 Kazuhiro Inaba 2012-01-18 23:29:42 PST
Created attachment 123079 [details]
Patch
Comment 8 Kazuhiro Inaba 2012-01-18 23:34:06 PST
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.
Comment 9 Kent Tamura 2012-01-18 23:44:17 PST
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.
Comment 10 Kazuhiro Inaba 2012-01-18 23:47:29 PST
(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...
Comment 11 WebKit Review Bot 2012-01-19 01:08:25 PST
Comment on attachment 123079 [details]
Patch

Clearing flags on attachment: 123079

Committed r105393: <http://trac.webkit.org/changeset/105393>
Comment 12 WebKit Review Bot 2012-01-19 01:08:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Tony Chang 2012-01-19 10:41:18 PST
(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.
Comment 14 James Robinson 2012-01-19 11:04:56 PST
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.