Closed Bug 1396041 Opened 3 years ago Closed 3 years ago

stylo: Assertion failure: !mPresContext->RestyleManager()->IsInStyleRefresh()

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

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

People

(Reporter: truber, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

The attached testcases causes an assertion in m-c rev 20170901-a3585c77e2b1 with stylo enabled by pref.

Assertion failure: !mPresContext->RestyleManager()->IsInStyleRefresh(), at /builds/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:337
#0: mozilla::EffectCompositor::PostRestyleForAnimation, at dom/animation/EffectCompositor.cpp:337
#1: mozilla::EffectCompositor::RequestRestyle, at dom/animation/EffectCompositor.cpp:287
#2: mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated, at dom/animation/KeyframeEffectReadOnly.cpp:142
#3: mozilla::dom::Animation::UpdateTiming, at dom/animation/Animation.cpp:1238
#4: mozilla::dom::Animation::CancelNoUpdate, at dom/animation/Animation.cpp:817
#5: mozilla::AnimationCollection<mozilla::dom::CSSTransition>::PropertyDtor, at dom/animation/Animation.h:149
#6: nsPropertyTable::PropertyList::DeletePropertyFor, at dom/base/nsPropertyTable.cpp:296
#7: nsPropertyTable::DeleteProperty, at dom/base/nsPropertyTable.cpp:232
#8: nsINode::DeleteProperty, at dom/base/nsINode.cpp:195
#9: mozilla::dom::Element::UnbindFromTree, at dom/base/Element.cpp:1898
#10: nsGenericHTMLElement::UnbindFromTree, at dom/html/nsGenericHTMLElement.cpp:537
#11: nsFrameManager::DestroyAnonymousContent, at layout/base/nsFrameManager.cpp:695
#12: nsIFrame::DestroyAnonymousContent, at layout/generic/nsFrame.cpp:247
#13: nsMeterFrame::DestroyFrom, at layout/forms/nsMeterFrame.cpp:57
#14: nsBlockFrame::DoRemoveFrame, at layout/generic/nsBlockFrame.cpp:6014
#15: nsBlockFrame::RemoveFrame, at layout/generic/nsBlockFrame.cpp:5327
#16: nsFrameManager::RemoveFrame, at layout/base/nsFrameManager.cpp:536
#17: nsCSSFrameConstructor::ContentRemoved, at layout/base/nsCSSFrameConstructor.cpp:8908
#18: nsCSSFrameConstructor::RecreateFramesForContent, at layout/base/nsCSSFrameConstructor.cpp:10098
#19: nsCSSFrameConstructor::RecreateFramesForContent, at layout/base/nsCSSFrameConstructor.cpp:10072
#20: mozilla::RestyleManager::ProcessRestyledFrames, at layout/base/RestyleManager.cpp:1509
#21: mozilla::ServoRestyleManager::DoProcessPendingRestyles, at layout/base/ServoRestyleManager.cpp:1136
Flags: in-testsuite?
Attached file testcase.html
So this is a regression in bug 1393791, which causes us to synchronously invoke UnbindFromTree on native-anonymous content during frame teardown.

In general, there's no reason we should be posting restyles on this stuff, since it's being unbound from the tree. I can see various ways to fix this, but I'll defer to the animation folks on what makes the most sense.
Blocks: 1393791
Flags: needinfo?(hikezoe)
Flags: needinfo?(bbirtles)
Priority: -- → P2
(Just curious, does this also repro without stylo?)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> (Just curious, does this also repro without stylo?)

It does not appear to repro with Stylo disabled, in my macOS Stylo build. I confirmed it DID repro with Stylo enabled.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> So this is a regression in bug 1393791, which causes us to synchronously
> invoke UnbindFromTree on native-anonymous content during frame teardown.
> 
> In general, there's no reason we should be posting restyles on this stuff,
> since it's being unbound from the tree.

That's right. I think we should check that the element is associated with a document in EffectCompositor::RequestRestyle(). To do this, ClearInDocument() has to be moved right before DeleteProperty() sets in UnbindFromTree().  I think it should work, let's see what happens. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=673444fe3a48113ed71b62761973e7371e42263f

Brian might have different ideas, so I'd like to defer him how to solve this eventually.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> > So this is a regression in bug 1393791, which causes us to synchronously
> > invoke UnbindFromTree on native-anonymous content during frame teardown.
> > 
> > In general, there's no reason we should be posting restyles on this stuff,
> > since it's being unbound from the tree.
> 
> That's right. I think we should check that the element is associated with a
> document in EffectCompositor::RequestRestyle().

EffectCompositor::RequestRestyle already checks that (in the call to nsComputedDOMStyle::GetPresShellForContent).

> To do this,
> ClearInDocument() has to be moved right before DeleteProperty() sets in
> UnbindFromTree().

Yes, that part will need to change.

> I think it should work, let's see what happens. 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=673444fe3a48113ed71b62761973e7371e42263f
> 
> Brian might have different ideas, so I'd like to defer him how to solve this
> eventually.

I think the ClearInDocument changes are good. I'd prefer to drop the changes to EffectCompositor::PostRestyleForAnimation unless we have a test case that needs it.
Flags: needinfo?(bbirtles)
Assignee: nobody → hikezoe
(In reply to Brian Birtles (:birtles) from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> > > So this is a regression in bug 1393791, which causes us to synchronously
> > > invoke UnbindFromTree on native-anonymous content during frame teardown.
> > > 
> > > In general, there's no reason we should be posting restyles on this stuff,
> > > since it's being unbound from the tree.
> > 
> > That's right. I think we should check that the element is associated with a
> > document in EffectCompositor::RequestRestyle().
> 
> EffectCompositor::RequestRestyle already checks that (in the call to
> nsComputedDOMStyle::GetPresShellForContent).

Oh right. I missed the part. Thanks!
The try looks good (all known oranges).
Comment on attachment 8904102 [details]
Bug 1396041 - Disassociate element from document before destroying animations in UnbindFromTree().

https://reviewboard.mozilla.org/r/175872/#review180846

::: commit-message-8e052:1
(Diff revision 1)
> +Bug 1396041 - Disaccosiate element from document before destroying animations in UnbindFromTree(). r?birtles

Disassociate

::: commit-message-8e052:3
(Diff revision 1)
> +So that we can avoid unnecessary restyle request during destroying animations
> +for such orphaned elements.

This allows us to avoid posting animation-related restyles when removing elements from the document tree.

::: layout/style/crashtests/crashtests.list:213
(Diff revision 1)
> +load 1396041.html
>  load 1395725.html

Let's put it at the end so at least the order is locally sorted.
Attachment #8904102 - Flags: review?(bbirtles) → review+
Thanks for the review!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e4c2725acf2
Disassociate element from document before destroying animations in UnbindFromTree(). r=birtles
https://hg.mozilla.org/mozilla-central/rev/5e4c2725acf2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Flags: in-testsuite? → in-testsuite+
Regressions: 1541767
You need to log in before you can comment on or make changes to this bug.