Bug 35486 - canvas fillText with @font-face crashes
Summary: canvas fillText with @font-face crashes
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL: http://philip.html5.org/tests/canvas/...
Depends on:
Reported: 2010-02-27 14:04 PST by Philip Taylor
Modified: 2010-06-28 10:00 PDT (History)
8 users (show)

See Also:

Proposed patch (1.94 KB, patch)
2010-06-25 04:40 PDT, Robin Cao
no flags Details | Formatted Diff | Diff
Updated patch (4.36 KB, patch)
2010-06-27 20:20 PDT, Robin Cao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Taylor 2010-02-27 14:04:09 PST
The linked test case crashes randomly when loading (most of the time, but not always), in nightly WebKit on Windows and in Chromium on Linux.

It loads a font with @font-face, and doesn't use the font in the page itself, but tries drawing with it in onload onto the canvas (which won't work because it hasn't got around to downloading the font yet); and then it draws in the canvas again, after a time delay, at which point it usually crashes.
Comment 1 Jakob Petsovits 2010-06-15 11:56:23 PDT
The issue is that the SimpleFontData object is deleted by CSSFontFaceSource::fontLoaded() - called from CachedFont::checkNotify(), CachedFont::data(), and that one from Loader::Host::didFinishLoading().

When CanvasRenderingContext2D::drawTextInternal() accesses the font the second time, after loading has finished, the Font's cached SimpleFontData is a corrupt pointer and (mostly) crashes when trying to draw. Still trying to understand why the font has to be deleted...
Comment 2 Jakob Petsovits 2010-06-24 13:40:02 PDT
Alright, so just to conclude my previous comment, or rephrase it in a better way...

When the font is first accessed, it hasn't been loaded yet; it does not yet exist in the font cache or anywhere. Instead, CSSFontFaceSource::getFontData() kicks off loading and, as an interim solution, creates a copy of a system font (likely the last-resort fallback font, but there might be a more suitable one). When the font is fully loaded, these interim objects will be deleted so that on next access, the actually desired font will be generated from the downloaded data when it's accessed by CSSFontFaceSource::getFontData() the next time.

By deleting the interim SimpleFontData objects though, CSSFontFaceSource::pruneTable() pulls the foundations from existing Font objects like the one in CanvasRenderingContext2D. Apparently the SimpleFontData objects are not expected to last that long in the RenderTree, which might well be the case, only the CanvasRenderingContext2D is not part of the RenderTree so I guess that doesn't apply here. Anyways, I can't see a straightforward fix right now.
Comment 3 Robin Cao 2010-06-24 20:09:45 PDT
This issue is a little tricky. Normally we get font objects from RenderStyle, and font objects get updated automatically after calling recalcStyle. But the font object in CanvasRenderingContext2D seems to be an exception, so it may become invalid at some point.

We can override recalcStyle() in HTMLCanvasElement, and update the font object from there if needed. The patch will look like this:

Index: WebCore/html/HTMLCanvasElement.h
+    virtual void recalcStyle(StyleChange);

Index: WebCore/html/HTMLCanvasElement.cpp
+void HTMLCanvasElement::recalcStyle(StyleChange change)
+    HTMLElement::recalcStyle(change);
+    // Update font if needed.
+    if (change == Force && m_context && m_context->is2d()) {
+        CanvasRenderingContext2D* ctx = static_cast<CanvasRenderingContext2D*>(m_context.get());
+        ctx->setFont(ctx->font());
+    }

Jakob, do you think this is a viable solution?
Comment 4 Jakob Petsovits 2010-06-25 01:12:10 PDT
Awesome, I think that's the solution we're looking for, yes.
I can't currently look it up (posting only from my phone), but do other contexts have font properties too? In that case, we should update those as well.
Comment 5 Robin Cao 2010-06-25 01:54:51 PDT
Only CanvasRenderingContext2D has font properties.
I will upload a patch later.
Comment 6 Robin Cao 2010-06-25 04:40:07 PDT
Created attachment 59757 [details]
Proposed patch
Comment 7 David Levin 2010-06-25 08:11:19 PDT
Layout test?
Comment 8 Jakob Petsovits 2010-06-25 08:35:50 PDT
The Philip canvas tests are already in WebKit, this bug concerns canvas/philip/tests/2d.text.draw.fontface.repeat.html.
Comment 9 mitz 2010-06-27 09:09:52 PDT
Comment on attachment 59757 [details]
Proposed patch

The patch looks correct to me, but rather than calling setFont(), could you just reset the m_realizedFont member of the state? Alternatively, could you just call a method that calls Font::update() rather than involving the CSS parser and style selector?
Comment 10 Robin Cao 2010-06-27 20:20:14 PDT
Created attachment 59870 [details]
Updated patch

Update the patch according to mitz's comment.
Comment 11 mitz 2010-06-27 20:27:00 PDT
(In reply to comment #8)
> The Philip canvas tests are already in WebKit, this bug concerns canvas/philip/tests/2d.text.draw.fontface.repeat.html.

I am not sure what this comment means. Does the test fail without this patch and pass with it? If so, new results need to be included in the patch. If not, the question from comment #7 stands: can you make a regression test for this bug?

The code change itself looks good.
Comment 12 Jakob Petsovits 2010-06-27 22:00:31 PDT
Yes, the test crashes without the patch. Didn't test out the fix yet; if it does solve the problem as I believe it does, the crash is avoided afterwards.
Comment 13 Robin Cao 2010-06-27 22:35:08 PDT
(In reply to comment #12)
> Yes, the test crashes without the patch. Didn't test out the fix yet; if it does solve the problem as I believe it does, the crash is avoided afterwards.

The patch fixes the crash, i have verified that. The test result need not be updated, as the patch does not affect test output.
Comment 14 WebKit Commit Bot 2010-06-28 10:00:49 PDT
Comment on attachment 59870 [details]
Updated patch

Clearing flags on attachment: 59870

Committed r62016: <http://trac.webkit.org/changeset/62016>
Comment 15 WebKit Commit Bot 2010-06-28 10:00:54 PDT
All reviewed patches have been landed.  Closing bug.