472237 - Crash [@ nsFontFaceLoader::OnStreamComplete] with @font-face and removing root element

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

This testcase causes a crash where ctx->GetUserFontSet() is null in nsFontFaceLoader::OnStreamComplete.  Five stack frames deeper, something touches a member variable and crashes.

Comment 1

[John Daggett (:jtd)]

Attachment: modified testcase, use font on server with access control headers set

Jesse, I'm not seeing this crash.  To start with, the testcase uses a font cross-site without access controls enabled, so the font fails to load.  Do you see a crash when loading this attachment from bugzilla?  I tried both with mozilla central latest and the latest nightly:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090105 Minefield/3.2a1pre

Attached is a new testcase that loads fonts from a server that sets the access control headers when sending the font.  For Apache, to allow a font to be used anywhere:

.htaccess:

<FilesMatch "\.(ttf|otf)$">
<IfModule mod_headers.c>
Header set Access-Control-Allow-Origin "*"
</IfModule>
</FilesMatch>


A small nit, but if you could follow the guidelines of the original font author (i.e. pages using Fontin via @font-face should include a link back to the author's page) that would be best.  I realize that's a bit nonsensical in this case but it helps encourage respect for font license terms specified by font designers.

Comment 2

[Jesse Ruderman]

My testcase crashes an updated debug build of Firefox, even when loaded from Bugzilla.  For this code, it doesn't matter whether the font load passes the access-control checks:

http://hg.mozilla.org/mozilla-central/annotate/82e26897aba9/layout/style/nsFontFaceLoader.cpp#l128

Comment 3

[John Daggett (:jtd)]

Note: to reproduce, need to remove all @font-face rules from user stylesheets

Comment 4

[John Daggett (:jtd)]

Attachment: patch, v.0.1, check for null user font set

Simple fix, just check for a null user font set and bail if null.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 355543 [details] [diff] [review]
patch, v.0.1, check for null user font set

r+sr=dbaron, although I'm wondering if we should have done something to stop the loads that aren't needed anymore.  (In fact, I sort of think we should have.)  If we do that, though, we need to make sure that the ones that are still needed are still loading.  (Do we do something that causes the load to be shared if we rebuild the user font set during a load, or do we do the whole load twice?)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(It might be good to file followup bugs on some of those things if they're problems...)

Comment 7

[John Daggett (:jtd)]

Checked in on trunk
http://hg.mozilla.org/mozilla-central/rev/3ef1e2ac1c51

Comment 8

[John Daggett (:jtd)]

(In reply to comment #5)
> (From update of attachment 355543 [details] [diff] [review])
> r+sr=dbaron, although I'm wondering if we should have done something to stop
> the loads that aren't needed anymore.  (In fact, I sort of think we should
> have.)  If we do that, though, we need to make sure that the ones that are
> still needed are still loading.  (Do we do something that causes the load to be
> shared if we rebuild the user font set during a load, or do we do the whole
> load twice?)

Loads are associated with a load group, so those are automatically stopped when the presContext is dumped.  But I think you're thinking about the scenario where:

  page loads with large downloadable font
  stylesheet changed via script in a way that causes a user font set rebuild
  page loads same/different fonts using new user font set

In this case you could end up calling through to the new user font set with an old proxy font entry which is bad.

Comment 9

[John Daggett (:jtd)]

Attachment: patch, v.0.1a, including crashtest

For some reason when the testcase is appended on the end of crashtest.list the crash doesn't occur (without the patch).  But with an additional 'load about:blank' it does.  Weirdness...

Comment 10

[Jesse Ruderman]

Please change the @font-face in the crashtest to use one of the small fonts in our repository.  Using an external URL can create random oranges when the tubes are clogged.

Comment 11

[John Daggett (:jtd)]

Switched the crashtest to a local font:
http://hg.mozilla.org/mozilla-central/rev/be51e923473c

Comment 12

[Jesse Ruderman]

Thanks.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> But I think you're thinking about the scenario
> where:
> 
>   page loads with large downloadable font
>   stylesheet changed via script in a way that causes a user font set rebuild
>   page loads same/different fonts using new user font set
> 
> In this case you could end up calling through to the new user font set with an
> old proxy font entry which is bad.

This sounds like bug 462593 comment 11 and sounds like it would be fixed by bug 462593 comment 13.

Comment 14

[John Daggett (:jtd)]

Checked in on 1.9.1 branch
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a806e1074255

Comment 15

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre.