stylo: panicked at '<html> (0x7fffc7acb380) has still dirty bit true or animation-only dirty bit true on animation for mask

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments)

Posted file anim-for-mask.html
This is another variant of bug 1399349.  Bug 1397057 actually fixed the case on weebly.com, but it does not fix a real problem. Attaching file causes the same panic.

Also this is somewhat related to bug 1400022.  Some SVG elements call PostRestyleEvent for propagating nsChangeHint_InvalidateRenderingObservers without restyle hint during post traversal.  The call stack is something like this;


#2  0x00007fffe838e140 in mozilla::ServoRestyleManager::PostRestyleEvent (this=0x7fffccd50d40, aElement=0x7fffcef8eeb0, aRestyleHint=<optimized out>,-
    aMinChangeHint=nsChangeHint_InvalidateRenderingObservers) at /home/ikezoe/autoland/layout/base/ServoRestyleManager.cpp:335
#3  0x00007fffe83c80df in nsLayoutUtils::PostRestyleEvent (aElement=0x7fffcef8eeb0, aRestyleHint=aRestyleHint@entry=0,-
    aMinChangeHint=aMinChangeHint@entry=nsChangeHint_InvalidateRenderingObservers) at /home/ikezoe/autoland/layout/base/nsLayoutUtils.cpp:8008
#4  0x00007fffe853f525 in nsSVGRenderingObserverProperty::DoUpdate (this=this@entry=0x7fffcc6bfb00) at /home/ikezoe/autoland/layout/svg/nsSVGEffects.cpp:237
#5  0x00007fffe853f5f0 in nsSVGPaintingProperty::DoUpdate (this=0x7fffcc6bfb00) at /home/ikezoe/autoland/layout/svg/nsSVGEffects.cpp:497
#6  0x00007fffe854d30e in nsSVGRenderingObserverList::InvalidateAll (this=0x7fffd8b9d4f0) at /home/ikezoe/autoland/layout/svg/nsSVGEffects.cpp:808
#7  0x00007fffe8448c8c in InvalidateFrameInternal (aFrame=0x7fffdafbecd8, aHasDisplayItem=true) at /home/ikezoe/autoland/layout/generic/nsFrame.cpp:6458
#8  0x00007fffe8545016 in mozilla::SVGGeometryFrame::DidSetStyleContext (this=0x7fffdafbecd8, aOldStyleContext=0x7fffcc696aa8)
    at /home/ikezoe/autoland/layout/svg/SVGGeometryFrame.cpp:200
#9  0x00007fffe83ae4fc in nsIFrame::SetStyleContext (this=this@entry=0x7fffdafbecd8, aContext=0x7fffcc6959c8) at /home/ikezoe/autoland/layout/generic/nsIFrame.h:769
#10 0x00007fffe83930d4 in mozilla::ServoRestyleManager::ProcessPostTraversal (this=this@entry=0x7fffccd50d40, aElement=aElement@entry=0x7fffccf78da0,-
    aParentContext=aParentContext@entry=0x0, aRestyleState=..., aFlags=aFlags@entry=mozilla::ServoPostTraversalFlags::Empty)
    at /home/ikezoe/autoland/layout/base/ServoRestyleManager.cpp:866
#11 0x00007fffe839feb3 in mozilla::ServoRestyleManager::DoProcessPendingRestyles (this=0x7fffccd50d40, aFlags=aFlags@entry=mozilla::ServoTraversalFlags::Empty)
    at /home/ikezoe/autoland/layout/base/ServoRestyleManager.cpp:1121

