Bug 35486 – canvas fillText with @font-face crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Philip Taylor]

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.

[Jakob Petsovits]

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...

[Jakob Petsovits]

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.

[Robin Cao]

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?

[Jakob Petsovits]

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.

[Robin Cao]

Only CanvasRenderingContext2D has font properties.
I will upload a patch later.

[Robin Cao]

Created attachment 59757 [details]
Proposed patch

[David Levin]

Layout test?

[Jakob Petsovits]

The Philip canvas tests are already in WebKit, this bug concerns canvas/philip/tests/2d.text.draw.fontface.repeat.html.

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?

[Robin Cao]

Created attachment 59870 [details]
Updated patch

Update the patch according to mitz's comment.

(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.

[Jakob Petsovits]

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.

[Robin Cao]

(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.

[WebKit Commit Bot]

Comment on attachment 59870 [details]
Updated patch

Clearing flags on attachment: 59870

Committed r62016: <http://trac.webkit.org/changeset/62016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.