1350244 - stylo: Hit MOZ_CRASH(Array not threadsafe) in layout/reftests/bugs/652301-1b.html

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Emilio Cobos Álvarez (:emilio)]

STR: With my patches in bug 1350140:

  ./mach run layout/reftests/bugs/652301-1b.html

Stack:

#0  0x00007fffe19b17ff in nsCSSValue::Array::Release() (this=0x7fffcf915e80) at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/include/nsCSSValue.h:1150
#1  0x00007fffe4008291 in nsCSSValue::DoReset() (this=0x7fffcf912360) at /home/emilio/projects/moz/stylo/layout/style/nsCSSValue.cpp:445
#2  0x00007fffe19b16ce in nsCSSValue::Reset() (this=0x7fffcf912360) at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/include/nsCSSValue.h:905
#3  0x00007fffe19b16a8 in nsCSSValue::~nsCSSValue() (this=0x7fffcf912360, __in_chrg=<optimized out>) at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/include/nsCSSValue.h:616
#4  0x00007fffe400f021 in nsCSSValueList::~nsCSSValueList() (this=0x7fffcf912360, __in_chrg=<optimized out>) at /home/emilio/projects/moz/stylo/layout/style/nsCSSValue.cpp:2320
#5  0x00007fffe400f819 in nsCSSValueSharedList::~nsCSSValueSharedList() (this=0x7fffcf914080, __in_chrg=<optimized out>) at /home/emilio/projects/moz/stylo/layout/style/nsCSSValue.cpp:2522
#6  0x00007fffe1a89fc0 in nsCSSValueSharedList::Release() (this=0x7fffcf914080) at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/include/nsCSSValue.h:1222
#7  0x00007fffe1aa06d5 in mozilla::RefPtrTraits<nsCSSValueSharedList>::Release(nsCSSValueSharedList*) (aPtr=0x7fffcf914080)
    at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:40
#8  0x00007fffe1a977b5 in RefPtr<nsCSSValueSharedList>::ConstRemovingRefPtrTraits<nsCSSValueSharedList>::Release(nsCSSValueSharedList*) (aPtr=0x7fffcf914080)
    at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:395
#9  0x00007fffe1a90b99 in RefPtr<nsCSSValueSharedList>::~RefPtr() (this=0x7fffcf91e3c8, __in_chrg=<optimized out>)
    at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:78
#10 0x00007fffe4091fee in nsStyleDisplay::~nsStyleDisplay() (this=0x7fffcf91e350, __in_chrg=<optimized out>) at /home/emilio/projects/moz/stylo/layout/style/nsStyleStruct.cpp:3380
#11 0x00007fffe3f671ec in Gecko_Destroy_nsStyleDisplay(nsStyleDisplay*) (ptr=0x7fffcf91e350) at /home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/layout/style/nsStyleStructList.h:98
[..]
#33 0x00007fffe41454b6 in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, nsStyleContext*, mozilla::ServoStyleSet*, nsStyleChangeList&) (this=0x7fffc3cd7f60, aElem
ent=0x7fffc0b83080, aParentContext=0x7fffb2051a18, aStyleSet=0x7fffc3ca9c50, aChangeList=...) at /home/emilio/projects/moz/stylo/layout/base/ServoRestyleManager.cpp:180
#34 0x00007fffe41453dd in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, nsStyleContext*, mozilla::ServoStyleSet*, nsStyleChangeList&) (this=0x7fffc3cd7f60, aElem
ent=0x7fffd0b08230, aParentContext=0x7fffb2051918, aStyleSet=0x7fffc3ca9c50, aChangeList=...) at /home/emilio/projects/moz/stylo/layout/base/ServoRestyleManager.cpp:292
#35 0x00007fffe41453dd in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, nsStyleContext*, mozilla::ServoStyleSet*, nsStyleChangeList&) (this=0x7fffc3cd7f60, aElem
ent=0x7fffd073bf20, aParentContext=0x7fffb200c438, aStyleSet=0x7fffc3ca9c50, aChangeList=...) at /home/emilio/projects/moz/stylo/layout/base/ServoRestyleManager.cpp:292
#36 0x00007fffe41453dd in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, nsStyleContext*, mozilla::ServoStyleSet*, nsStyleChangeList&) (this=0x7fffc3cd7f60, aElem
ent=0x7fffc07e7380, aParentContext=0x7fffb2004cb8, aStyleSet=0x7fffc3ca9c50, aChangeList=...) at /home/emilio/projects/moz/stylo/layout/base/ServoRestyleManager.cpp:292
#37 0x00007fffe41453dd in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, nsStyleContext*, mozilla::ServoStyleSet*, nsStyleChangeList&) (this=0x7fffc3cd7f60, aElem
ent=0x7fffc0a324c0, aParentContext=0x0, aStyleSet=0x7fffc3ca9c50, aChangeList=...) at /home/emilio/projects/moz/stylo/layout/base/ServoRestyleManager.cpp:292
#38 0x00007fffe41458a7 in mozilla::ServoRestyleManager::ProcessPendingRestyles() (this=0x7fffc3cd7f60) at /home/emilio/projects/moz/stylo/layout/base/ServoRestyleManager.cpp:398
#39 0x00007fffe414df24 in mozilla::RestyleManager::ProcessPendingRestyles() (this=0x7fffc3cd7f60)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Seems under ProcessPostTraversal, where it's fine to destroy them. Bobby, should we assert a different thing there?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This seems to happen only locally btw, not sure what's going on on buildbots.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Presumably the harness ends before the flush that causes us to ProcessPendingRestyles happens.

