1365937 - hazard builds with stylo enabled fail due to write hazards

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Nathan Froyd [:froydnj]]

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=87f25c5d32bdf5337a1a25ec09b5b488179f9a1f&selectedJob=99942866 failing thus:

TEST-UNEXPECTED-FAIL 5 heap write hazards detected out of 4 allowed | undefined

The write hazards are:

[26.64s] #56 Analyzing Gecko_MatchStringArgPseudo ...
Error: External function
Location: Servo_DeclarationBlock_GetCssText ### SafeArguments: <arg1>
Stack Trace:
_ZNK7mozilla21ServoDeclarationBlock8ToStringER9nsAString$void mozilla::ServoDeclarationBlock::ToString(nsAString*) const @ obj-analyzed/dist/include/mozilla/ServoDeclarationBlock.h#54 ### SafeArguments: aString
_ZNK7mozilla16DeclarationBlock8ToStringER9nsAString$void mozilla::DeclarationBlock::ToString(nsAString*) const @ obj-analyzed/dist/include/mozilla/DeclarationBlockInlines.h#58 ### SafeArguments: aResult
_ZNK11nsAttrValue8ToStringER9nsAString$void nsAttrValue::ToString(nsAString*) const @ dom/base/nsAttrValue.cpp#648 ### SafeArguments: aResult
_ZNK11nsAttrValue8ToStringERN7mozilla3dom9DOMStringE$void nsAttrValue::ToString(mozilla::dom::DOMString*) const @ obj-analyzed/dist/include/nsAttrValue.h#539 ### SafeArguments: aNameSpaceID
_ZNK7mozilla3dom7Element7GetAttrEiP7nsIAtomRNS0_9DOMStringE$uint8 mozilla::dom::Element::GetAttr(int32, nsIAtom*, mozilla::dom::DOMString*) const @ obj-analyzed/dist/include/mozilla/dom/Element.h#749 ### SafeArguments: aNameSpaceID aResult
_ZNK7mozilla3dom7Element7GetAttrEiP7nsIAtomR9nsAString$uint8 mozilla::dom::Element::GetAttr(int32, nsIAtom*, nsAString*) const @ dom/base/Element.cpp#2667 ### SafeArguments: aNameSpaceID aResult
_ZNK10nsIContent7GetAttrEiP7nsIAtomR9nsAString$uint8 nsIContent::GetAttr(int32, nsIAtom*, nsAString*) const @ dom/base/FragmentOrElement.cpp#989 ### SafeArguments: aResult <arg2>
_ZNK10nsIContent7GetLangER9nsAString$uint8 nsIContent::GetLang(nsAString*) const @ obj-analyzed/dist/include/nsIContent.h#939 ### SafeArguments: aElement
_ZN18nsCSSRuleProcessor19StringPseudoMatchesEPKN7mozilla3dom7ElementENS0_18CSSPseudoClassTypeEPKDsPK11nsIDocumentbNS0_11EventStatesEPbSC_$uint8 nsCSSRuleProcessor::StringPseudoMatches(mozilla::dom::Element*, uint8, uint16*, nsIDocument*, uint8, mozilla::EventStates, uint8*, uint8*) @ layout/style/nsCSSRuleProcessor.cpp#1782 ### SafeArguments: <arg6> <arg7>
Gecko_MatchStringArgPseudo @ layout/style/ServoBindings.cpp#707 ### SafeArguments: <arg3>

[27.02s] #99 Analyzing Gecko_SetGradientImageValue ...
Error: AddRef/Release on nsISupports
Location: _ZN12nsStyleImage7SetNullEv$void nsStyleImage::SetNull() @ layout/style/nsStyleStruct.cpp#2262 ### SafeArguments: this
Stack Trace:
_ZN12nsStyleImage15SetGradientDataEP15nsStyleGradient$void nsStyleImage::SetGradientData(nsStyleGradient*) @ layout/style/nsStyleStruct.cpp#2297 ### SafeArguments: <this>
Gecko_SetGradientImageValue @ layout/style/ServoBindings.cpp#1159 ### SafeArguments: <arg0>

