Not handling "missing font-family" very gracefully

[caitp]

This seems to occur in places where "font-family" is specified.

On my mac, "about-mozilla.html" renders correctly (it uses font-family: serif; in several places)

However, "ligatures.html" fails to render when specifying font-family: Calibri;

I haven't dug in very deep yet, but it seems like maybe falling back on a default font would be a more correct or safe behaviour than just dying.

I'll be able to look at this a bit more next week if it's still open

---

I should add, this is not limited to the mac, dholbert had run into this on linux x64 as well (died testing about-mozilla.html)

rust: task failed at 'assertion failed: fonts.len() > 0', /Users/caitp/git/servo/src/components/servo-gfx/font_context.rs:130
rust: domain main @0x7fba3980c010 root task failed
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: task failed at 'killed', /Users/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
rust: ~"tracing 4430275392:"
rust: ~"tracing first child"
rust: ~"tracing last child"
rust: ~"tracing 4430275440:"
rust: ~"tracing parent"
rust: ~"tracing first child"
rust: ~"tracing last child"
rust: ~"tracing 4430275728:"
rust: ~"tracing parent"
rust: ~"tracing first child"
rust: ~"tracing last child"
rust: ~"tracing prev sibling"
rust: ~"tracing 4430275920:"
rust: ~"tracing parent"
rust: ~"tracing prev sibling"
rust: ~"tracing 4430275824:"
rust: ~"tracing parent"
rust: ~"tracing first child"
rust: ~"tracing last child"
rust: ~"tracing next sibling"
rust: ~"tracing prev sibling"
rust: ~"tracing 4430275872:"
rust: ~"tracing parent"
rust: ~"tracing 4430275776:"
rust: ~"tracing parent"
rust: ~"tracing next sibling"
rust: ~"tracing 4430275488:"
rust: ~"tracing parent"
rust: ~"tracing first child"
rust: ~"tracing last child"
rust: ~"tracing next sibling"
rust: ~"tracing 4430275680:"
rust: ~"tracing parent"
rust: ~"tracing prev sibling"
rust: ~"tracing 4430275584:"
rust: ~"tracing parent"
rust: ~"tracing first child"
rust: ~"tracing last child"
rust: ~"tracing next sibling"
rust: ~"tracing prev sibling"
rust: ~"tracing 4430275632:"
rust: ~"tracing parent"
rust: ~"tracing 4430275536:"
rust: ~"tracing parent"
rust: ~"tracing next sibling"
Assertion failed: (!borrow_list), function delete_this, file /Users/caitp/git/servo/src/compiler/rust/src/rt/rust_task.cpp, line 80.

[caitp]

caitp@sunbeach:~/git/servo/build$ uname -a
Linux sunbeach 3.8.0-22-generic #33-Ubuntu SMP Thu May 16 15:17:14 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux
caitp@sunbeach:~/git/servo/build$ export RUST_LOG=servo_gfx
caitp@sunbeach:~/git/servo/build$ ./servo ../src/test/ligatures.html 2> log.txt

yields the following:

...
...
...

rust: ~"(create font group) transformed family is `Calibri`"
rust: ~"next_bucket(39, 256) = 40"
rust: ~"FontList: Couldn\'t find font family with name=Calibri"
rust: ~"FontList: Couldn\'t find font face in family[Calibri] matching style"
rust: ~"(create font group) didn\'t find `Calibri`"
rust: task failed at 'assertion failed: fonts.len() > 0', /home/caitp/git/servo/src/components/servo-gfx/font_context.rs:133
rust: task failed at 'connection closed', /home/caitp/git/servo/src/compiler/rust/src/libcore/option.rs:343
rust: task failed at 'option::get none', /home/caitp/git/servo/src/compiler/rust/src/libcore/option.rs:367
terminate called after throwing an instance of 'rust: domain main @0x7fc1d4201c80 root task failed
rust_task*'
rust: task failed at 'killed', /home/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281
Redirecting call to abort() to mozalloc_abort
rust: task failed at 'killed', /home/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281

rust: task failed at 'killed', /home/caitp/git/servo/src/compiler/rust/src/libcore/pipes.rs:281

I mean, we knew that this is where the failure was happening, but you know.

I'm wondering what the right way to select a fallback font would be? It would probably make sense to just fall-back on the top-level CSS (eg html.css in Firefox) which should hopefully point to a "safe" font, but maybe there is a better way, or else something specifically asked for in the CSS3 draft (if there is, I'm not aware of it, since I don't know too much about CSS)

[burg]

The spec is here: http://www.w3.org/TR/CSS2/fonts.html

Briefly speaking, for each text run, the described algorithm comes up with an ordered list of preferred font faces. (the group is implemented as FontGroup in Servo, and created by FontContext::create_font_group.) When rendering, if a glyph is not available in the first font face of the list, try with the next font.

The last font in the list is supposed to be a font "of last resort", hence the assertion that the font list is nonzero in length. Somewhere in create_font_group we need to append a last-resort font. This should be delegated to the various platform-specific font list implementations, because the last-resort font differs by platform. To see what gecko uses, this is a good starting point:

