Closed Bug 472237 Opened 11 years ago Closed 11 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: jruderman, Assigned: jtd)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, verified1.9.1)

Crash Data

Attachments

(4 files)

This testcase causes a crash where ctx->GetUserFontSet() is null in nsFontFaceLoader::OnStreamComplete.  Five stack frames deeper, something touches a member variable and crashes.
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.
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
Assignee: nobody → jdaggett
Status: NEW → ASSIGNED
Note: to reproduce, need to remove all @font-face rules from user stylesheets
Flags: blocking1.9.1?
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1b3
Flags: blocking1.9.1? → blocking1.9.1+
Simple fix, just check for a null user font set and bail if null.
Attachment #355543 - Flags: superreview?(dbaron)
Attachment #355543 - Flags: review?(dbaron)
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?)
Attachment #355543 - Flags: superreview?(dbaron)
Attachment #355543 - Flags: superreview+
Attachment #355543 - Flags: review?(dbaron)
Attachment #355543 - Flags: review+
(It might be good to file followup bugs on some of those things if they're problems...)
Checked in on trunk
http://hg.mozilla.org/mozilla-central/rev/3ef1e2ac1c51
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(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.
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...
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.
Switched the crashtest to a local font:
http://hg.mozilla.org/mozilla-central/rev/be51e923473c
Thanks.
Flags: in-testsuite+
(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.
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.
Status: RESOLVED → VERIFIED
Depends on: 615858
No longer depends on: 615858
Crash Signature: [@ nsFontFaceLoader::OnStreamComplete]
You need to log in before you can comment on or make changes to this bug.