1633486 - Hit MOZ_CRASH(Should have a suitable from value to use) at servo/ports/geckolib/glue.rs:529

Status: VERIFIED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 17aa41e3cb7c (built with --enable-debug).

Hit MOZ_CRASH(Should have a suitable from value to use) at servo/ports/geckolib/glue.rs:529

rax = 0x0000562557c9fa48   rdx = 0x0000000000000000
rcx = 0x0000000000000b40   rbx = 0x00007fa9380bdbca
rsi = 0x00007fa939e928b0   rdi = 0x00007fa939e91680
rbp = 0x00007fa9380bd9b0   rsp = 0x00007fa9380bd9a0
r8 = 0x00007fa939e928b0    r9 = 0x00007fa9380bf700
r10 = 0x0000000000000000   r11 = 0x0000000000000000
r12 = 0x0000000000000028   r13 = 0x000000000000001c
r14 = 0x0000000000000211   r15 = 0x00007fa92998b1ca
rip = 0x00007fa926de70c3
OS|Linux|0.0.0 Linux 5.3.0-46-generic #38~18.04.1-Ubuntu SMP Tue Mar 31 04:17:56 UTC 2020 x86_64
CPU|amd64|family 6 model 94 stepping 3|8
GPU|||
Crash|SIGSEGV|0x0|2
2|0|libxul.so|RustMozCrash|hg:hg.mozilla.org/mozilla-central:mozglue/static/rust/wrappers.cpp:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|17|0x15
2|1|libxul.so|mozglue_static::panic_hook|hg:hg.mozilla.org/mozilla-central:mozglue/static/rust/lib.rs:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|89|0x9
2|2|libxul.so|core::ops::function::Fn::call|git:github.com/rust-lang/rust:src/libcore/ops/function.rs:f3e1a954d2ead4e2fc197c7da7d71e6c61bad196|72|0xc
2|3|libxul.so|std::panicking::rust_panic_with_hook|git:github.com/rust-lang/rust:src/libstd/panicking.rs:f3e1a954d2ead4e2fc197c7da7d71e6c61bad196|475|0x6
2|4|libxul.so|std::panicking::begin_panic|git:github.com/rust-lang/rust:src/libstd/panicking.rs:f3e1a954d2ead4e2fc197c7da7d71e6c61bad196|404|0x8
2|5|libxul.so|geckoservo::glue::compose_animation_segment|hg:hg.mozilla.org/mozilla-central:servo/ports/geckolib/glue.rs:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|529|0x1f
2|6|libxul.so|Servo_ComposeAnimationSegment|hg:hg.mozilla.org/mozilla-central:servo/ports/geckolib/glue.rs:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|601|0x8
2|7|libxul.so|mozilla::layers::AnimationHelper::SampleAnimationForEachNode(mozilla::TimeStamp, mozilla::TimeStamp, mozilla::layers::AnimatedValue const*, nsTArray<mozilla::layers::PropertyAnimationGroup>&, nsTArray<RefPtr<RawServoAnimationValue> >&)|hg:hg.mozilla.org/mozilla-central:gfx/layers/AnimationHelper.cpp:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|283|0x1c
2|8|libxul.so|mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer *, (lambda at /builds/worker/checkouts/gecko/gfx/layers/composite/AsyncCompositionManager.cpp:648:40), (lambda at /builds/worker/checkouts/gecko/gfx/layers/TreeTraversal.h:166:44)>|hg:hg.mozilla.org/mozilla-central:gfx/layers/TreeTraversal.h:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|139|0x8d
2|9|libxul.so|mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer *, (lambda at /builds/worker/checkouts/gecko/gfx/layers/composite/AsyncCompositionManager.cpp:648:40), (lambda at /builds/worker/checkouts/gecko/gfx/layers/TreeTraversal.h:166:44)>|hg:hg.mozilla.org/mozilla-central:gfx/layers/TreeTraversal.h:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|143|0xb
2|10|libxul.so|mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer *, (lambda at /builds/worker/checkouts/gecko/gfx/layers/composite/AsyncCompositionManager.cpp:648:40), (lambda at /builds/worker/checkouts/gecko/gfx/layers/TreeTraversal.h:166:44)>|hg:hg.mozilla.org/mozilla-central:gfx/layers/TreeTraversal.h:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|143|0xb
2|11|libxul.so|mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer *, (lambda at /builds/worker/checkouts/gecko/gfx/layers/composite/AsyncCompositionManager.cpp:648:40), (lambda at /builds/worker/checkouts/gecko/gfx/layers/TreeTraversal.h:166:44)>|hg:hg.mozilla.org/mozilla-central:gfx/layers/TreeTraversal.h:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|143|0xb
2|12|libxul.so|mozilla::layers::AsyncCompositionManager::TransformShadowTree(mozilla::TimeStamp, mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>, mozilla::layers::CompositorBridgeParentBase::TransformsToSkip)|hg:hg.mozilla.org/mozilla-central:gfx/layers/composite/AsyncCompositionManager.cpp:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|1450|0x26
2|13|libxul.so|mozilla::layers::CompositorBridgeParent::CompositeToTarget(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::gfx::DrawTarget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*)|hg:hg.mozilla.org/mozilla-central:gfx/layers/ipc/CompositorBridgeParent.cpp:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|1021|0x1c
2|14|libxul.so|mozilla::layers::CompositorVsyncScheduler::Composite(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:gfx/layers/ipc/CompositorVsyncScheduler.cpp:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|249|0x21
2|15|libxul.so|mozilla::detail::RunnableMethodImpl<mozilla::layers::CompositorVsyncScheduler*, void (mozilla::layers::CompositorVsyncScheduler::*)(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp), true, (mozilla::RunnableKind)1, mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp>::Run()|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.h:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|1220|0x50
2|16|libxul.so|MessageLoop::RunTask(already_AddRefed<nsIRunnable>)|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|442|0x11
2|17|libxul.so|MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&)|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|450|0x27
2|18|libxul.so|MessageLoop::DoWork()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|523|0xb
2|19|libxul.so|base::MessagePumpDefault::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_pump_default.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|35|0x9
2|20|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|315|0x17
2|21|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|290|0x8
2|22|libxul.so|base::Thread::ThreadMain()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/thread.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|192|0x8
2|23|libxul.so|ThreadFunc(void*)|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/platform_thread_posix.cc:17aa41e3cb7cdff3b94e26e351e29cc8b9bab18a|40|0x6
2|24|libpthread.so.0||||0x76db
2|25|libc.so.6||||0x12188f

