Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permanent failure in /css21_dev/html4/font-family-013.htm and /css21_dev/html4/fonts-013.htm #7625

Closed
jdm opened this issue Sep 14, 2015 · 26 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Sep 14, 2015

These tests are both using unicode characters, and the run which contained a failure showed the characters appearing as boxes.

The test itself is interesting:

   .a span.test { font: 4em "Ahem", "Times New Roman"; }
   .a span.control { font: 4em "Times New Roman"; }
   .b span.test { font: 4em "Ahem", "Arial"; }
   .b span.control { font: 4em "Arial"; }
   .c span.test { font: 4em "Ahem", "Courier New"; }
   .c span.control { font: 4em "Courier New"; }

It looks like it's assuming that Ahem should not be present??

@jdm
Copy link
Member Author

@jdm jdm commented Sep 14, 2015

It's unclear to me why this would intermittently fail unless we haven't made Ahem available on all machines.

@jdm
Copy link
Member Author

@jdm jdm commented Sep 14, 2015

Or maybe this is testing glyph fallback behaviour, since presumably Ahem doesn't include those characters...

@jdm
Copy link
Member Author

@jdm jdm commented Sep 14, 2015

Oh, are we missing the msttcore fonts on some linux machines? @larsbergstrom @metajack @edunham

@jdm
Copy link
Member Author

@jdm jdm commented Sep 14, 2015

This was first seen on linux2, by by the way.

@jdm
Copy link
Member Author

@jdm jdm commented Sep 14, 2015

11:17 <jdm> jack: actually wait, my theory doesn't hold up because the output is <box><box><box><box> - test
11:17 <jdm> jack: with the ones on the left being "Ahem", [something else], and the ones on the right being [something else]
11:18 <jdm> so if the font was missing we should see <box><box><box><box>
11:18 <jdm> on both sides
11:18 <jack> ah
11:18 <jdm> so perhaps this is an actual problem in our glyph selection code or something
@jdm
Copy link
Member Author

@jdm jdm commented Sep 14, 2015

Seen on linux1, too. Did we make a change to saltfs and not actually update it until I did so earlier today?

@metajack
Copy link
Contributor

@metajack metajack commented Sep 14, 2015

So I added msttcorefonts to our salt configs a week or so ago. I tested it a few times and it seemed ok, but perhaps that is busted somehow.

See servo/saltfs#111 and servo/saltfs#112

@jdm jdm changed the title Intermittent reftest failure in /css21_dev/html4/font-family-013.htm and /css21_dev/html4/fonts-013.htm Permanent failure in /css21_dev/html4/font-family-013.htm and /css21_dev/html4/fonts-013.htm Sep 14, 2015
@jdm jdm removed the I-intermittent label Sep 14, 2015
@metajack
Copy link
Contributor

@metajack metajack commented Sep 14, 2015

@bors-servo retry

  • spinning up builders to test possible fix
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Sep 16, 2015

These tests assume that Ahem is installed, but rely on the next family in font-family being selected when Ahem has no glyph for a given character. For a while they passed because Ahem was not available. #7013 added Ahem, and these tests started failing because the fallback did not occur correctly. They started passing again when we installed msttcorefonts on the builders at some point.

If they fail again now, the most likely explanation is that msttcorefonts is missing.

@metajack
Copy link
Contributor

@metajack metajack commented Sep 16, 2015

They are missing. The build automation is failing to install these correctly, and I'm still debugging.

@metajack
Copy link
Contributor

@metajack metajack commented Sep 17, 2015

This is now fixed.

@metajack metajack closed this Sep 17, 2015
@jdm jdm reopened this Oct 16, 2015
@jdm
Copy link
Member Author

@jdm jdm commented Oct 16, 2015

This started happening again.

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 16, 2015

I manually installed the fonts on linux2.

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Oct 18, 2015

I think that #8053 is a better long-term solution.

@gsnedders
Copy link
Contributor

@gsnedders gsnedders commented Oct 27, 2015

Even without the MS fonts installed shouldn't they still pass?

The final stage of font matching algorithm is:

