stylo: heap write hazard from Gecko_MatchStringArgPseudo calling InitSystemMetrics

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: sfink, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
[24.05s] #50 Analyzing Gecko_MatchStringArgPseudo ...
Error: Variable assignment _ZL14sSystemMetrics$nsCSSRuleProcessor.cpp:nsTArray<nsCOMPtr<nsIAtom> >* sSystemMetrics
Location: _ZL17InitSystemMetricsv$nsCSSRuleProcessor.cpp:uint8 InitSystemMetrics() @ https://searchfox.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#1069
Stack Trace:
_ZN18nsCSSRuleProcessor15HasSystemMetricEP7nsIAtom$uint8 nsCSSRuleProcessor::HasSystemMetric(nsIAtom*) @ https://searchfox.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#1216
_ZN18nsCSSRuleProcessor19StringPseudoMatchesEPKN7mozilla3dom7ElementENS0_18CSSPseudoClassTypeEPKDsPK11nsIDocumentbNS0_11EventStatesEbPbSC_$uint8 nsCSSRuleProcessor::StringPseudoMatches(mozilla::dom::Element*, uint8, uint16*, nsIDocument*, uint8, mozilla::EventStates, uint8, uint8*, uint8*) @ https://searchfox.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#1702 ### SafeArguments: <arg6> <arg7> <arg8>
Gecko_MatchStringArgPseudo @ https://searchfox.org/mozilla-central/source/layout/style/ServoBindings.cpp#586 ### SafeArguments: <arg3>
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Assignee: nobody → manishearth
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a5b4722e254d3a1a53b1caa8ec9b5c7cf7950be

Comment 4

2 months ago
mozreview-review
Comment on attachment 8858178 [details]
Bug 1356305 - stylo: Initialize system metrics before traversing

https://reviewboard.mozilla.org/r/130118/#review132784

r=me with those two fixes.

::: gfx/thebes/gfxUserFontSet.h:178
(Diff revision 2)
>      friend class gfxUserFontEntry;
>      friend class gfxOTSContext;
>  
>  public:
>  
> -    NS_INLINE_DECL_REFCOUNTING(gfxUserFontSet)
> +    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(gfxUserFontSet)

This should go away.

::: layout/style/nsCSSRuleProcessor.cpp:1064
(Diff revision 2)
> -static bool
> -InitSystemMetrics()
> +/* static */ bool
> +nsCSSRuleProcessor::InitSystemMetrics()
>  {
>    NS_ASSERTION(!sSystemMetrics, "already initialized");
>  
>    sSystemMetrics = new nsTArray< nsCOMPtr<nsIAtom> >;
>    NS_ENSURE_TRUE(sSystemMetrics, false);

|new| is infallible now so this whole thing can become a void function.
Attachment #8858178 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 months ago
try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2a888cb48ec99fd2d9227865bc1cddeea2d5665
Comment hidden (mozreview-request)
(Reporter)

Updated

2 months ago
Blocks: 1356458
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 months ago
We can't access prefs from nsLayoutStatics. Doing it in pre traverse instead

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49452e83d081654da49eb413a32135e0c5b3ad56
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 months ago
I changed the way I'm handling this since we can't do pref access in nsLayoutStatics, need re-review.

Also, r?sfink on the analysis change. I don't think I can teach the analysis that we always call this function before traversal. Perhaps I should structure it differently (asserting that sSystemMetrics exists when off main thread)? It seems like the analysis won't understand the assert so I'd need to whitelist it anyway.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=06eb380377e1e7542f6533f3bc5c2aba4823d7bc
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 months ago
Seems to be working now.

Final try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e99c8e630e4485aa4318464d2304f1080af14d0
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8858178 - Flags: review?(sphink)

Comment 17

2 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e269a211fe7
stylo: Initialize system metrics before traversing; r=bholley
(Assignee)

Updated

2 months ago
Blocks: 1356266

Comment 18

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