[27.24s] #171 Analyzing Gecko_CSSValue_SetNumber ...
Error: AddRef/Release on nsISupports
Location: _ZN10nsCSSValue7DoResetEv$void nsCSSValue::DoReset() @ layout/style/nsCSSValue.cpp#459 ### SafeArguments: this
Stack Trace:
_ZN10nsCSSValue5ResetEv$void nsCSSValue::Reset() @ obj-analyzed/dist/include/nsCSSValue.h#888 ### SafeArguments: this
_ZN10nsCSSValue13SetFloatValueEf9nsCSSUnit$void nsCSSValue::SetFloatValue(float32, uint32) @ layout/style/nsCSSValue.cpp#490 ### SafeArguments: <this>
Gecko_CSSValue_SetNumber @ layout/style/ServoBindings.cpp#1790 ### SafeArguments: <arg0>

[27.28s] #195 Analyzing Gecko_nsStyleFont_FixupNoneGeneric ...
Error: Field write mozilla::FontFamilyList.mDefaultFontType
Location: _ZN7mozilla14FontFamilyList18SetDefaultFontTypeENS_14FontFamilyTypeE$void mozilla::FontFamilyList::SetDefaultFontType(uint32) @ obj-analyzed/dist/include/gfxFontFamilyList.h#335 ### SafeArguments: aFont
Stack Trace:
_ZN10nsRuleNode16FixupNoneGenericEP6nsFontPK13nsPresContexthPKS0_$void nsRuleNode::FixupNoneGeneric(nsFont*, nsPresContext*, uint8, nsFont*) @ layout/style/nsRuleNode.cpp#442
Gecko_nsStyleFont_FixupNoneGeneric @ layout/style/ServoBindings.cpp#1970

[27.29s] #196 Analyzing Gecko_GetBaseSize ...
Error: Field write nsFont.size
Location: _ZN7mozilla18LangGroupFontPrefs10InitializeEP7nsIAtom$void mozilla::LangGroupFontPrefs::Initialize(nsIAtom*) @ layout/base/StaticPresData.cpp#199 ### SafeArguments: <this>
Stack Trace:
Gecko_GetBaseSize @ layout/style/ServoBindings.cpp#1991

bholley, do you know what's going on here?

Comment 1

[Bobby Holley (:bholley)]

Don't have time to look right now, but my guess is that there's something that was conditionally compiled that's adding a hazard. We should stop conditionally compiling it and adjust the hazard threshold appropriately.

Comment 2

[Manish Goregaokar [:manishearth]]

Probably need to mark Servo_DeclarationBlock_GetCssText as a safe external function.

Comment 3

[Chris Peterson [:cpeterson]]

Manish, can you take a look at this Stylo hazard bug? It's one of the remaining blockers for building Stylo by default and Nathan is busy with the libclang configure work.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1365937 - Mark Servo_DeclarationBlock_GetCssText as threadsafe;

Review commit: https://reviewboard.mozilla.org/r/141598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141598/

Comment 5

[Manish Goregaokar [:manishearth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10c573bca54de2f296ec240702b5212c7024130a

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8870150 [details]
Bug 1365937 - Mark Servo_DeclarationBlock_GetCssText as threadsafe;

https://reviewboard.mozilla.org/r/141598/#review145264

r=me, I guess, with the below fixed.

::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js:29
(Diff revision 1)
>          "fmod",
>          "floor",
>          "ceil",
>          /memchr/,
>          "strlen",
> +        /Servo_DeclarationBlock_GetCssText/,

Shouldn't this be just `"Servo_DeclarationBlock_GetCssText"`, since such functions are all `extern "C"`?

Comment 7

[Manish Goregaokar [:manishearth]]

> Shouldn't this be just `"Servo_DeclarationBlock_GetCssText"`, since such functions are all `extern "C"`?

I think the analysis gets it as _Servo.... or something. There was trouble with this last time, regex is better.

Comment 8

[Manish Goregaokar [:manishearth]]

Hazard build passes. There are a billion test failures due to the previous patches, though, might want to look into those.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10c573bca54de2f296ec240702b5212c7024130a

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c41adb3ead5a
Mark Servo_DeclarationBlock_GetCssText as threadsafe; r=froydnj

Comment 10

[Chris Peterson [:cpeterson]]

Thanks, Manish!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c41adb3ead5a