To me (at least for stylo), it seems that the InvalidateRenderingObservers change hint is just needed to be propagated to PrecessRestyledFrames() (I mean we don't call PostRestyleEvent at all).

What I don't quite understand is that we have test cases that require the InvalidateRenderingObservers change hint or not.  I did push a try [1] that dropped two PostRestyleEvent calls, it seems there is no test case, or we don't need to call them.  If the PostRestyleEvent calls is really unnecessary, the panic is not a big problem, but if it's necessary we have to solve it somehow. 

CCing people who are super familiar with SVG stuff.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=46ccc713950271a34fda5400a261a017f2954d54
:jwatt, longsonr, CJ?
Flags: needinfo?(longsonr)
Flags: needinfo?(jwatt)
Flags: needinfo?(cku)
See Also: → 1400022
Oh, what? They post hints from DidSetStyleContext?

That's definitely no good. We support posting hints from change hint processing, which go through here, but posting them from the post-traversal breaks all sorts of invariants.

If we can remove those, that'd be great. If we cannot, we can figure out something, but it's still pretty annoying.
Those hints are how we cause changes to filter/clipPath/mask/symbol etc elements children to cause redraws of the filtered/clipped/masked/used elements. Without them animation of filters/clipPaths/masks/symbols via SMIL or javascript won't refresh properly during the animation.
Flags: needinfo?(longsonr)
Yeah, I understand, but it should not be called from DidSetStyleContext, right?
Now I think the culprit is InvalidateFrame() call in SVGGeometryFrame::DidSetStyleContext(), which was introduced in bug 901955. I guess we should fix nsSVGUtils::CanOptimizeOpacity() instead of calling InvalidateFrame() from DidSetStyleContext().
Clearing ni?s. Now I am confident that checking whether a given frame has opacity animations or not in CanOptimizeOpacity() is the right thing to do.
Flags: needinfo?(jwatt)
Flags: needinfo?(cku)
Bonus point! The opacity animation in the test case in comment 0 is pretty awkward on gecko, but with the patch it's pretty smooth on stylo. Sadly I can't write reftest for it though. It's pretty difficult since the animation does not work smoothly on gecko, but even so it works awkward anyway.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Gah, dropping InvalidateFrame() call caused 1 color component difference all
> over the place. :jwatt is that OK with you?
> 
> https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/
> reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/
> HCspiTUoRaWAyBSgOv4Ahw/runs/0/artifacts/public/logs/live_backing.
> log&only_show_unexpected=1

Never mind,  I did use a wrong function to check a given frame has opacity animations.
This try should work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6659d0c292fa32087fe4d12efecda833c77afcfb
Flags: needinfo?(jwatt)
There are still two reftests that rely on the InvalidateFrame in DidStyleContext(), outer-svg-border-and-padding-01.svg and dynamic-opacity-property-01.svg. The former changes opacity attribute directly, it can be fixed by posting nsChangeHint_InvalidateRenderingObservers in SVGGeometryFrame::AttributeChanged for the case (honestely I think we should do such change hint handling in GetAttributeChangeHint). But the latter changes style.opacity, it's annoying.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Priority: -- → P2
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> There are still two reftests that rely on the InvalidateFrame in
> DidStyleContext(), outer-svg-border-and-padding-01.svg and
> dynamic-opacity-property-01.svg. The former changes opacity attribute
> directly, it can be fixed by posting
> nsChangeHint_InvalidateRenderingObservers in
> SVGGeometryFrame::AttributeChanged for the case (honestely I think we should
> do such change hint handling in GetAttributeChangeHint). But the latter
> changes style.opacity, it's annoying.

I can't come up with any ideas to tell opacity style changes, so I decided to post InvalidateRenderingObservers change hint any style is changed for geometry frame in SVGGeometryFrame::AttributeChanged.  (Actually I think we should check these changes in nsSVGUtils::CanOptimizeOpacity() though).
:jwatt, are you OK with this way?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd3380ecc39d87ba761d0d9630fd31645df69ec6
Flags: needinfo?(jwatt)
To be clear, I will post patches and request review for now.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Created attachment 8909670 [details]
> Bug 1400035 - Check the frame has opacity animations in
> nsSVGUtils::CanOptimizeOpacity().
> 
> If the frame has opacity animation, we can't optimize it at all.

This is basically what bug 1278825 is about but - at least back whet that was filed - the fix was not so simple. I need to do a bit of digging.
See Also: → 1278825
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > Created attachment 8909670 [details]
> > Bug 1400035 - Check the frame has opacity animations in
> > nsSVGUtils::CanOptimizeOpacity().
> > 
> > If the frame has opacity animation, we can't optimize it at all.
> 
> This is basically what bug 1278825 is about but - at least back whet that
> was filed - the fix was not so simple. I need to do a bit of digging.

Bug 1278825 might be related. But the two test case that fail without InvalidateFrame call in DidSetStyleContext() have no animations. Though we might have no tests for such cases, but even so, on current Gecko the animation in comment 0 does not work, on Stylo the animation works smoothly with attachment 8909670 [details] (and attachment 8909672 [details] to avoid the panic).
I think I am getting closer to the failure reason of the two test cases.
From nsIFrame::BuildDisplayListForStackingContext [1]

  bool useOpacity = HasVisualOpacity(effectSet) &&
                    !nsSVGUtils::CanOptimizeOpacity(this) &&
                    (!usingSVGEffects || nsDisplayOpacity::NeedsActiveLayer(aBuilder, this));

HasVisualOpacity() returns false if a value which is greater than 0.99 is specified for opacity.  We have a special handling [2] for such values in nsStyleEffects::CalcDifference().

    // If we're going from the optimized >=0.99 opacity value to 1.0 or back, then
    // repaint the frame because DLBI will not catch the invalidation.  Otherwise,
    // just update the opacity layer.
    if ((mOpacity >= 0.99f && mOpacity < 1.0f && aNewData.mOpacity == 1.0f) ||
        (aNewData.mOpacity >= 0.99f && aNewData.mOpacity < 1.0f && mOpacity == 1.0f)) {
      hint |= nsChangeHint_RepaintFrame;

I think we should do the same thing (setting repaint frame change hint) for CanOptimizeOpacity() case. I think it's a proper solution, but I have no idea how to check CanOptimizeOpacity there.

[1] https://hg.mozilla.org/mozilla-central/file/a0eb21bf55e1/layout/generic/nsFrame.cpp#l2488
[2] https://hg.mozilla.org/mozilla-central/file/a0eb21bf55e1/layout/style/nsStyleStruct.cpp#l4595
Comment on attachment 8909671 [details]
Bug 1400035 - Propagate RepaintFrame change hint instead of UpdateOpacityLayer for SVG geometry frames.

https://reviewboard.mozilla.org/r/181194/#review186812

This only handles opacity property changes that originate in an attribute change. That doesn't seem to handle opacity property changes that occurred in style sheets. Either that means style sheet changes won't work (this patch is incomplete) or else it seems like this patch is not needed (all opacity changes are handled elsewhere).

Feel free to re-request review if you can clear up these questions, but is looks like the patch is wrong.
Attachment #8909671 - Flags: review?(jwatt) → review-
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #19)
> Comment on attachment 8909671 [details]
> Bug 1400035 - Post InvalidateRenderingObservers change hint for
> SVGGeometryFrame when opacity attribute/style is changed.
> 
> https://reviewboard.mozilla.org/r/181194/#review186812
> 
> This only handles opacity property changes that originate in an attribute
> change. That doesn't seem to handle opacity property changes that occurred
> in style sheets. Either that means style sheet changes won't work (this
> patch is incomplete) or else it seems like this patch is not needed (all
> opacity changes are handled elsewhere).
> 
> Feel free to re-request review if you can clear up these questions, but is
> looks like the patch is wrong.

Yeah, fair enough. But posting restyle events from DidSetStyleContext() looks also wrong to me. Anyway we have to figure out a way to make the SVG opacity optimization proper.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)
> Yeah, fair enough. But posting restyle events from DidSetStyleContext()
> looks also wrong to me. Anyway we have to figure out a way to make the SVG
> opacity optimization proper.

