hazard builds with stylo enabled fail due to write hazards

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: froydnj, Assigned: manishearth)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
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?
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley) → needinfo?(manishearth)
(Assignee)

Comment 2

9 months ago
Probably need to mark Servo_DeclarationBlock_GetCssText as a safe external function.
Flags: needinfo?(manishearth)
Blocks: 1356991
No longer blocks: 1330412
Priority: -- → P1
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.
Flags: needinfo?(manishearth)
Comment hidden (mozreview-request)
(Reporter)

Comment 6

9 months ago
mozreview-review
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"`?
Attachment #8870150 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 7

9 months ago
> 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.
(Assignee)

Updated

9 months ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
(Assignee)

Comment 8

9 months ago
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

9 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c41adb3ead5a
Mark Servo_DeclarationBlock_GetCssText as threadsafe; r=froydnj
Thanks, Manish!
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c41adb3ead5a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.