http://dxr.mozilla.org/search?q=resort%20path%3Agfx%2Fthebes&tree=mozilla-central

[caitp]

This is just a preliminary commit -- As you can see there's no fallback fonts added for the linux platform (I wasn't sure how to translate the Gecko behaviour http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFT2Fonts.cpp#303) to work nicely here.

Further, I couldn't get it to find "Arial Unicode US" on my mac, so for now I've just changed this to "Arial" -- A better choice can be found later I guess.

It might make sense to provide multiple fallbacks, in which case returning a comma-separated list should be simple enough, or maybe there is a better strategy for this.

@burg if you have time I think it would be worth getting some suggestions from you on this since you sound like you know your way around this stuff.

[caitp]

The msttcorefonts package doesn't provide much (eg the GNU Unifont, which might make a nice fallback on systems where it's available -- at least in terms of representing the entire BMP and also denoting which planes/scripts other characters fall into, which is nice). So for now I've just set this to fall back on Arial.

I still don't really think this is the right way to do things, but it does get around the nasty fonts.len() assertion failure when a requested font-family isn't found.

[burg]

Looks good in general.

I would prefer that we pass a list of FontFamily instances instead of a string, since there's no guarantee the string corresponds to an actual font family in the FontList map. At the same time, we don't want platform handles to keep wrappers alive. So, maybe the compromise for now is to pass a list of font family strings, and debug assert that they actually correspond to FontFamily instances.

The Mac fallback should be Arial Unicode MS, (not US). Hardcoding seems ok as a first fix.

Since fallback fonts are UA-specific, each platform could implement this using platform-specific font matching systems. For example, CTFontDescriptor can be used to specify font attributes and the library will provide the best match. Fontconfig on linux has similar behavior. (Normally we bypass this matching since font matching is defined by CSS; in this case the behavior is undefined so we might as well use the matching machinery.) I recommend hardcoding some values that prevent crashes, then filing a followup issue to make it smarter.

[mcepl]

Just to say that on Linux, one usually uses "serif" and "sans-serif" aliases as a fallback.

[burg]

@caitp looks like you need to rebase

[caitp]

Yeah I'll be rebasing this today -- In response to @mcepl: It looks like the issue Daniel Holbert encountered when testing on Linux x86_64 was that we failed to resolve a font for font-family: serif (about-mozilla.html uses this). This happened during my testing as well, so "serif" does not seem to be very safe, necessarily. Looks like it only seems to find fonts provided by msttcorefonts (at least, those are the only family names I've had any luck getting picked up).

For sure there are more problems than just the "not setting up a last-resort", I think, eg

// TODO(Issue #192: handle generic font families, like 'serif' and 'sans-serif'.

Seems to not be happening yet, at least not for platform=linux

[caitp]

More will need to be done on this at some point, but I think this works as an initial fix -- I would prefer if the arrays of family-names could live somewhere and be modified by user prefs, but this seems to "work" for now at least.

[mcepl]

@caitp

font-family: serif (about-mozilla.html uses this).

I am not sure whether serif (as default hard-coded value in CSS standard) is same as "serif" (alias provided by Xorg or fontconfig).

Looks like it only seems to find fonts provided by msttcorefonts

It used to be (many years ago, when I still used it) default on Debian, and it is quite certainly still present on Fedora/RHEL. But yes, it might be distro-specific.

[caitp]

@mcepl Well, we are "supposed" to be using fontconfig to figure out these aliases on linux builds, but this does not seem to actually be happening currently. or rather, it does not seem to be "working". Having a non-aliased common font as a fallback is probably a good idea.

But I sort of agree that using font-family names as fallbacks is kind of not the best strategy, it would be better to hand back full font entries so that we can guarantee that they actually exist. I think, anyway.

[caitp]

code-style is a bit screwed up on that last patch, will submit a quick fix

[jdm]

@caitp: it turns out that buildbot flips out when encountering commit messages >1024 characters in length. Would you mind amending that for now?

[caitp]

@jdm -- My commit messages may have slightly extended beyond the usual 50 characters, but none of them were longer than 1 line... that sounds like a different problem!

Nevertheless I've squashed the commits together, so hopefully that will solve the problem.

... Oh, I see... bors-servo is adding all of the issue text into the commit message. Weird.

[caitp]

Will open a second PR to fix this, so that the issue text is removed.

See #511

[jdm]

r+

[bors-servo]

saw approval from jdm
at caitp@24ef69d

[bors-servo]

merging caitp/servo/issue-479 = 24ef69d into auto

[bors-servo]

caitp/servo/issue-479 = 24ef69d merged ok, testing candidate = 3203b33

[bors-servo]

all tests pass:
http://buildbot-master-servo-01.srv.servo.releng.use1.mozilla.com:8001/builders/linux/builds/54
http://buildbot-master-servo-01.srv.servo.releng.use1.mozilla.com:8001/builders/mac/builds/8

[bors-servo]

fast-forwarding master to auto = 3203b33