Closed Bug 849976 Opened 11 years ago Closed 11 years ago

text reftests using @font-face need to be annotated with HTTP(..)

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: billm, Assigned: jtd)

Details

Attachments

(1 file)

Most of the tests in layout/reftests/text that use custom fonts are annotation with HTTP(..), which allows them to access those fonts. However, there are a few tests that don't have the annotation:

font-selection-fallback-1.html
ligature-with-space-1.html
variation-selector-unsupported-1.html

Consequently, they don't load the custom fonts and so they fall back to the default font. I think this means that they're not testing what they're supposed to test.

In bug 846890, I changed a flag so that these tests actually load the correct fonts. The result is that they fail intermittently with the following assertion:

###!!! ASSERTION: caching a font associated with no family yet: 'aFontEntry->mFamilyName.Length() != 0', file ../../../gfx/thebes/gfxUserFontSet.cpp, line 773

Additionally, nothing gets drawn and so the test fails (for some reason the reference part of the test does manage to draw something). It seems like there's some kind of timing issue at play.

Here are some examples of the failure:

https://tbpl.mozilla.org/php/getParsedLog.php?id=20481027&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=20480842&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=20480875&tree=Mozilla-Inbound

Since this is an existing problem (i.e., the tests are already broken), I'm planning to annotate them as failing randomly and possibly asserting. I'm filing this bug for fixing the underlying problem.
FTR, it looks like these three tests come from bug 678561, bug 634556 and bug 427672 respectively.

(In reply to Bill McCloskey (:billm) from comment #0)
> In bug 846890, I changed a flag so that these tests actually load the
> correct fonts. The result is that they fail intermittently with the
> following assertion:
> 
> ###!!! ASSERTION: caching a font associated with no family yet:
> 'aFontEntry->mFamilyName.Length() != 0', file
> ../../../gfx/thebes/gfxUserFontSet.cpp, line 773
> 
> Additionally, nothing gets drawn and so the test fails (for some reason the
> reference part of the test does manage to draw something). It seems like
> there's some kind of timing issue at play.

Hmm, that's a bit worrying. That assertion comes from bug 833169, and suggests there may be something slightly broken about the fontEntry family name management that was added there.

(I note that Jesse has already filed bug 847272 on that assertion.)
I have a feeling this is something different from bug 846890, these assertions occur under Linux builds where the name is constructed differently from Win/OSX/Android builds.
[clarifying summary, since once bug 846890 has landed, these tests will no longer "pass vacuously" anymore]
Summary: A few text reftests pass vacuously → A few text reftests pass simply because they can't access the fonts they're testing, and they fail/assert if we give them access.
Version: unspecified → Trunk
Assignee: nobody → jdaggett
Summary: A few text reftests pass simply because they can't access the fonts they're testing, and they fail/assert if we give them access. → text reftests using @font-face need to be annotated with HTTP(..)
Talking with dholbert on IRC, it appears that adding HTTP(..) may not be needed for reftests containing @font-face rules in the future, once bug 846890 lands.  In that case, if the fix on bug 847272 fixes this then we can close this as a dupe.
It would probably be useful for this to land separately, and first, to make any (test failure) regressions clearer.
Comment on attachment 724272 [details] [diff] [review]
patch, enable HTTP(..) for reftests that use @font-face rules

If this passes try, feel free to land with r=me

(I just suggested that billm do this in lieu of his last patch on bug 846890, but actually I think dbaron makes a good point here that this should likely land separately & first, so that we can verify that these tests pass in the way that they were meant to be tested, independent of the stuff that's going on in bug 846890.)
Attachment #724272 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6ce961aa9996
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: