Closed
Bug 1365937
Opened 7 years ago
Closed 7 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•7 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•7 years ago
|
||
Probably need to mark Servo_DeclarationBlock_GetCssText as a safe external function.
Flags: needinfo?(manishearth)
Updated•7 years ago
|
Priority: -- → P1
Comment 3•7 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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10c573bca54de2f296ec240702b5212c7024130a
Flags: needinfo?(manishearth)
![]() |
Reporter | |
Comment 6•7 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•7 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•7 years ago
|
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 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•7 years ago
|
||
Thanks, Manish!
status-firefox53:
--- → unaffected
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c41adb3ead5a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•