Comment 1

[Mayank Bansal]

Also happens with WR

Comment 2

[Boris Chiou [:boris]]

All I know is: if we disable the pref of the implicit-keyframe, we don't hit this crash. Perhaps we don't handle well in some cases.

Comment 3

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200428100141-a99c73301874.
The bug appears to have been introduced in the following build range:
> Start: df596657bebcb96b917d75ff452316bbe8140a1a (20200218213359)
> End: 28cf163158a673037d20ccc1aa7b825e406e927b (20200219043403)
> Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df596657bebcb96b917d75ff452316bbe8140a1a&tochange=28cf163158a673037d20ccc1aa7b825e406e927b

Comment 4

[Boris Chiou [:boris]]

The test case creates an animation with all properties by implicit keyframes. I tried to make it simpler: only create opacity: ['initial'], and I still can reproduce this issue. It seems opacity shouldn't be applied to ::marker pseudo element, so I suspect that's why we can not get an underlying value for opacity on ::marker.

Comment 5

[Boris Chiou [:boris]]

Anyway, looks like for a normal animation on ::marker, we should have a valid underlying value. However, this case doesn't have underlying value, so I got a lot of warnings or a moz crash on opacity or color property. So where is the underlying value? Perhaps the change from hidden to unhidden is the reason. We perhaps need to handle this special case.

Comment 6

[Boris Chiou [:boris]]

Looks like this crash happens when the following conditions meet:

1. The animation property (e.g. opacity) is running on the compositor
2. EnsureBaseStyle() cannot get the animation target because this pseudo-element is hidden when we call EnsureBaseStyle(). (Therefore KeyframeEffect::mBaseValues is empty.)
3. After we unhide the element, we don't call EnsureBaseStyle() again, and then we send the null base style and the animation info into the compositor, and we try to sample animations between null fromValue and a valid toValue, without underlying value (i.e. base style), so we hit this release crash.

Comment 7

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #6)

Looks like this crash happens when the following conditions meet:

1. The animation property (e.g. opacity) is running on the compositor
2. EnsureBaseStyle() cannot get the animation target because this pseudo-element is hidden when we call EnsureBaseStyle(). (Therefore KeyframeEffect::mBaseValues is empty.)
3. After we unhide the element, we don't call EnsureBaseStyle() again, and then we send the null base style and the animation info into the compositor, and we try to sample animations between null fromValue and a valid toValue, without underlying value (i.e. base style), so we hit this release crash.

And I'm surprised this only happens on ::marker. If I use ::before, we call EnsureBaseStyle again after unhidding the element.

Comment 8

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #7)

And I'm surprised this only happens on ::marker. If I use ::before, we call EnsureBaseStyle again after unhidding the element.

Looks like when we unhide the element, we construct the generated content, e.g. ::marker, and then traverse its style subtree. However, it seems we don't update the animation rules during this traversal.

If we are animating on ::before or ::after, we update the animation rules during traversing the style subtree after constructing the generated content. This is the difference I notice. I need to dig deeper into this.

Comment 9

[Boris Chiou [:boris]]

I suspect we have to update something like this:
https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/servo/components/style/gecko/wrapper.rs#1449

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1633486 - Add ::marker when checking may_have_animations() during traversal.

When unhidding a ::marker element, we construct its generated item, and
then call StyleNewSubtree() on this generated item. During traversal, we
may update any animation related values in Gecko_UpdateAnimations(), which
may update the base styles for animation properties.

The test case is an animation segment from "null" to "inital" value. We
replace the "null" value with the base style value for the specific animation
property, so we can do interpolation properly.
(e.g. opacity: "null => initial" becomes "1.0 => initial")
If we don't update the animation related values in
Gecko_UpdateAnimations after generating ::marker, we may do
interpolation from "null" to "initial", which causes a panic.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12d2219afdcd
Add ::marker when checking may_have_animations() during traversal. r=emilio

Comment 12

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/12d2219afdcd

Comment 13

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200518152416-a627b6676824.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

:boris, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.