Closed
Bug 1365937
Opened 8 years ago
Closed 8 years ago
hazard builds with stylo enabled fail due to write hazards
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox53 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | fixed |
People
(Reporter: froydnj, Assigned: manishearth)
References
Details
Attachments
(1 file)
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)
Comment 1•8 years ago
|
||
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•8 years ago
|
||
Probably need to mark Servo_DeclarationBlock_GetCssText as a safe external function.
Flags: needinfo?(manishearth)
Updated•8 years ago
|
Priority: -- → P1
Comment 3•8 years ago
|
||
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) |
Assignee | ||
Comment 5•8 years ago
|
||
Flags: needinfo?(manishearth)
Reporter | ||
Comment 6•8 years 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•8 years 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•8 years ago
|
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•8 years 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
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c41adb3ead5a
Mark Servo_DeclarationBlock_GetCssText as threadsafe; r=froydnj
Comment 10•8 years ago
|
||
Thanks, Manish!
status-firefox53:
--- → unaffected
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
Comment 11•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•