Comment 4

[Bobby Holley (:bholley)]

So we're hitting this:

http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/xpcom/base/nsISupportsImpl.h#66

This broke with bug 1348606, because I changed Array to use the standard refcount machinery, which has owner-thread tracking. And so when we (presumably) synthesize this array on a style worker, the owner thread is the style thread. And this doesn't show up on CI, because CI doesn't run the parallel traversal.

The workaround for now is STYLO_THREADS=1

Comment 5

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24982f413b64842b4130665d449f98b2a7af4e6d

Comment 6

[Bobby Holley (:bholley)]

Attachment: For Reference: Use a ThreadSafeArray for eCSSUnit_Function. WIP

So, got most of the way through this one, but I think it's starting to get
unwieldy enough that it's worth throwing in the towel, backing out bug
1348606, and just making all the array refcounts threadsafe.

MozReview-Commit-ID: L0njGSaX2xy

Comment 7

[Bobby Holley (:bholley)]

Attachment: Use threadsafe refcounts for all nsCSSValue arrays. v1

MozReview-Commit-ID: AfEqX8btQnz

This patch assumes bug 1350244 is backed out except for the patch to switch
to the normal refcounting macro.

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b4a04b279d20a830ff11fdaa7c172551b9a4148

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8851742 [details] [diff] [review]
Use threadsafe refcounts for all nsCSSValue arrays. v1

I'm unsure about the performance indication of this. How much slower would a thread-safe refcount be comparing to a owner-thread-only refcount?

The patch is probably fine, but I'd rather defer the decision to heycam...

Comment 10

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> Comment on attachment 8851742 [details] [diff] [review]
> Use threadsafe refcounts for all nsCSSValue arrays. v1
> 
> I'm unsure about the performance indication of this. How much slower would a
> thread-safe refcount be comparing to a owner-thread-only refcount?

The answer in general is that it goes from "maybe an l1 cache miss" to ~5ns on an i7.

With the previous patches plus the abandoned WIP patch in this bug, we end up changing about half of the overall callsites to ThreadSafeArray. Changing all of them seemed like a worthwhile price to pay to avoid all the complexity of having two possible array representations.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8851742 [details] [diff] [review]
Use threadsafe refcounts for all nsCSSValue arrays. v1

Review of attachment 8851742 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think this is probably fine.

I'm not sure which bug comment 7 meant to refer to.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #11)
> Comment on attachment 8851742 [details] [diff] [review]
> Use threadsafe refcounts for all nsCSSValue arrays. v1
> 
> Review of attachment 8851742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, I think this is probably fine.
> 
> I'm not sure which bug comment 7 meant to refer to.

Sorry, I meant bug 1348606.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bd66bfcf13e
Back out bug 1348606 part 5 (Use a threadsafe array for counters). r=me
https://hg.mozilla.org/integration/autoland/rev/650053787c73
Back out bug 1348606 part 4 (Turn nsCSSValue::Array into a macro). r=me
https://hg.mozilla.org/integration/autoland/rev/25519c41d637
Back out bug 1348606 part 3 (Inline nsCSSValue::Array::SizeOfIncludingThis). r=me
https://hg.mozilla.org/integration/autoland/rev/bcf5f54a5744
Back out bug 1348606 part 2 (Get rid of CSSVALUE_LIST_FOR_EXTRA_VALUES). r=me
https://hg.mozilla.org/integration/autoland/rev/14f17c5bf865
Use threadsafe refcounts for all nsCSSValue arrays. r=heycam

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bd66bfcf13e
https://hg.mozilla.org/mozilla-central/rev/650053787c73
https://hg.mozilla.org/mozilla-central/rev/25519c41d637
https://hg.mozilla.org/mozilla-central/rev/bcf5f54a5744
https://hg.mozilla.org/mozilla-central/rev/14f17c5bf865