Closed Bug 1399006 Opened 3 years ago Closed 3 years ago

stylo: Fatal Assertion dropping CachedBorderImageData during servo traversal

Categories

(Core :: ImageLib, defect, P2, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: tsmith, Assigned: vliu)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase, Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

Attached file test_case.html
#0 0x51b557 in MOZ_CrashOOL /src/mfbt/Assertions.cpp:33:3
#1 0x7f3653bce2f2 in nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const /src/xpcom/base/nsISupportsImpl.cpp:43:5
#2 0x7f3656264ac8 in AssertOwnership<29> /src/obj-firefox/dist/include/nsISupportsImpl.h:69:5
#3 0x7f3656264ac8 in Release /src/image/ImageWrapper.cpp:128
#4 0x7f3656264ac8 in mozilla::image::ClippedImage::Release() /src/image/ClippedImage.cpp:213
#5 0x7f3653c14427 in ReleaseObjects /src/xpcom/ds/nsCOMArray.cpp:272:5
#6 0x7f3653c14427 in nsCOMArray_base::Clear() /src/xpcom/ds/nsCOMArray.cpp:281
#7 0x7f3653c28a4e in nsCOMArray_base::~nsCOMArray_base() /src/xpcom/ds/nsCOMArray.cpp:52:3
#8 0x7f365a8472a6 in ~CachedBorderImageData /src/obj-firefox/dist/include/nsStyleStruct.h:399:8
#9 0x7f365a8472a6 in operator() /src/obj-firefox/dist/include/mozilla/UniquePtr.h:528
#10 0x7f365a8472a6 in reset /src/obj-firefox/dist/include/mozilla/UniquePtr.h:343
#11 0x7f365a8472a6 in ~UniquePtr /src/obj-firefox/dist/include/mozilla/UniquePtr.h:288
#12 0x7f365a8472a6 in nsStyleImage::~nsStyleImage() /src/layout/style/nsStyleStruct.cpp:2250
#13 0x7f365a836d70 in nsStyleBorder::~nsStyleBorder() /src/layout/style/nsStyleStruct.cpp:408:1
#14 0x7f3660299571 in style::gecko_properties::{{impl}}::drop /src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-dc7c941fc56784c9/out/gecko_properties.rs:22064
#15 0x7f3660299571 in core::ptr::drop_in_place<style::gecko_bindings::structs::root::mozilla::GeckoBorder> /checkout/src/libcore/ptr.rs:60
#16 0x7f3660299571 in core::ptr::drop_in_place<servo_arc::ArcInner<style::gecko_bindings::structs::root::mozilla::GeckoBorder>> /checkout/src/libcore/ptr.rs:60
#17 0x7f3660299571 in core::ptr::drop_in_place<alloc::boxed::Box<servo_arc::ArcInner<style::gecko_bindings::structs::root::mozilla::GeckoBorder>>> /checkout/src/libcore/ptr.rs:60
#18 0x7f3660299571 in _$LT$servo_arc..Arc$LT$T$GT$$GT$::drop_slow::h0fb47fbba26bb271 /src/servo/components/servo_arc/lib.rs:245
#19 0x7f3660298f5a in servo_arc::{{impl}}::drop<style::gecko_bindings::structs::root::mozilla::GeckoBorder> /src/servo/components/servo_arc/lib.rs:379
#20 0x7f3660298f5a in core::ptr::drop_in_place<servo_arc::Arc<style::gecko_bindings::structs::root::mozilla::GeckoBorder>> /checkout/src/libcore/ptr.rs:60
#21 0x7f3660298f5a in servo_arc::{{impl}}::drop<style::gecko_bindings::structs::root::mozilla::GeckoBorder> /src/servo/components/servo_arc/lib.rs:775
#22 0x7f3660298f5a in core::ptr::drop_in_place<servo_arc::RawOffsetArc<style::gecko_bindings::structs::root::mozilla::GeckoBorder>> /checkout/src/libcore/ptr.rs:60
#23 0x7f3660298f5a in core::ptr::drop_in_place<style::gecko_bindings::structs::root::ServoComputedData> /checkout/src/libcore/ptr.rs:60
#24 0x7f3660298f5a in core::ptr::drop_in_place<style::gecko_bindings::structs::root::mozilla::ServoStyleContext> /checkout/src/libcore/ptr.rs:60
#25 0x7f3660298f5a in core::ptr::drop_in_place<style::gecko_properties::ComputedValues> /checkout/src/libcore/ptr.rs:60
#26 0x7f3660298f5a in core::ptr::drop_in_place<servo_arc::ArcInner<style::gecko_properties::ComputedValues>> /checkout/src/libcore/ptr.rs:60
#27 0x7f3660298f5a in core::ptr::drop_in_place<alloc::boxed::Box<servo_arc::ArcInner<style::gecko_properties::ComputedValues>>> /checkout/src/libcore/ptr.rs:60
#28 0x7f3660298f5a in _$LT$servo_arc..Arc$LT$T$GT$$GT$::drop_slow::h0a5fa88d824d76d8 /src/servo/components/servo_arc/lib.rs:245
#29 0x7f366028e238 in servo_arc::{{impl}}::drop<style::gecko_properties::ComputedValues> /src/servo/components/servo_arc/lib.rs:379
#30 0x7f366028e238 in core::ptr::drop_in_place<servo_arc::Arc<style::gecko_properties::ComputedValues>> /checkout/src/libcore/ptr.rs:60
#31 0x7f366028e238 in style::matching::MatchMethods::finish_restyle<style::gecko::wrapper::GeckoElement> /src/servo/components/style/matching.rs:667
#32 0x7f366028e238 in style::traversal::compute_style::hf61de7b9017a79f1 /src/servo/components/style/traversal.rs:726
#33 0x7f3660296082 in style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure> /src/servo/components/style/traversal.rs:477
#34 0x7f3660296082 in style::gecko::traversal::{{impl}}::process_preorder<closure> /src/servo/components/style/gecko/traversal.rs:37
#35 0x7f3660296082 in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> /src/servo/components/style/parallel.rs:188
#36 0x7f3660296082 in style::parallel::traverse_nodes::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,collections::vec_deque::Drain<style::dom::SendNode<style::gecko::wrapper::GeckoNode>>> /src/servo/components/style/parallel.rs:282
#37 0x7f3660296082 in rayon_core::scope::{{impl}}::execute_job_closure::{{closure}}<closure,()> /src/third_party/rust/rayon-core/src/scope/mod.rs:354
#38 0x7f3660296082 in std::panic::{{impl}}::call_once<(),closure> /checkout/src/libstd/panic.rs:296
#39 0x7f3660296082 in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> /checkout/src/libstd/panicking.rs:454
#40 0x7f3660296082 in panic_abort::__rust_maybe_catch_panic /checkout/src/libpanic_abort/lib.rs:40
#41 0x7f3660296082 in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> /checkout/src/libstd/panicking.rs:433
#42 0x7f3660296082 in std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> /checkout/src/libstd/panic.rs:361
#43 0x7f3660296082 in rayon_core::unwind::halt_unwinding<closure,()> /src/third_party/rust/rayon-core/src/unwind.rs:19
#44 0x7f3660296082 in rayon_core::scope::{{impl}}::execute_job_closure<closure,()> /src/third_party/rust/rayon-core/src/scope/mod.rs:354
#45 0x7f3660296082 in rayon_core::scope::{{impl}}::execute_job<closure> /src/third_party/rust/rayon-core/src/scope/mod.rs:343
#46 0x7f3660296082 in rayon_core::scope::{{impl}}::spawn::{{closure}}<closure> /src/third_party/rust/rayon-core/src/scope/mod.rs:274
#47 0x7f3660296082 in _$LT$rayon_core..job..HeapJob$LT$BODY$GT$$u20$as$u20$rayon_core..job..Job$GT$::execute::he7e1344a87a39d18 /src/third_party/rust/rayon-core/src/job.rs:142
#48 0x7f36607aa729 in rayon_core::job::{{impl}}::execute /src/third_party/rust/rayon-core/src/job.rs:55
#49 0x7f36607aa729 in rayon_core::registry::{{impl}}::execute /src/third_party/rust/rayon-core/src/registry.rs:476
#50 0x7f36607aa729 in rayon_core::registry::{{impl}}::wait_until_cold<rayon_core::latch::CountLatch> /src/third_party/rust/rayon-core/src/registry.rs:460
#51 0x7f36607aa729 in rayon_core::registry::WorkerThread::wait_until::he55fc0dee82c0e2c /src/third_party/rust/rayon-core/src/registry.rs:436
#52 0x7f36607aa2e9 in rayon_core::registry::main_loop /src/third_party/rust/rayon-core/src/registry.rs:559
#53 0x7f36607aa2e9 in rayon_core::registry::{{impl}}::new::{{closure}} /src/third_party/rust/rayon-core/src/registry.rs:145
#54 0x7f36607aa2e9 in std::sys_common::backtrace::__rust_begin_short_backtrace::h5fc46c8445d7f702 /checkout/src/libstd/sys_common/backtrace.rs:136
#55 0x7f36607aa0a9 in std::thread::{{impl}}::spawn::{{closure}}::{{closure}}<closure,()> /checkout/src/libstd/thread/mod.rs:364
#56 0x7f36607aa0a9 in std::panic::{{impl}}::call_once<(),closure> /checkout/src/libstd/panic.rs:296
#57 0x7f36607aa0a9 in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> /checkout/src/libstd/panicking.rs:454
#58 0x7f36607aa0a9 in panic_abort::__rust_maybe_catch_panic /checkout/src/libpanic_abort/lib.rs:40
#59 0x7f36607aa0a9 in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> /checkout/src/libstd/panicking.rs:433
#60 0x7f36607aa0a9 in std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> /checkout/src/libstd/panic.rs:361
#61 0x7f36607aa0a9 in std::thread::{{impl}}::spawn::{{closure}}<closure,()> /checkout/src/libstd/thread/mod.rs:363
#62 0x7f36607aa0a9 in _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h8c2f63f0700774f4 /checkout/src/liballoc/boxed.rs:648
#63 0x7f36607ffcf3 in alloc::boxed::{{impl}}::call_once<(),()> /checkout/src/liballoc/boxed.rs:658
#64 0x7f36607ffcf3 in std::sys_common::thread::start_thread /checkout/src/libstd/sys_common/thread.rs:21
#65 0x7f36607ffcf3 in std::sys::imp::thread::Thread::new::thread_start::h227b2afaa9316a8d /checkout/src/libstd/sys/unix/thread.rs:84
#66 0x7f36727646b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
#67 0x7f36717ed3dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Flags: in-testsuite?
I couldn't reproduce, but the stack looks more like stylo is releasing objects on the wrong thread?
Attached file assertion-bt.log
I can reproduce this issue with the latest build in my local Mac. The attached file was the back trace I hit. I will take some time to get more information.
From looked into this, ImageWrapper was constructed in main thread but was released in stylo thread. ImageWrapper isn't a thread-safe object so nsAutoOwningThread was detected for this. Maybe we should.

