System fonts exist; we should handle them.

Pushed the basic framework for system fonts and handling for font-family and font-size. I'll do the rest in the next patch. Once all are in some of the fiddly special-casing in the mako will get cleaned up.

Comment on attachment 8849887 [details] Bug 1349417 - Part 1: stylo: Factor out system font computation into nsRuleNode::ComputeSystemFont; https://reviewboard.mozilla.org/r/122630/#review125160 ::: layout/style/nsRuleNode.cpp:3581 (Diff revision 1) > + // always use 2 points smaller than what the browser has defined as > + // the default proportional font. > + // Assumption: system defined font is proportional > + aSystemFont->size = > + std::max(defaultVariableFont->size - > + nsPresContext::CSSPointsToAppUnits(2), 0); Please align this line with that after the paren.

Comment on attachment 8849888 [details] Bug 1349417 - Part 2: stylo: Add basic system font support, use for font-size and font-family; https://reviewboard.mozilla.org/r/122632/#review125164 The magic specified value should be set on all subprops of font shorthand rather than only font-family and font-size. This matters because authors can specify different value on individual subprop to override system font value. And that would also affect how we handle serializing font shorthand. ::: layout/style/ServoBindings.h:205 (Diff revision 1) > bool Gecko_ElementHasCSSAnimations(RawGeckoElementBorrowed aElement, > nsIAtom* aPseudoTagOrNull); > > // Atoms. > nsIAtom* Gecko_Atomize(const char* aString, uint32_t aLength); > +nsIAtom* Gecko_Atomize16(const nsAString* aString); This can probably be in a separate commit. ::: layout/style/ServoBindings.cpp:825 (Diff revision 1) > + nsFont* system = new (aDest) nsFont(); > + > + const nsFont* defaultVariableFont = > + aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID, > + aLang); > + *system = *defaultVariableFont; Can we just `nsFont* system = new (aDest) nsFont(*defaultVariableFont);` rather than default-init it then copy-assigning? ::: servo/components/style/properties/helpers.mako.rs:260 (Diff revision 1) > + % if property.ident in "font_size font_family".split(): > + if let Some(sf) = specified_value.get_system() { > + longhands::system_font::resolve_system_font(sf, context); > + } > + % endif This should really check all subprops of font shorthand. ::: servo/components/style/properties/longhand/font.mako.rs:1109 (Diff revision 1) > + } > + % endfor > + }; > + unsafe { > + bindings::Gecko_nsFont_InitSystem(&mut system, id as i32, > + cx.style.get_font().gecko().mLanguage.mRawPtr, Is it guaranteed that `-x-lang` gets resolved before this? ::: servo/components/style/properties/longhand/font.mako.rs:1135 (Diff revision 1) > + /// Compute and cache a system font > + /// > + /// Must be called before attempting to compute a system font > + /// specified value > + pub fn resolve_system_font(system: SystemFont, context: &mut Context) { > + if context.style.cached_system_font.is_none() { Should probably `debug_assert` that if it is not none, the cached thing matches the given system font. Probably you can store the specified enum as part of the computed struct. ::: servo/components/style/properties/shorthand/font.mako.rs:40 (Diff revision 1) > + % for name in "style variant weight stretch size_adjust kerning variant_caps variant_position".split(): > + font_${name}: font_${name}::get_initial_specified_value(), > + % endfor > + line_height: line_height::get_initial_specified_value(), These all should also have a magic value as noted above.

> The magic specified value should be set on all subprops of font shorthand rather than only font-family and font-size I know, see comment 3 :) This is a partial set of patches, I'm still working on the rest. Lots of code to update. > Is it guaranteed that `-x-lang` gets resolved before this? bug 1341775, which this is based on, forces the "early" font props (family and size) to run after all the other early props. I assumed lang was one of these, but it turns out it isn't. Will fix. > These all should also have a magic value as noted above. Yes, as part of upcoming patches :)

