This was found by bhackett's analysis in bug 1294915. We're reading the global frame property table, which sets mLastFrame without synchronization while iterating children in StyleChildrenIterator. Error: Field write mozilla::FramePropertyTable.mLastFrame Location: void* mozilla::FramePropertyTable::GetInternal(nsIFrame*, mozilla::FramePropertyDescriptorUntyped*, uint8*) @ layout/base/FramePropertyTable.cpp:73 Stack Trace: mozilla::FramePropertyTable::PropertyType<T> mozilla::FramePropertyTable::Get(const nsIFrame*, mozilla::FramePropertyTable::Descriptor<T>, bool*) [with T = nsAbsoluteContainingBlock; mozilla::FramePropertyTable::PropertyType<T> = nsAbsoluteContainingBlock*; mozilla::FramePropertyTable::Descriptor<T> = const mozilla::FramePropertyDescriptor<nsAbsoluteContainingBlock>*] @ layout/base/FramePropertyTable.h:215 mozilla::FrameProperties::PropertyType<T> mozilla::FrameProperties::Get(mozilla::FrameProperties::Descriptor<T>, bool*) const [with T = nsAbsoluteContainingBlock; mozilla::FrameProperties::PropertyType<T> = nsAbsoluteContainingBlock*; mozilla::FrameProperties::Descriptor<T> = const mozilla::FramePropertyDescriptor<nsAbsoluteContainingBlock>*] @ layout/base/FramePropertyTable.h:421 nsAbsoluteContainingBlock* nsIFrame::GetAbsoluteContainingBlock() const @ layout/generic/nsFrame.cpp:257 nsFrameList* nsFrame::GetChildList(uint32) const @ layout/generic/nsFrame.cpp:1493 nsFrameList* nsContainerFrame::GetChildList(uint32) const @ layout/generic/nsContainerFrame.cpp:278 nsFrameList* nsBlockFrame::GetChildList(uint32) const @ layout/generic/nsBlockFrame.cpp:585 nsFrameList* nsComboboxControlFrame::GetChildList(uint32) const @ layout/forms/nsComboboxControlFrame.cpp:1427 nsIFrame* nsLayoutUtils::GetAfterFrameForContent(nsIFrame*, nsIContent*) @ layout/base/nsLayoutUtils.cpp:1583 nsIFrame* nsLayoutUtils::GetAfterFrame(nsIFrame*) @ layout/base/nsLayoutUtils.cpp:1595 nsIContent* mozilla::dom::AllChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp:451 nsIContent* mozilla::dom::StyleChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp:593 Gecko_GetNextStyleChild @ layout/style/ServoBindings.cpp:159 This seems to happen for the ::before and ::after pseudos (that StyleChildrenIterator filters out), so probably the way to fix this is just fixing bug 1296065. The best way to fix

(In reply to Emilio Cobos Álvarez [:emilio] from comment #0) > This seems to happen for the ::before and ::after pseudos (that > StyleChildrenIterator filters out), so probably the way to fix this is just > fixing bug 1296065. > The best way to fix Unfortunately I think we're going to stop filtering them out, so we'll need a different solution here.

Comment on attachment 8829323 [details] [diff] [review] Avoid writing to shared data in FramePropertyTable during parallel traversal. v1 Not sure if the perf hit of the TLS lookup here is acceptable. Hopefully it is. If it isn't, my next idea would be to bubble an aIsMainThread boolean up the callstack until we reach a level from which the check is acceptable. Brian, is there some way we can satisfy the analysis with the code in this patch? We could explicitly whitelist the writes, or somehow teach it to recognize when code is in an NS_IsMainThread conditional block.

Try push with talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88c42f353b9a66636c8735786ed7075f5d2cf22d

We should probably add thread assertions to the setters too.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3) > Brian, is there some way we can satisfy the analysis with the code in this > patch? We could explicitly whitelist the writes, or somehow teach it to > recognize when code is in an NS_IsMainThread conditional block. Yes, it would be easy to modify the analysis to ignore code that only executes when NS_IsMainThread returns true (this would work for both assignments and called functions dominated by the call).

So in terms of the performance implications.... The extra TLS lookup is only in the miss case, when we already had the cost of the GetEntry() call. So chances are it's not really that big a deal, in the sense that GetEntry probably takes a lot more time than NS_IsMainThread(), though it might be worth verifying this. A bigger question is what this means in practice for the _number_ of misses. This only matters on the parallel styling threads, obviously.

Comment on attachment 8829323 [details] [diff] [review] Avoid writing to shared data in FramePropertyTable during parallel traversal. v1 r=me, I think, but would still be good to have those perf numbers...

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5) > We should probably add thread assertions to the setters too. Good idea, I'll add some. (In reply to Brian Hackett (:bhackett) from comment #6) > Yes, it would be easy to modify the analysis to ignore code that only > executes when NS_IsMainThread returns true (this would work for both > assignments and called functions dominated by the call). \o/

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7) > The extra TLS lookup is only in the miss case, when we already had the cost > of the GetEntry() call. So chances are it's not really that big a deal, in > the sense that GetEntry probably takes a lot more time than > NS_IsMainThread(), though it might be worth verifying this. > > A bigger question is what this means in practice for the _number_ of misses. > This only matters on the parallel styling threads, obviously. Probably not a big deal. We only get here with the StyleChildrenIterator, which is already a slow path. I think most of the hot paths are probably from gecko layout. (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #8) > r=me, I think, but would still be good to have those perf numbers... Try push in comment 4 shows no significant deltas.

Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/264cbcf1b37f Avoid writing to shared data in FramePropertyTable during parallel traversal. r=bz

I think relying on try to catch performance anything is probably useless, unless you introduce really huge slowdowns throughout the code. A more useful metric would be a testcase that exercises the cache miss case of this code as much as we can manage and seeing how much _that_ regresses. That is, what is the worst-case impact?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #12) > I think relying on try to catch performance anything is probably useless, > unless you introduce really huge slowdowns throughout the code. > > A more useful metric would be a testcase that exercises the cache miss case > of this code as much as we can manage and seeing how much _that_ regresses. > That is, what is the worst-case impact? I agree in theory, but given the reasoning in comment 7, that investigation doesn't feel like a very high priority to me, unless you feel otherwise.

That's fair. As long as you're confident that we're not ending up with much larger numbers of misses and all.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14) > That's fair. As long as you're confident that we're not ending up with much > larger numbers of misses and all. Well, we'd only end up with misses in the servo traversal case, and we will see it in the profiles if it's a problem.