Absolutely, I agree. It's late and I need sleep, but I'll continue working on an alternative solution in the morning (unless you come up with another one before then).
Thank you :jwatt for the cooperation!  I noticed that we can fix up the change hint in RestyleManager::ProcessRestyledFrames(), this should work, at least I can confirm locally, the two test cases pass.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=101bbb21730dacc1924bb5f8e37af912bfb3faef
No suspicious failures appeared on the try. I hope I am on the right track.
Comment on attachment 8909670 [details]
Bug 1400035 - Check the frame has opacity animations in nsSVGUtils::CanOptimizeOpacity().

https://reviewboard.mozilla.org/r/181192/#review187204
Attachment #8909670 - Flags: review?(jwatt) → review+
Comment on attachment 8909672 [details]
Bug 1400035 - Drop InvalidateFrame() call in SVGGeometryFrame::DidSetStyleContext().

https://reviewboard.mozilla.org/r/181196/#review187206
Attachment #8909672 - Flags: review?(jwatt) → review+
Comment on attachment 8909671 [details]
Bug 1400035 - Propagate RepaintFrame change hint instead of UpdateOpacityLayer for SVG geometry frames.

https://reviewboard.mozilla.org/r/181194/#review187304

This looks better, although I somewhat feel like we're making difficult to reason about code even harder. I don't currently have a better suggestion though.

::: commit-message-726d4:3
(Diff revision 2)
> +Bug 1400035 - Propagate RepaintFrame change hint instead of UpdateOpacityLayer for SVG geometry frames. r?jwatt
> +
> +We do return RepaintFrame change hint from nsStyleEffects::CalcDifference for

Please move the bulk of this comment inline into the code.
Attachment #8909671 - Flags: review?(jwatt) → review+
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #29)
> Comment on attachment 8909671 [details]
> Bug 1400035 - Propagate RepaintFrame change hint instead of
> UpdateOpacityLayer for SVG geometry frames.
> 
> https://reviewboard.mozilla.org/r/181194/#review187304
> 
> This looks better, although I somewhat feel like we're making difficult to
> reason about code even harder. I don't currently have a better suggestion
> though.
> 
> ::: commit-message-726d4:3
> (Diff revision 2)
> > +Bug 1400035 - Propagate RepaintFrame change hint instead of UpdateOpacityLayer for SVG geometry frames. r?jwatt
> > +
> > +We do return RepaintFrame change hint from nsStyleEffects::CalcDifference for
> 
> Please move the bulk of this comment inline into the code.

Oh right, indeed. Thank for the review!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4ae3d8fd4a4
Check the frame has opacity animations in nsSVGUtils::CanOptimizeOpacity().  r=jwatt
https://hg.mozilla.org/integration/autoland/rev/2db87d9e38c1
Propagate RepaintFrame change hint instead of UpdateOpacityLayer for SVG geometry frames. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/26228016eeae
Drop InvalidateFrame() call in SVGGeometryFrame::DidSetStyleContext(). r=jwatt
Flags: needinfo?(jwatt)
You need to log in before you can comment on or make changes to this bug.