If there is no font within the family selected in 2, then use a UA-dependent default 'font-family' and repeat step 2, using the best match that can be obtained within the default font. If a particular character cannot be displayed using this font, then the UA may use other means to determine a suitable font for that character. The UA should map each character for which it has no suitable font to a visible symbol chosen by the UA, preferably a "missing character" glyph from one of the font faces available to the UA.

That means the essentially the test becomes equivalent to:

<p>The two must look the same:</p>
<p style="font-family: Ahem, Times">Ţęşţ</p>
<p style="font-family: initial">Ţęşţ</p>

Both should render using the UA default 'font-family', no?

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Nov 3, 2015

Yes, it sounds like we have a bug in font selection/fallback and installing MS fonts just avoids triggering it. Long term, we should investigate that bug and fix it. In the mean time, installing MS fonts on CI servers is troublesome so maybe we can simply disable these tests.

@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Dec 31, 2015

@metajack This is happening again after the move to the new EC2 builders. I think we worked around it before by having people just log in and install the fonts manually. Should we do that again, or disable the tests?

@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Dec 31, 2015

Hrm, it says installed on both machines:

root@ip-172-31-37-44:~# sudo apt-get install msttcorefonts
Reading package lists... Done
Building dependency tree       
Reading state information... Done
Note, selecting 'ttf-mscorefonts-installer' instead of 'msttcorefonts'
ttf-mscorefonts-installer is already the newest version.
0 upgraded, 0 newly installed, 0 to remove and 86 not upgraded.
root@ip-172-31-37-44:~# 
@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Dec 31, 2015

Also, are we sure we can't use the fonts-liberation package instead to avoid this, at least for CI/testing purposes?

@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Dec 31, 2015

You can try to force a re-install to see if the eula was accidentally not accepted or not completed by doing:

sudo apt-get --purge --reinstall install ttf-mscorefonts-installer

I suspect what's happening with the rules is sometimes the eula is not agreed to, and then the package is marked as installed without having actually installed any files.

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Dec 31, 2015

I’m in favor of disabling these tests until we fix the underlying font selector issues so that Microsoft fonts are not required.

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jan 10, 2016

This is happening again

#9214 (comment)

#9209 (comment)

@edunham
Copy link
Contributor

@edunham edunham commented Jan 10, 2016

@larsbergstrom Manually reinstalling the font on a running buildslave will squash this issue for an indeterminate period of time -- unless we save the changes to a new AMI then configure Buildbot to use it, we'll hit a regression every time the slave gets terminated then restarted from the old (broken) AMI.

Is there a reason we can't perform this test successfully with free fonts? If so, we can make a new AMI with the fonts installed and EULA accepted and tell Buildbot to use it instead, though copying in that way may not even technically be permitted by the fonts' licenses.

SimonSapin added a commit that referenced this issue Jan 10, 2016
This is a bug, font fallback should be more flexible than this.

#7625
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jan 10, 2016

I’m in favor of disabling these tests until we fix the underlying font selector issues so that Microsoft fonts are not required.

#9235

bors-servo added a commit that referenced this issue Jan 10, 2016
Disable tests that fail unless non-free Microsoft fonts are installed.

This is a bug, font fallback should be more flexible than this.

#7625

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9235)
<!-- Reviewable:end -->
@metajack
Copy link
Contributor

@metajack metajack commented Jan 11, 2016

@edunham I don't understand why we can't make the automation work when the manual interaction does work. I really don't want to be using custom AMIs, since that makes everything more difficult, and harder for community members to reproduce easily.

mbrubeck added a commit to mbrubeck/servo that referenced this issue May 20, 2016
This is used as a fallback for any characters that don't have glyphs in the
specified font.  Fixes servo#7625.
mbrubeck added a commit to mbrubeck/servo that referenced this issue May 20, 2016
This is used as a fallback for any characters that don't have glyphs in the
specified font.

Fixes servo#7625.
mbrubeck added a commit to mbrubeck/servo that referenced this issue May 20, 2016
This is used as a fallback for any characters that don't have glyphs in the
specified font.

Fixes servo#7625.
@jdm
Copy link
Member Author

@jdm jdm commented Jan 8, 2020

This shouldn't affect us any longer.

@jdm jdm closed this Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.