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

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: Tomcat, Assigned: hiro)

Tracking

(Blocks: 2 bugs, {crash})

unspecified
mozilla55
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

6 months ago
Created attachment 8860829 [details]
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
I see functions like Servo_GetComputedKeyframeValues in the stack. Guessing it's something related to animation. ni? hiro.
Flags: needinfo?(hikezoe)
(Assignee)

Comment 2

6 months ago
This is a case that we call UpdateEffectProperties() for element which got in display:none subtree.  We need null check for this case.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

6 months ago
mozreview-review
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).
(Assignee)

Comment 5

6 months ago
This is a good opportunity to remove silly my mistake (redundant static_cast there).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

6 months ago
mozreview-review
Comment on attachment 8860857 [details]
Bug 1358965 - Drop unnecessary static_cast.

https://reviewboard.mozilla.org/r/132862/#review135986
Attachment #8860857 - Flags: review?(bbirtles) → review+

Comment 9

6 months ago
mozreview-review
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.
(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")
(Assignee)

Comment 11

6 months ago
(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.
(Assignee)

Comment 12

6 months ago
(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.
(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

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

https://reviewboard.mozilla.org/r/132848/#review135992
Attachment #8860854 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 15

6 months ago
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.
(Assignee)

Comment 16

6 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

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

Comment 20

6 months ago
Pushed by hikezoe@mozilla.com:
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
(Reporter)

Comment 21

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e652e7d35e7
https://hg.mozilla.org/mozilla-central/rev/f4353d26ccd1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.