(In reply to Manish Goregaokar [:manishearth] from comment #9) > > The magic specified value should be set on all subprops of font shorthand rather than only font-family and font-size > > I know, see comment 3 :) > > This is a partial set of patches, I'm still working on the rest. Lots of > code to update. Oh, okay. I didn't read the bug comment before I opened the review page. Sorry about that.

Comment on attachment 8849888 [details] Bug 1349417 - Part 2: stylo: Add basic system font support, use for font-size and font-family; https://reviewboard.mozilla.org/r/122632/#review126274 ::: servo/components/style/properties/longhand/font.mako.rs:223 (Diff revision 8) > + if let SpecifiedValue::System(s) = *self { > + Some(s) > + } else { > + None > + } Maybe more compact with `match`? ::: servo/components/style/properties/longhand/font.mako.rs:241 (Diff revision 8) > + impl ToCss for SpecifiedValue { > + fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { > + match *self { > + SpecifiedValue::Values(ref v) => { > + let mut iter = v.iter(); > + try!(iter.next().unwrap().to_css(dest)); Probably use `?` syntax? ::: servo/components/style/properties/longhand/font.mako.rs:1108 (Diff revision 8) > + % for font in system_fonts: > + ${to_camel_case(font)}, > + % endfor Can we make variants `SystemFont` to have their corresponding value of `LookAndFeel::FontID` directly so that we can simply cast rather than needing an extra `match`? Or probably just let it to be an alias of `LookAndFeel_FontID`? ::: servo/components/style/properties/longhand/font.mako.rs:1121 (Diff revision 8) > + fn to_computed_value(&self, cx: &Context) -> Self::ComputedValue { > + use gecko_bindings::bindings; > + use gecko_bindings::structs::{LookAndFeel_FontID, nsFont}; > + use std::mem; > + > + let mut system: nsFont = unsafe { mem::uninitialized() }; Probably consider moving this down to just before calling `Gecko_nsFont_InitSystem` and add a comment that the function would treat this as uninitialized memory. ::: servo/components/style/properties/longhand/font.mako.rs:1124 (Diff revision 8) > + SystemFont::${to_camel_case(font)} => { > + LookAndFeel_FontID::eFont_${to_camel_case(font.replace("-moz-", ""))} > + } The indention doesn't look right. Either deindent the first two lines, or indent the last brace further. ::: servo/components/style/properties/shorthand/font.mako.rs:20 (Diff revision 8) > + use properties::longhands::font_family; > + use properties::longhands::system_font::SystemFont; Doesn't seem to me importing `font_family` is useful here. Maybe later patch? ::: servo/components/style/properties/shorthand/font.mako.rs:27 (Diff revision 8) > + % endif > % endif > % if data.testing: > use properties::longhands::font_language_override; > % endif > - use properties::longhands::font_family::SpecifiedValue as FontFamily; > + use properties::longhands::font_family::{SpecifiedValue as FontFamily}; Why this change...? Probably belongs to another commit?

Reading the spec again, and playing font shorthand a bit with Chrome, it seems to me Gecko is going the wrong way. The spec indicates that system fonts should get resolved at parse time (which is also what Chrome does). That should make lots of things more elegant and simpler: all the subproperties have meaningful value (so serialization is simpler), and no magic on cascading. Actually, it seems to me that nsRuleNode::ComputeSystemFont (factored out in part 1) doesn't rely on any other properties at all, and the only thing Gecko_nsFont_InitSystem (added in part 2) needs from the cascading is getting the default font, which we can totally avoid if we can just reject unrecognized value during parsing, or we force every platform to return something for all predefined system fonts. jfkthame, do you think we can change Gecko's behavior here?

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #44) > Reading the spec again, and playing font shorthand a bit with Chrome, it > seems to me Gecko is going the wrong way. > > The spec indicates that system fonts should get resolved at parse time FWIW, that's not entirely clear to me. Which part of the spec are you referring to here? > (which is also what Chrome does). That should make lots of things more > elegant and simpler: all the subproperties have meaningful value (so > serialization is simpler), and no magic on cascading. My initial reaction was that resolving system fonts at parse time could be problematic, because I think in theory, it could be possible for a system font name to resolve to different actual fonts depending on the language of the element where it's used. However, it's not clear to me that this ever happens in practice, as I think the various platform look-and-feel implementations end up returning a specific (non-language-dependent) font/size for all the system font values. So I suspect it would be OK after all: we could simplify the implementation to just eliminate the apparent dependency on lang altogether, and make it explicit that the system font values are resolved at parse time. But I'd suggest checking with people like :dbaron and :heycam as well before pushing ahead with something like this -- maybe worth raising it on dev.tech.layout to see if anyone foresees issues with it.

Comment on attachment 8850294 [details] Bug 1349417 - Part 3: stylo: System font support for keyword font longhands; https://reviewboard.mozilla.org/r/122886/#review132842 ::: servo/components/style/properties/data.py:16 (Diff revision 8) > +SYSTEM_FONT_LONGHANDS = """font_family font_size font_style > + font_variant_caps font_stretch font_kerning > + font_variant_position""".split() You would probably need to rebase on top of bug 1354876. ::: servo/components/style/properties/longhand/box.mako.rs:97 (Diff revision 8) > } > % endif > > ${helpers.gecko_keyword_conversion(Keyword('display', ' '.join(values), > - gecko_enum_prefix='StyleDisplay'))} > + gecko_enum_prefix='StyleDisplay', > + gecko_strip_moz_prefix=False))} This change seems to be unrelated? ::: servo/components/style/properties/longhand/font.mako.rs:267 (Diff revision 8) > - "normal italic oblique", > + "normal italic oblique", > - gecko_constant_prefix="NS_FONT_STYLE", > + gecko_constant_prefix="NS_FONT_STYLE", > - gecko_ffi_name="mFont.style", > + gecko_ffi_name="mFont.style", > - spec="https://drafts.csswg.org/css-fonts/#propdef-font-style", > + spec="https://drafts.csswg.org/css-fonts/#propdef-font-style", > - animatable=False)} > + animatable=False)} Align to the paren? ::: servo/components/style/properties/longhand/font.mako.rs:872 (Diff revision 8) > // FIXME: This prop should be animatable > -${helpers.single_keyword("font-stretch", > +${helpers.single_keyword_system("font-stretch", > - "normal ultra-condensed extra-condensed condensed \ > + "normal ultra-condensed extra-condensed condensed \ > - semi-condensed semi-expanded expanded extra-expanded \ > + semi-condensed semi-expanded expanded extra-expanded \ > - ultra-expanded", > + ultra-expanded", > + cast_to="i32", Why is the `cast_to` has a different value than `cast_type`? And should we just merge these two arguments?

Comment on attachment 8850411 [details] Bug 1349417 - Part 4: stylo: System font support for font-weight; https://reviewboard.mozilla.org/r/122998/#review132852 ::: servo/components/style/properties/longhand/font.mako.rs:381 (Diff revision 6) > % endfor > SpecifiedValue::Normal => Ok(computed_value::T::Weight400), > SpecifiedValue::Bold => Ok(computed_value::T::Weight700), > SpecifiedValue::Bolder | > - SpecifiedValue::Lighter => Err(()) > + SpecifiedValue::Lighter | > + SpecifiedValue::System(..) => Err(()), Just `unreachable!()`?

Comment on attachment 8850412 [details] Bug 1349417 - Part 5: stylo: System font support for font-size-adjust; https://reviewboard.mozilla.org/r/123000/#review132866

Comment on attachment 8850413 [details] Bug 1349417 - Part 8: stylo: Serialize system fonts correctly; https://reviewboard.mozilla.org/r/123002/#review132868 I'm not sure what effect do you want here. I think there are two ways to serialize system font: 1. if all subprops are system font, serialize the system font on font shorthand, otherwise, serialize nothing for font shorthand and just leave it to independent subprops 2. serialize font shorthand when there is any system font in subprops, and serialize non-system-font subprops afterwards The first way is easier, and probably matches what Gecko currently does, while the second way would need some more hack. However, this patch doesn't seem to fit in either of them. I suppose you want to go the first way, but the patch seems wrong to me. ::: servo/components/style/properties/shorthand/font.mako.rs:117 (Diff revision 6) > }) > } > > + % if product == "gecko": > + impl<'a> LonghandsToSerialize<'a> { > + /// You may want to remove this line. ::: servo/components/style/properties/shorthand/font.mako.rs:127 (Diff revision 6) > + if let Some(s) = self.${prop}.get_system() { > + if s != sys { > + return None > + } > + } Probably you should return `None` when any of them is not a system font? And I suppose `s` should never be different from `sys`, so probably just `debug_assert!` for that is enough? ::: servo/components/style/properties/shorthand/font.mako.rs:139 (Diff revision 6) > + return None > + } > + if self.line_height != &line_height::get_initial_specified_value() { > + return None > + } > + Some(sys) Trailing whitespace. ::: servo/components/style/properties/shorthand/font.mako.rs:147 (Diff revision 6) > + % if product == "gecko": > + if let Some(sys) = self.all_system() { > + return sys.to_css(dest); > + } > + % endif If there is any system font, but not all, probably you should also return with empty output here?

Comment on attachment 8850663 [details] Bug 1349417 - Part 9: stylo: Update test expectations; https://reviewboard.mozilla.org/r/123180/#review132882 ::: layout/style/test/stylo-failures.md:391 (Diff revision 4) > * issues arround font shorthand servo/servo#15032 servo/servo#15036 > - * test_bug377947.html [1] > + * test_bug377947.html [2] > * test_value_storage.html `'font'` [144] > * test_shorthand_property_getters.html `font shorthand` [1] > - * test_system_font_serialization.html [10] > + * test_system_font_serialization.html [6] I expect we should almost fix this section in this bug... Probably worth checking what's left here.

> I'm not sure what effect do you want here. I think there are two ways to serialize system font: Yeah, I got some things inverted there. Fixed.

Comment on attachment 8850413 [details] Bug 1349417 - Part 8: stylo: Serialize system fonts correctly; https://reviewboard.mozilla.org/r/123002/#review133208 ::: servo/components/style/properties/shorthand/font.mako.rs:132 (Diff revision 8) > + if let Some(sys) = sys { > + debug_assert!(s == sys); > + } Probably can simplify to `debug_assert!(sys.is_none() || s == sys.unwrap());`.

Comment on attachment 8850663 [details] Bug 1349417 - Part 9: stylo: Update test expectations; https://reviewboard.mozilla.org/r/123180/#review133210 The only reason currently listed in stylo-failures.md for font shorthand failures is this bug, so I still think it is better checking why the tests are still failing, which may reveal some unknown bug in the code.

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #73) > Comment on attachment 8850663 [details] > Bug 1349417 - Part 9: stylo: Update test expectations; > > https://reviewboard.mozilla.org/r/123180/#review133210 > > The only reason currently listed in stylo-failures.md for font shorthand > failures is this bug, so I still think it is better checking why the tests > are still failing, which may reveal some unknown bug in the code. You can check so via removing the corresponding line then run the test with --failure-pattern-file=stylo-failures.md and see what failures are there.

Part 7 seems to be non-trivial, and I probably shouldn't review it late at night...

The reasons behind the failures seem to be: - We don't do -moz-use-system-font . I don't think that needs to still exist - It expects us to serialize `font: system-foo; font-longhand: bar` as specified. This may need a bunch of fixups to the serialization stuff which we can do in a followup. - We don't seem to support multiplication in calc?

(In reply to Manish Goregaokar [:manishearth] from comment #76) > The reasons behind the failures seem to be: > > - We don't do -moz-use-system-font . I don't think that needs to still exist I guess we can probably fix the serialization side of this? Otherwise we should change the test instead... > - It expects us to serialize `font: system-foo; font-longhand: bar` as > specified. This may need a bunch of fixups to the serialization stuff which > we can do in a followup. Ok. > - We don't seem to support multiplication in calc? What...? I think we don't support division is a known issue, but also multiplication?

Seems like we do support multiplication. Will look closer at that test then.

Comment on attachment 8850294 [details] Bug 1349417 - Part 3: stylo: System font support for keyword font longhands; https://reviewboard.mozilla.org/r/122886/#review133332 ::: servo/components/style/properties/longhand/font.mako.rs:1941 (Diff revision 9) > }).collect::<Vec<_>>(); > let ret = ComputedSystemFont { > font_family: longhands::font_family::computed_value::T(family), > font_size: Au(system.size), > + % for kwprop in kw_font_props: > + ${kwprop}: longhands::${kwprop}::computed_value::T::from_gecko_keyword(system.style as u32), This seems to be wrong. Why all of them are from `system.style`?

Comment on attachment 8858295 [details] Bug 1349417 - Part 7: stylo: System font support for bitflag properties and font-language-override; https://reviewboard.mozilla.org/r/130270/#review133330 ::: servo/components/style/properties/longhand/font.mako.rs:1060 (Diff revision 2) > + #[derive(Debug, Clone, PartialEq)] > + pub enum SpecifiedValue { > + Value(VariantAlternates), > + System(SystemFont) > + } > + > impl ToCss for SpecifiedValue { > fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { > + match *self { > + SpecifiedValue::Value(v) => v.to_css(dest), > + SpecifiedValue::System(_) => Ok(()) > + } > + } > + } > + > + > + impl SpecifiedValue { > + pub fn system_font(f: SystemFont) -> Self { > + SpecifiedValue::System(f) > + } > + pub fn get_system(&self) -> Option<SystemFont> { > + if let SpecifiedValue::System(s) = *self { > + Some(s) > + } else { > + None > + } > + } > + } > + > + impl ToComputedValue for SpecifiedValue { > + type ComputedValue = computed_value::T; > + > + fn to_computed_value(&self, context: &Context) -> computed_value::T { > + match *self { > + SpecifiedValue::Value(v) => v, > + SpecifiedValue::System(_) => { > + <%self:nongecko_unreachable> > + context.style.cached_system_font.as_ref().unwrap().font_variant_alternates > + </%self:nongecko_unreachable> > + } > + } > + } > + > + fn from_computed_value(other: &computed_value::T) -> Self { > + SpecifiedValue::Value(*other) > + } > + } This part of code seems to be duplicate among various properties. Could we probably define a macro or helper for it? ::: servo/components/style/properties/longhand/font.mako.rs:1240 (Diff revision 2) > + #[derive(Debug, Clone, PartialEq)] > + pub enum SpecifiedValue { > + Value(VariantEastAsian), > + System(SystemFont) > + } > + > impl ToCss for SpecifiedValue { > fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { > + match *self { > + SpecifiedValue::Value(v) => v.to_css(dest), > + SpecifiedValue::System(_) => Ok(()) > + } > + } > + } > + > + > + impl SpecifiedValue { > + pub fn system_font(f: SystemFont) -> Self { > + SpecifiedValue::System(f) > + } > + pub fn get_system(&self) -> Option<SystemFont> { > + if let SpecifiedValue::System(s) = *self { > + Some(s) > + } else { > + None > + } > + } > + } > + > + impl ToComputedValue for SpecifiedValue { > + type ComputedValue = computed_value::T; > + > + fn to_computed_value(&self, context: &Context) -> computed_value::T { > + match *self { > + SpecifiedValue::Value(v) => v, > + SpecifiedValue::System(_) => { > + <%self:nongecko_unreachable> > + context.style.cached_system_font.as_ref().unwrap().font_variant_east_asian > + </%self:nongecko_unreachable> > + } > + } > + } > + > + fn from_computed_value(other: &computed_value::T) -> Self { > + SpecifiedValue::Value(*other) > + } > + } Like this. ::: servo/components/style/properties/longhand/font.mako.rs:1421 (Diff revision 2) > + #[derive(Debug, Clone, PartialEq)] > + pub enum SpecifiedValue { > + Value(VariantLigatures), > + System(SystemFont) > + } > + > impl ToCss for SpecifiedValue { > fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { > + match *self { > + SpecifiedValue::Value(v) => v.to_css(dest), > + SpecifiedValue::System(_) => Ok(()) > + } > + } > + } > + > + > + impl SpecifiedValue { > + pub fn system_font(f: SystemFont) -> Self { > + SpecifiedValue::System(f) > + } > + pub fn get_system(&self) -> Option<SystemFont> { > + if let SpecifiedValue::System(s) = *self { > + Some(s) > + } else { > + None > + } > + } > + } > + > + impl ToComputedValue for SpecifiedValue { > + type ComputedValue = computed_value::T; > + > + fn to_computed_value(&self, context: &Context) -> computed_value::T { > + match *self { > + SpecifiedValue::Value(v) => v, > + SpecifiedValue::System(_) => { > + <%self:nongecko_unreachable> > + context.style.cached_system_font.as_ref().unwrap().font_variant_ligatures > + </%self:nongecko_unreachable> > + } > + } > + } > + > + fn from_computed_value(other: &computed_value::T) -> Self { > + SpecifiedValue::Value(*other) > + } > + } And this. ::: servo/components/style/properties/longhand/font.mako.rs:1613 (Diff revision 2) > + #[derive(Debug, Clone, PartialEq)] > + pub enum SpecifiedValue { > + Value(VariantNumeric), > + System(SystemFont) > + } > + > impl ToCss for SpecifiedValue { > fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { > + match *self { > + SpecifiedValue::Value(v) => v.to_css(dest), > + SpecifiedValue::System(_) => Ok(()) > + } > + } > + } > + > + impl SpecifiedValue { > + pub fn system_font(f: SystemFont) -> Self { > + SpecifiedValue::System(f) > + } > + pub fn get_system(&self) -> Option<SystemFont> { > + if let SpecifiedValue::System(s) = *self { > + Some(s) > + } else { > + None > + } > + } > + } > + > + impl ToComputedValue for SpecifiedValue { > + type ComputedValue = computed_value::T; > + > + fn to_computed_value(&self, context: &Context) -> computed_value::T { > + match *self { > + SpecifiedValue::Value(v) => v, > + SpecifiedValue::System(_) => { > + <%self:nongecko_unreachable> > + context.style.cached_system_font.as_ref().unwrap().font_variant_numeric > + </%self:nongecko_unreachable> > + } > + } > + } > + > + fn from_computed_value(other: &computed_value::T) -> Self { > + SpecifiedValue::Value(*other) > + } > + } And this. ::: servo/components/style/properties/longhand/font.mako.rs:2296 (Diff revision 2) > + font_variant_alternates: longhands::font_variant_alternates::computed_value::T::from_gecko_keyword(system.variantAlternates), > + font_variant_ligatures: longhands::font_variant_ligatures::computed_value::T::from_gecko_keyword(system.variantLigatures), > + font_variant_east_asian: longhands::font_variant_east_asian::computed_value::T::from_gecko_keyword(system.variantEastAsian), > + font_variant_numeric: longhands::font_variant_numeric::computed_value::T::from_gecko_keyword(system.variantNumeric), > + font_language_override: longhands::font_language_override::computed_value::T(system.languageOverride), Can these be merged into the loop above somehow?

Comment on attachment 8850294 [details] Bug 1349417 - Part 3: stylo: System font support for keyword font longhands; https://reviewboard.mozilla.org/r/122886/#review133332 > This seems to be wrong. Why all of them are from `system.style`? Good catch! Fixed.

Comment on attachment 8858295 [details] Bug 1349417 - Part 7: stylo: System font support for bitflag properties and font-language-override; https://reviewboard.mozilla.org/r/130270/#review133766 ::: servo/components/style/properties/longhand/font.mako.rs:19 (Diff revision 3) > %else: > unreachable!() > %endif > </%def> > > +// Define ToComputedValue, ToCSS, and other boilerplate for a specified value `ToCss`?

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c1ead1f0ab71 Part 1: stylo: Factor out system font computation into nsRuleNode::ComputeSystemFont; r=xidorn https://hg.mozilla.org/integration/autoland/rev/fbdddabe4c8d Part 2: stylo: Add basic system font support, use for font-size and font-family; r=xidorn https://hg.mozilla.org/integration/autoland/rev/e6a9aaf8c102 Part 9: stylo: Update test expectations; r=xidorn

sorry had to back this out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92543322&repo=autoland&lineNumber=13443

Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/54e64f30caea Backed out 3 changesets for build bustage

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3586e7877b9e Part 1: stylo: Factor out system font computation into nsRuleNode::ComputeSystemFont; r=xidorn https://hg.mozilla.org/integration/autoland/rev/98094a8c0d46 Part 2: stylo: Add basic system font support, use for font-size and font-family; r=xidorn https://hg.mozilla.org/integration/autoland/rev/49c8dc08ff64 Part 9: stylo: Update test expectations; r=xidorn

should be fixed now, sorry.

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1c87f8dbd2dc Backed out changeset 49c8dc08ff64 https://hg.mozilla.org/integration/autoland/rev/89f138c550ba Backed out changeset 98094a8c0d46 https://hg.mozilla.org/integration/autoland/rev/62f2ca894a51 Backed out changeset 3586e7877b9e for hazard failures

Looks like outparams need to be annotated now :| https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d169e791ab66934bcbd6ae5a741dc68622eba0d

Hm, so annotating the outparam didn't work. The relevant code is void Gecko_nsFont_InitSystem(nsFont* aDest, int32_t aFontId, const nsStyleFont* aFont, RawGeckoPresContextBorrowed aPresContext) { const nsFont* defaultVariableFont = aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID, aFont->mLanguage); // We have passed uninitialized memory to this function, // initialize it. We can't simply return an nsFont because then // we need to know its size beforehand. Servo cannot initialize nsFont // itself, so this will do. nsFont* system = new (aDest) nsFont(*defaultVariableFont); *system = *defaultVariableFont; LookAndFeel::FontID fontID = static_cast<LookAndFeel::FontID>(aFontId); nsRuleNode::ComputeSystemFont(system, fontID, aPresContext); } nsRuleNode::ComputeSystemFont(nsFont* aSystemFont, LookAndFeel::FontID aFontID, const nsPresContext* aPresContext) { gfxFontStyle fontStyle; float devPerCSS = (float)nsPresContext::AppUnitsPerCSSPixel() / aPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom(); nsAutoString systemFontName; if (LookAndFeel::GetFont(aFontID, systemFontName, fontStyle, devPerCSS)) { systemFontName.Trim("\"'"); aSystemFont->fontlist = FontFamilyList(systemFontName, eUnquotedName); aSystemFont->fontlist.SetDefaultFontType(eFamily_none); aSystemFont->style = fontStyle.style; aSystemFont->systemFont = fontStyle.systemFont; aSystemFont->weight = fontStyle.weight; aSystemFont->stretch = fontStyle.stretch; aSystemFont->size = NSFloatPixelsToAppUnits(fontStyle.size, aPresContext->DeviceContext()-> AppUnitsPerDevPixelAtUnitFullZoom()); //aSystemFont->langGroup = fontStyle.langGroup; aSystemFont->sizeAdjust = fontStyle.sizeAdjust } where aDest is an uninitialized outparam that gets filled in by the placement new, giving the `system` nsFont (which should be the same as aDest unless allocation fails, in which case it will be null). The analysis considers the write to aSystemFont->style to be a heap write (it is, but to a whitelisted outparam). I suspect it's ignoring the placement new somehow? I tried whitelisting both `system` and `aDest` and the analysis still triggers, so I'm not sure what to do here.

I tried using `aDest` instead of `system` (both should be the same). That heap hazard went away, but there's a new one: Error: External function Location: *memchr$void* memchr(void*, int32, uint64) Stack Trace: _ZL9FindChar1PKcjiDsi$nsStringObsolete.cpp:int32 FindChar1(int8*, uint32, int32, uint16, int32) @ xpcom/string/nsStringObsolete.cpp#82 ### SafeArguments: aTrimTrailing _ZN8nsString4TrimEPKcbbb$void nsString::Trim(int8*, uint8, uint8, uint8) @ xpcom/string/nsTStringObsolete.cpp#633 ### SafeArguments: <this> aDefaultVariableFont _ZN10nsRuleNode17ComputeSystemFontEP6nsFontN7mozilla11LookAndFeel6FontIDEPK13nsPresContextPKS0_$void nsRuleNode::ComputeSystemFont(nsFont*, uint32, nsPresContext*, nsFont*) @ layout/style/nsRuleNode.cpp#3580 ### SafeArguments: aDest Gecko_nsFont_InitSystem @ layout/style/ServoBindings.cpp#1005 ### SafeArguments: <arg0> The nsAutoString is owned, not sure why this is triggering.

Chatted with sfink on IRC, I think I figured out how to bypass it. r=sfink on https://hg.mozilla.org/try/rev/325f6744afd74d28cbbea101c0ceb806ec956d68 (from IRC)

Additional patch to pass hazard analysis

Comment on attachment 8860541 [details] [diff] [review] Bug 1349417 - Part 10: Use mutex to guard system font computation; r?bholley Review of attachment 8860541 [details] [diff] [review]: ----------------------------------------------------------------- seems ok in general, though would be good to have steve take a look. That doesn't need to block landing. ::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js @@ +320,5 @@ > } > > + // We manually lock here > + if (/nsRuleNode::ComputeSystemFont/.test(callee) && > + /Gecko_nsFont_InitSystem/.test(name)) So this says to ignore hazards in ComputeSystemFont if (and only if) Gecko_nsFont_InitSystem is on the stack? That seems reasonable, I just don't know the analysis well enough to check that's what it actually does. ::: layout/style/ServoBindings.cpp @@ +70,5 @@ > #include "mozilla/ServoArcTypeList.h" > #undef SERVO_ARC_TYPE > > + > +static Mutex* sServoFontMetricsLock = nullptr; Um, I assume this gets initialized somewhere in one of the other patches? This diff seems like it's adding the lock but not initializing it...

> Um, I assume this gets initialized somewhere in one of the other patches? This diff seems like it's adding the lock but not initializing it... Yes, in InitializeServo() > So this says to ignore hazards in ComputeSystemFont if (and only if) Gecko_nsFont_InitSystem is on the stack? I think if and only if the caller is InitSystem, it can't be further up the stack. So yeah, this is fine.

Comment on attachment 8860541 [details] [diff] [review] Bug 1349417 - Part 10: Use mutex to guard system font computation; r?bholley Review of attachment 8860541 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js @@ +320,5 @@ > } > > + // We manually lock here > + if (/nsRuleNode::ComputeSystemFont/.test(callee) && > + /Gecko_nsFont_InitSystem/.test(name)) I know all the other ones here are doing these sloppy unanchored regexp matches, but the Gecko_ functions have C linkage and are exact-matched elsewhere, so I'd prefer if this were name == "Gecko_nsFont_InitSystem"

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fb358c9dd073 Part 1: stylo: Factor out system font computation into nsRuleNode::ComputeSystemFont; r=xidorn https://hg.mozilla.org/integration/autoland/rev/6430444ade07 Part 2: stylo: Add basic system font support, use for font-size and font-family; r=xidorn https://hg.mozilla.org/integration/autoland/rev/c798ce122e10 Part 9: stylo: Update test expectations; r=xidorn https://hg.mozilla.org/integration/autoland/rev/f6a7287bdc63 Part 10: Use mutex to guard system font computation; r=bholley https://hg.mozilla.org/integration/autoland/rev/7a7df2395851 Placement new should preserve safety, r=bhackett

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0a18e825015b Fixup system font heap analysis ; r=bustage

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/91767db31b3f Backed out changeset 7a7df2395851 https://hg.mozilla.org/integration/autoland/rev/184dc928df68 Ignore all calls within InitSystem; r=bustage

I never ended up using the placement new fix, but I included it in my land since it would get forgotten otherwise. This created some spurious failures, so I backed it out. We can figure out why that happened later. See https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7a7df2395851bc9934a2c7bc4ff6bc782c5cfc48&filter-searchStr=33cbe96a7540f88d211c7d912395dea68f975e22 for the failures introduced.

https://hg.mozilla.org/mozilla-central/rev/fb358c9dd073 https://hg.mozilla.org/mozilla-central/rev/6430444ade07 https://hg.mozilla.org/mozilla-central/rev/c798ce122e10 https://hg.mozilla.org/mozilla-central/rev/f6a7287bdc63 https://hg.mozilla.org/mozilla-central/rev/0a18e825015b https://hg.mozilla.org/mozilla-central/rev/184dc928df68

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc1739bf2de Placement new should preserve safety, r=bhackett