1. Find out why the release flow run in the wrong thread. Or.
2. Post to main-thread to destruct ImageWrapper.
Assignee: nobody → vliu
Blocks: 1388995
Whiteboard: [gfx-noted]
(In reply to Vincent Liu[:vliu] from comment #3)
> From looked into this, ImageWrapper was constructed in main thread but was
> released in stylo thread. ImageWrapper isn't a thread-safe object so
> nsAutoOwningThread was detected for this. Maybe we should.
> 
> 1. Find out why the release flow run in the wrong thread. Or.
> 2. Post to main-thread to destruct ImageWrapper.

I have to correct the previous information that the construction/releasing were both in style thread. Sorry about that.
RasterImage (and VectorImage, I think?) needs to be freed on the main thread. If it is possible that the ImageWrapper contains the last reference to the underlying RasterImage/VectorImage, it will also need to free its reference on the main thread. This is easily solved with a proxy release in ~ImageWrapper, but something to keep in mind here.
In general I think we defer image resolution for computed values to the main thread. Dropping _mostly_ happens on the main thread (because the style is held alive by the frame, which is updated during the post-traversal), but there are situations where we might not have a frame.

So we probably want to check IsInServoTraversal() in the nsStyleImage destructor, and use NS_ReleaseOnMainThread if so.
Priority: -- → P2
Summary: MOZ_CrashOOL in [@ Release] → stylo: Fatal Assertion dropping CachedBorderImageData during servo traversal
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> In general I think we defer image resolution for computed values to the main
> thread. Dropping _mostly_ happens on the main thread (because the style is
> held alive by the frame, which is updated during the post-traversal), but
> there are situations where we might not have a frame.
> 
> So we probably want to check IsInServoTraversal() in the nsStyleImage
> destructor, and use NS_ReleaseOnMainThread if so.

Thanks for the suggestion. From the current understanding, if I move the UniquePtr mCachedBIData into main-thread to release called in the nsStyleImage destructor, it should be fixed. I will have a patch soon to fix it.
(In reply to Vincent Liu[:vliu] from comment #7)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> > In general I think we defer image resolution for computed values to the main
> > thread. Dropping _mostly_ happens on the main thread (because the style is
> > held alive by the frame, which is updated during the post-traversal), but
> > there are situations where we might not have a frame.
> > 
> > So we probably want to check IsInServoTraversal() in the nsStyleImage
> > destructor, and use NS_ReleaseOnMainThread if so.
> 
> Thanks for the suggestion. From the current understanding, if I move the
> UniquePtr mCachedBIData into main-thread to release called in the
> nsStyleImage destructor, it should be fixed. I will have a patch soon to fix
> it.

Wouldn't it be easier to just have ~CachedBorderImageData invoke PurgeCachedImages, which already handles the OMT case?
(Thanks for taking this btw - to be clear, we need this fix in before the branch on Thursday)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)

> 
> Wouldn't it be easier to just have ~CachedBorderImageData invoke
> PurgeCachedImages, which already handles the OMT case?

Thanks for the good suggestion. Could you please have a review?
Attachment #8909574 - Flags: review?(bobbyholley)
Attachment #8909574 - Flags: review?(bobbyholley) → review+
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa2260faa05
Invoke PurgeCachedImages to release object in main thread while ServoTraversal. r=bholley
https://hg.mozilla.org/mozilla-central/rev/5fa2260faa05
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Can we land a test for this?
Flags: needinfo?(vliu)
We should add test_case.html as 1399006.html to layout/style/crashtests/crashtests.list
Hi bobby,

Can you please help to review the patch for crash test? Thanks
Flags: needinfo?(vliu)
Attachment #8911034 - Flags: review?(bobbyholley)
Comment on attachment 8911034 [details] [diff] [review]
0001-Bug-1399006-Add-crashtest.-r-bholley.patch

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

::: layout/style/crashtests/crashtests.list
@@ +218,5 @@
>  load 1397091.html
>  load 1398479.html
>  load 1398581.html
>  load 1400035.html
>  load 1399546.html

Nit: while you are here, could you swap 1399546.html and 1400035.html and added your test accordingly so that they are in order?
Attachment #8911034 - Flags: review?(bobbyholley) → review+
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #17)
> Comment on attachment 8911034 [details] [diff] [review]

> Nit: while you are here, could you swap 1399546.html and 1400035.html and
> added your test accordingly so that they are in order?

Thanks for the suggestions. The modified patch was attached with nit and r+ carried.
Attachment #8911034 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.