1143537 - crash in nsRuleNode::ComputeFontFeatures

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Daggett (:jtd)]

crashreport:
https://crash-stats.mozilla.com/report/index/f7734a46-7941-4429-a735-089402150316

stack:
nsRuleNode::ComputeFontFeatures(nsCSSValuePairList const*, nsTArray<gfxFontFeature>&) 	layout/style/nsCSSValue.h
nsStyleUtil::AppendFontFeatureSettings(nsCSSValue const&, nsAString_internal&) 	layout/style/nsStyleUtil.cpp
mozilla::dom::FontFace::GetFeatureSettings(nsString&) 	layout/style/FontFace.cpp
mozilla::dom::FontFaceBinding::get_featureSettings 	obj-firefox/x86_64/dom/bindings/FontFaceBinding.cpp
mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp
js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
ApplyOrCall 	js/src/vm/Debugger.cpp
DebuggerObject_call 	js/src/vm/Debugger.cpp
js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const 	js/src/proxy/DirectProxyHandler.cpp
js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/Proxy.cpp
js::proxy_Call(JSContext*, unsigned int, JS::Value*) 	js/src/proxy/Proxy.cpp
js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
js::jit::DoCallFallback 	js/src/jit/BaselineIC.cpp

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92ffbe6e2233

Comment 2

[Cameron McCormack (:heycam)]

I'd like to make unicode-range serialize without leading zeroes.  Is that acceptable?  This would make serialization of the default value (U+0-1FFFF) consistent with how that default value is written in the css-fonts-3 spec as well as the default value of the unicodeRange FontFaceDescriptors dictionary member.  Otherwise, the default value will be serialized differently depending on whether the value came from the underlying @font-face rule.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> Otherwise, the default value will be serialized differently
> depending on whether the value came from the underlying @font-face rule.

Actually that's not quite right; when missing from the @font-face rule, inspecting the rule using getPropertyValue will return the empty string.  Still, I think it would be good to be consistent.

Comment 4

[Cameron McCormack (:heycam)]

FWIW, Chrome will serialize the unicode-range descriptor without leading zeroes.

Comment 5

[John Daggett (:jtd)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> I'd like to make unicode-range serialize without leading zeroes.  Is that
> acceptable?  This would make serialization of the default value (U+0-1FFFF)
> consistent with how that default value is written in the css-fonts-3 spec as
> well as the default value of the unicodeRange FontFaceDescriptors dictionary
> member.  Otherwise, the default value will be serialized differently
> depending on whether the value came from the underlying @font-face rule.

Yup, that's reasonable.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 1: Fix test numbering.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 2: Test descriptor getters for CSS-connected FontFaces when the descriptor isn't present.

This test asserts without the rest of the patch queue applied.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 3: Serialize unicode-range without leading zeroes.

The try run showed failures in test_font_face_parser.html which I've fixed here and tested locally.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 4: Serialize FontFace unicodeRange and fontFeatureSettings with proper defaults.

Comment 10

[John Daggett (:jtd)]

Comment on attachment 8578371 [details] [diff] [review]
Part 2: Test descriptor getters for CSS-connected FontFaces when the descriptor isn't present.

r+ with changes below.

layout/style/test/test_font_loading_api.html:

>+    // (TEST 34) Test that descriptor getters for unspecified descriptors on
>+    // CSS-connected FontFace objects return their default values.
>+    var style = document.querySelector("style");
>+    var ruleText = "@font-face { font-family: something; src: url(x); }";
>+
>+    style.textContent = ruleText;
>+    document.body.offsetTop;

As noted on IRC, I think it would be good to omit these, assuming the font set is built synchronously.

I also think it would be useful to test the status of both the FontFace object and the FontFaceSet.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to John Daggett (:jtd) from comment #10)
> As noted on IRC, I think it would be good to omit these, assuming the font
> set is built synchronously.
> 
> I also think it would be useful to test the status of both the FontFace
> object and the FontFaceSet.

Bug 1143995 removes the flushes, so I'll just land that at the same time.  I had filed bug 1144003 for the status tests, but I'll just land it as part of this bug (with your implied r+).

Comment 13

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75bb9aa83c5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6171df7b4dbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fcd63998e56
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c18f83985f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d306f1d4537a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75bb9aa83c5e
https://hg.mozilla.org/mozilla-central/rev/6171df7b4dbf
https://hg.mozilla.org/mozilla-central/rev/2fcd63998e56
https://hg.mozilla.org/mozilla-central/rev/4c18f83985f7
https://hg.mozilla.org/mozilla-central/rev/d306f1d4537a