1358965 - stylo: Crash [@ alloc::arc::Arc<style::gecko_properties::GeckoBackground>::inner<style::gecko_properties::GeckoBackground> [arc.rs : 445 + 0x5]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

Found via topsite tests and reproduced on latest Stylo debugBuild from taskcluster

Steps to reproduce:
-> Load http://www.visitmaldives.com
--> Crash on load

Non Stylo Builds are fine, so i guess its stylo bug

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

I see functions like Servo_GetComputedKeyframeValues in the stack. Guessing it's something related to animation. ni? hiro.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

This is a case that we call UpdateEffectProperties() for element which got in display:none subtree.  We need null check for this case.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for null computed values.

We need null check for aOldComputedValues since the value might have been
cleared after we create a SequentialTask for EffectProperties or
CSSTransitions because the targe element got in display:none subtree.
Even so Gecko_UpdateAnimations is called because we need to stop CSS
animations in display:none subtree.

Review commit: https://reviewboard.mozilla.org/r/132848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132848/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860854 [details]
Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for null computed values.

https://reviewboard.mozilla.org/r/132848/#review135658

::: commit-message-73752:6
(Diff revision 1)
> +Even so Gecko_UpdateAnimations is called because we need to stop CSS
> +animations in display:none subtree.

I think we can also stop transitions in display:none subtree here (for most cases), but Boris does not think we don't need to do it here (see 1341372 comment 63).

Comment 5

[Hiroyuki Ikezoe (:hiro)]

This is a good opportunity to remove silly my mistake (redundant static_cast there).

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1358965 - Drop unnecessary static_cast.

Review commit: https://reviewboard.mozilla.org/r/132862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132862/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860854 [details]
Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for null computed values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132848/diff/1-2/

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8860857 [details]
Bug 1358965 - Drop unnecessary static_cast.

https://reviewboard.mozilla.org/r/132862/#review135986

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8860854 [details]
Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for null computed values.

https://reviewboard.mozilla.org/r/132848/#review135990

::: commit-message-73752:3
(Diff revision 2)
> +We need null check for aOldComputedValues since the value might have been
> +cleared after we create a SequentialTask for EffectProperties or
> +CSSTransitions because the targe element got in display:none subtree.
> +Even so Gecko_UpdateAnimations is called because we need to stop CSS
> +animations in display:none subtree.

(We don't need this here if we have the same comment in the patch itself.)

::: layout/style/ServoBindings.cpp:487
(Diff revision 2)
>          UpdateAnimations(const_cast<dom::Element*>(aElement), pseudoType,
>                           servoValues);
>      }
> +    // We need null check for aOldComputedValues since the value might have been
> +    // cleared after we create a SequentialTask for EffectProperties or
> +    // CSSTransitions because the targe element got in display:none subtree.

Nit: target element

But actually I'm having trouble making sense of this comment. What is the sequence of events here?

Is something like the following correct?

    // aOldComputedValues might be nullptr if the target element is now in
    // a display:none subtree. We still call Gecko_UpdateAnimations in this case
    // because we need to stop CSS animations in the display:none subtree.
    // However, we don't need to update transitions since they are stopped by
    // RestyleManager::AnimationsWithDestroyedFrame so we just return early
    // here.

Comment 10

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Comment on attachment 8860854 [details]
> Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for
> null computed values.
> 
> https://reviewboard.mozilla.org/r/132848/#review135658
> 
> ::: commit-message-73752:6
> (Diff revision 1)
> > +Even so Gecko_UpdateAnimations is called because we need to stop CSS
> > +animations in display:none subtree.
> 
> I think we can also stop transitions in display:none subtree here (for most
> cases), but Boris does not think we don't need to do it here (see 1341372
> comment 63).

So Boris is saying that we don't need to do it here because that will be covered by RestyleManager::AnimationsWithDestroyedFrame, right?

(Also, a very minor note, bugzilla will automatically link to comments in other bugs if you use the format "bug XXX comment XXX")

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> ::: layout/style/ServoBindings.cpp:487
> (Diff revision 2)
> >          UpdateAnimations(const_cast<dom::Element*>(aElement), pseudoType,
> >                           servoValues);
> >      }
> > +    // We need null check for aOldComputedValues since the value might have been
> > +    // cleared after we create a SequentialTask for EffectProperties or
> > +    // CSSTransitions because the targe element got in display:none subtree.
> 
> Nit: target element
> 
> But actually I'm having trouble making sense of this comment. What is the
> sequence of events here?
> 
> Is something like the following correct?
> 
>     // aOldComputedValues might be nullptr if the target element is now in
>     // a display:none subtree. We still call Gecko_UpdateAnimations in this
> case
>     // because we need to stop CSS animations in the display:none subtree.
>     // However, we don't need to update transitions since they are stopped by
>     // RestyleManager::AnimationsWithDestroyedFrame so we just return early
>     // here.

That's right.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > Comment on attachment 8860854 [details]
> > Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for
> > null computed values.
> > 
> > https://reviewboard.mozilla.org/r/132848/#review135658
> > 
> > ::: commit-message-73752:6
> > (Diff revision 1)
> > > +Even so Gecko_UpdateAnimations is called because we need to stop CSS
> > > +animations in display:none subtree.
> > 
> > I think we can also stop transitions in display:none subtree here (for most
> > cases), but Boris does not think we don't need to do it here (see 1341372
> > comment 63).
> 
> So Boris is saying that we don't need to do it here because that will be
> covered by RestyleManager::AnimationsWithDestroyedFrame, right?

Right.  Also I am guessing that if servo's machinery about display:none sub tree works as expected we will not need AnimationsWithDestroyedFrame eventually.

Comment 13

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Brian Birtles (:birtles) from comment #9)
> > Is something like the following correct?
> > 
> >     // aOldComputedValues might be nullptr if the target element is now in
> >     // a display:none subtree. We still call Gecko_UpdateAnimations in this
> > case
> >     // because we need to stop CSS animations in the display:none subtree.
> >     // However, we don't need to update transitions since they are stopped by
> >     // RestyleManager::AnimationsWithDestroyedFrame so we just return early
> >     // here.
> 
> That's right.

Ok, r=me with that then.

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8860854 [details]
Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for null computed values.

https://reviewboard.mozilla.org/r/132848/#review135992

Comment 15

[Hiroyuki Ikezoe (:hiro)]

A try run [1] noticed me that I made a big mistake that we have to check aComputedValues instead of aOldComputedValues. And considering the fact that checking aOldComputedValues fixed this crash, we should also check aOldComputedValues only for CSSTransitions case.  I will revise the first patch, and post a new one for the transition.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=723dcf41694ddf1f3512069ce8b8406a4a008979&selectedJob=93901606

Thank to the test case, the only one test case caught the mistake.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> And considering the fact that
> checking aOldComputedValues fixed this crash, we should also check
> aOldComputedValues only for CSSTransitions case.  I will revise the first
> patch, and post a new one for the transition.

This is wrong, we already check null old value in might_need_transitions_update().

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860854 [details]
Bug 1358965 - Don't call UpdateEffectProperties and UpdateTransitions() for null computed values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132848/diff/2-3/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8860857 [details]
Bug 1358965 - Drop unnecessary static_cast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132862/diff/1-2/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e652e7d35e7
Don't call UpdateEffectProperties and UpdateTransitions() for null computed values. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f4353d26ccd1
Drop unnecessary static_cast. r=birtles

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e652e7d35e7
https://hg.mozilla.org/mozilla-central/rev/f4353d26ccd1