Closed Bug 1374175 Opened 7 years ago Closed 7 years ago

stylo: panic on element.borrow_data().unwrap() at the end of Servo_TraverseSubtree()

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files)

Attached file A sample test case
Attaching file is an example to cause the panic.
There is an input element which has 'animation' style.

A stack trace:
#11 0x00007fffe7b4f286 in core::option::{{impl}}::unwrap<atomic_refcell::AtomicRef<style::data::ElementData>> (self=...) at /checkout/src/libcore/macros.rs:21
#12 0x00007fffe7be5565 in geckoservo::glue::Servo_TraverseSubtree (root=0x7fffc370eaa0, raw_data=0x7fffb914d000, snapshots=0x7fffdd17c5c8, root_behavior=Normal, 
    restyle_behavior=Normal) at /home/ikezoe/central/servo/ports/geckolib/glue.rs:305
#13 0x00007fffe4168c11 in mozilla::ServoStyleSet::PrepareAndTraverseSubtree (this=0x7fffdd17c380, aRoot=0x7fffc370eaa0, aRootBehavior=mozilla::TraversalRootBehavior::Normal, 
    aRestyleBehavior=mozilla::TraversalRestyleBehavior::Normal) at /home/ikezoe/central/layout/style/ServoStyleSet.cpp:383
#14 0x00007fffe416b0c2 in mozilla::ServoStyleSet::StyleNewSubtree (this=0x7fffdd17c380, aRoot=0x7fffc370eaa0) at /home/ikezoe/central/layout/style/ServoStyleSet.cpp:946
#15 0x00007fffe4382a6e in nsCSSFrameConstructor::GetAnonymousContent (this=0x7fffba7e7210, aParent=0x7fffc58a09c0, aParentFrame=0x7fffbbbf69a8, aContent=...)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:4324
#16 0x00007fffe4393c46 in nsCSSFrameConstructor::ProcessChildren (this=0x7fffba7e7210, aState=..., aContent=0x7fffc58a09c0, aStyleContext=0x7fffb9de9040, aFrame=0x7fffbbbf69a8, 
    aCanHaveGeneratedContent=true, aFrameItems=..., aAllowBlockStyles=false, aPendingBinding=0x7fffb8edae20, aPossiblyLeafFrame=0x7fffbbbf69a8)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:11154
#17 0x00007fffe4382069 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x7fffba7e7210, aItem=..., aState=..., aParentFrame=0x7fffbbbf68f8, aFrameItems=...)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:4118
#18 0x00007fffe43876db in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x7fffba7e7210, aState=..., aIter=..., aParentFrame=0x7fffbbbf68f8, aFrameItems=...)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:6303
#19 0x00007fffe43df256 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x7fffba7e7210, aState=..., aItems=..., aParentFrame=0x7fffbbbf68f8, aFrameItems=...)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:10932
#20 0x00007fffe439425f in nsCSSFrameConstructor::ProcessChildren (this=0x7fffba7e7210, aState=..., aContent=0x7fffbfeceb60, aStyleContext=0x7fffb9ddff00, aFrame=0x7fffbbbf68f8, 
    aCanHaveGeneratedContent=true, aFrameItems=..., aAllowBlockStyles=true, aPendingBinding=0x0, aPossiblyLeafFrame=0x7fffbbbf68f8)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:11240
#21 0x00007fffe4396b6c in nsCSSFrameConstructor::ConstructBlock (this=0x7fffba7e7210, aState=..., aContent=0x7fffbfeceb60, aParentFrame=0x7fffbbbf60a8, 
    aContentParentFrame=0x7fffbbbf60a8, aStyleContext=0x7fffb9ddff00, aNewFrame=0x7fffffffb970, aFrameItems=..., aPositionedFrameForAbsPosContainer=0x0, aPendingBinding=0x0)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:12239
#22 0x00007fffe437da06 in nsCSSFrameConstructor::ConstructDocElementFrame (this=0x7fffba7e7210, aDocElement=0x7fffbfeceb60, aFrameState=0x0)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:2690
#23 0x00007fffe438c253 in nsCSSFrameConstructor::ContentRangeInserted (this=0x7fffba7e7210, aContainer=0x0, aStartChild=0x7fffbfeceb60, aEndChild=0x0, aFrameState=0x0, 
    aAllowLazyConstruction=false, aForReconstruction=false, aProvidedTreeMatchContext=0x0) at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:7989
#24 0x00007fffe435a7fc in nsCSSFrameConstructor::ContentRangeInserted (this=0x7fffba7e7210, aContainer=0x0, aStartChild=0x7fffbfeceb60, aEndChild=0x0, aFrameState=0x0, 
    aAllowLazyConstruction=false, aProvidedTreeMatchContext=0x0) at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.h:276
#25 0x00007fffe438bce7 in nsCSSFrameConstructor::ContentInserted (this=0x7fffba7e7210, aContainer=0x0, aChild=0x7fffbfeceb60, aFrameState=0x0, aAllowLazyConstruction=false)
    at /home/ikezoe/central/layout/base/nsCSSFrameConstructor.cpp:7874

What's going on there is:

1) Process animation style in the initial traversal on the input element
2) Create a SequentialTask to create a CSS animation object for the input element
3) Create the CSS animation
4) Process animation-only restyle for the CSS animation (Note that this is still processed during the *initial* traversal)
5) During the animation-only restyle in compute_style_difference(),  RestyleDamage::reconstruct() is added because the input element does not have corresponding nsStyleContext yet at this moment
6) A NAC element (div class="anonymous-div") which is a child of the input element is processed in the animation-only restyle
7) In node_needs_traversal() the NAC element is considered as a child of going-to-reframe element [1] and is skipped to traversed

I don't still understand what's going there after 7), but the stack tell me that ServoStyleSet::StyleNewSubtree is called against the NAC and the NAC element has no ElementData.

[1] https://hg.mozilla.org/mozilla-central/file/95543bdc59bd/servo/components/style/traversal.rs#l298
So this seems to be because we set the restyle damage when processing the animation-only restyle, is that right?

Why are we even doing all that for animation-only restyle? seems like wasted work if we always have a proper restyle afterwards (do we?).
As far as I can tell, we can't skip setting restyle damage in animation-only restyle. For example, width animation produces such change hints. I actually don't know what the change hint is produced exactly.

I did a try with the skipping restyle damage. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd2769c4a66baedc6d921217fc3d0ff3f4f5f136
It turns out the approach which introduces HAS_GOT_INITIAL_STYLE flag is not a good idea since there are lots of cases that we can't distinguish we haven't got the initial nsStyleContext but need to accumulate restyle damage.  The failure tests in the try in comment 1 are such cases.

Instead of it, we can return no damage if there is no old nsStyleContext and the traversal is animation-only from compute_style_difference.  I think what Emilio suggested in comment 5 is this way.  I think I was misunderstanding the comment. 

Here is a try with the new approach:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b1d57662b3c479e7fb5b13fa8284b6405bee01f

The result looks much better. But still a couple of reftests for SMIL failed. I will investigate the failure reason today, but it would be appreciated any insight about the failure. thanks!
All the failure reftests are 'display' property animation.  It should not be relied on RestyleDamage::reconstruct() in initial restyle [1].
Brian, any suggestion?

[1] https://hg.mozilla.org/mozilla-central/file/f3483af8ecf9/servo/components/style/matching.rs#l1542
Flags: needinfo?(bbirtles)
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> So this seems to be because we set the restyle damage when processing the
> animation-only restyle, is that right?
> 
> Why are we even doing all that for animation-only restyle? seems like wasted
> work if we always have a proper restyle afterwards (do we?).

We don't always have a proper restyle after the animation-only restyle. Sometimes we only have the animation-only restyle.

I spoke to Hiro about the failing test cases and it looks like all the failures correspond to when we animate from 'none' to block/inline. It seems like we need to handle the case when we don't have a style context due to being display:none.
Flags: needinfo?(bbirtles)
OK, I found the reason why the test cases failed. That's because existing_style_for_restyle_damage fail to get old nsStyleContext in case where the element is display:none subtree.  Because Gecko_GetStyleContext does not care about the display:none case. Should we also use GetUndisplayedContent as well for display:none cases?
Cameron told me that we can return damages from compute_style_difference() depends on the display style changes.
We should return RestyleDamage::reconstruct() if display property changed from none to other and vise versa. In other cases we should return RestyleDamage::empty().  It fixed the test case in comment 0, and also does not cause any problems for the cases of SMIL display animations.

Here is a try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8413431c4fe209f51322129e7cf15ff49bce2f13
A new try with checking display property changes, not only from none to others.

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

Unfortunately details element case still failed.
The way Emilio told me did not fix the failure of the reftest for details element either.
The way is to get style differences with *servo* computed values using the patch for in 1372318.

FWIW, here is a try with patches for the way (sorry for ugaly patches)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=947c8fa98a78d8323dbf930fed81768f2984c271

An interesting point is that the details element reftest does not fail with STYLO_THREADS=1. Actually I did use the value locally, then I thought it works well...

Also, unfortunately another way Emilio suggested me does not fix the original panic problem. That's because the original case is not  UnstyledChildrenOnly traversal (my memory was wrong).  The NAC element is traversed  StyleNewSubtree.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Also, unfortunately another way Emilio suggested me does not fix the
> original panic problem. That's because the original case is not 
> UnstyledChildrenOnly traversal (my memory was wrong).  The NAC element is
> traversed from StyleNewSubtree.

from StyleNewSubtree for the NAC element from nsCSSFrameConstructor::GetAnonymousContent.
After the try, I noticed that we can check if the traversal is animation-only.  I mean we keep being conservative (i.e returning reconstruction damage) if the traversal is not animation-only.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71635f52c60f3681de5864a86af74b948b8ca606
Finally it's green!

I think the check should work with either ways (calc style difference with servo's computed values or checking display property changes).
Now I pushed another try with the display property changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49b8272a67fd372633b7184f4cac4a6522aa1a8f
Or, adding a new traversal flag for such NAC element and using the flag if we do traversal for NAC elements from nsCSSFrameConstructor::GetAnonymousContent(), and if the traversal has the flag, we never skip the NAC elements for traversal.  I think this is the right thing to do, but also think setting reconstruction damage should be more accurate (as far as possible).
Discussed with Cameron this morning, I decided to try the new traversal flag and see if what happens on try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6fab1a2ebeed0904f2650fbb1577acb065a3874
Dropping StyleNewChildren from AddFrameConstructionItemsInternal [1] might be an overkill though.

[1] https://hg.mozilla.org/try/rev/8cf8c14f1bfb6336cf5f1aff1c61d4e813e123bd#l1.12

Another try without the drop;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84e4c787f49cb49edcc728d91efa622f6b483500
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> Dropping StyleNewChildren from AddFrameConstructionItemsInternal [1] might
> be an overkill though.
> 
> [1]
> https://hg.mozilla.org/try/rev/8cf8c14f1bfb6336cf5f1aff1c61d4e813e123bd#l1.12
> 
> Another try without the drop;
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=84e4c787f49cb49edcc728d91efa622f6b483500

That dropping part was actually overkill, but the second try result looks really good.  Also I did confirm that the second one fixes bug 1377197 too. Yay!
Moved checking the flag point into the if block for NAC elements.

Try looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d7941085a19d3eddcb2fcb40698c3b76340ced7
Blocks: 1377197
Blocks: 1375902
Comment on attachment 8882602 [details]
Bug 1374175 - Return no damage if there is no display property changes.

Clearing review request since I found a solution to fix the detail elements failure.
Attachment #8882602 - Flags: review?(cam)
Depends on: 1377648
No longer blocks: 1377197
Comment on attachment 8882602 [details]
Bug 1374175 - Return no damage if there is no display property changes.

https://reviewboard.mozilla.org/r/153690/#review159072

::: servo/components/style/matching.rs:1615
(Diff revision 4)
> -        // Something else. Be conservative for now.
> -        StyleDifference::new(RestyleDamage::reconstruct(), StyleChange::Changed)
> +        let damage = if needs_reconstruction {
> +            RestyleDamage::reconstruct()
> +        } else {
> +            RestyleDamage::empty()
> +        };
> +        StyleDifference::new(damage, StyleChange::Changed)

Can you add a comment in here saying that we don't really know if there was a change in any style (since we didn't actually call compute_style_difference), but we return StyleChange::Changed conversatively.  (But that it shouldn't matter, since we only use this to determine whether we should continue recascading children, and we won't decide to traverse into the display:none subtree anyway.)
Attachment #8882602 - Flags: review?(cam) → review+
See Also: → 1377975
For the record, I talked about this with hiro a while ago, and I don't see why this is correct, really... If someone could prove the correctness of this change to me it'd be nice. Why don't we need to return reconstruct on position changes, for example?
I understand it's not perfect, but i haven't figure out any better way here.  To reduce the risk of regressions, I am going to add two additional test cases.

1) A reftest that has an animation to produce nsChangeHint_ReconstructFrame in initial restyling.
2) A crash test that has a SMIL animation of display property and has an element which creates NAC element.

If attachment 8882602 [details] did overkill, 1) case will fail.  As for 2), SMIL does not involve SequentialTask, so it's not affected in animation-only restyle after SequentialTasks.

Also, I am going to modify attachment 8882602 [details] to make compute_style_difference return empty damage only if it's only animation-only restyle.   This modification reduces the regression risk, I believe.

FWIW, here is a try result with these changes;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf8e96bfd59a5b5cede81179ee7f6a8188a35ad

Emilio, what do you think?
Flags: needinfo?(emilio+bugs)
I don't know, that still feels like a hack, honestly. The bug seems to me that is the fact that we generate a change hint for the animation-only restyle even on initial styling, and all this is only working around the problem. Presumably we shouldn't set the is_restyle() flag for an animation-only traversal.
Flags: needinfo?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> I don't know, that still feels like a hack, honestly. The bug seems to me
> that is the fact that we generate a change hint for the animation-only
> restyle even on initial styling,

Yes, right. A cumbersome point is some animations (e.g. width) need change hint even on the initial styling.

> and all this is only working around the problem.

Yes, right.  I have only work around ideas.

> Presumably we shouldn't set the is_restyle() flag for an
> animation-only traversal.

I haven't succeeded to manipulate with WAS_RESTYLED flag.
OK. I will land this patch as it is tommorow, and keep this issue in mind I can come up with a good idea to fix this issue.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > I don't know, that still feels like a hack, honestly. The bug seems to me
> > that is the fact that we generate a change hint for the animation-only
> > restyle even on initial styling,
> 
> Yes, right. A cumbersome point is some animations (e.g. width) need change
> hint even on the initial styling.

Why is that? We haven't even laid out, why would we ever need a change hint?

> > and all this is only working around the problem.
> 
> Yes, right.  I have only work around ideas.
> 
> > Presumably we shouldn't set the is_restyle() flag for an
> > animation-only traversal.
> 
> I haven't succeeded to manipulate with WAS_RESTYLED flag.
> OK. I will land this patch as it is tommorow, and keep this issue in mind I
> can come up with a good idea to fix this issue.

I'm really opposed to this... But I guess it has already been r+'d so... Can you at least add a comment in the relevant place saying something like "FIXME: This is a workaround for bug 1374175, we do need a proper solution for this"?.
See Also: → 1378972
Blocks: 1378871
(In reply to Emilio Cobos Álvarez [:emilio] from comment #28)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > > I don't know, that still feels like a hack, honestly. The bug seems to me
> > > that is the fact that we generate a change hint for the animation-only
> > > restyle even on initial styling,
> > 
> > Yes, right. A cumbersome point is some animations (e.g. width) need change
> > hint even on the initial styling.
> 
> Why is that? We haven't even laid out, why would we ever need a change hint?
> 
> > > and all this is only working around the problem.
> > 
> > Yes, right.  I have only work around ideas.
> > 
> > > Presumably we shouldn't set the is_restyle() flag for an
> > > animation-only traversal.
> > 
> > I haven't succeeded to manipulate with WAS_RESTYLED flag.
> > OK. I will land this patch as it is tommorow, and keep this issue in mind I
> > can come up with a good idea to fix this issue.
> 
> I'm really opposed to this... But I guess it has already been r+'d so... Can
> you at least add a comment in the relevant place saying something like
> "FIXME: This is a workaround for bug 1374175, we do need a proper solution
> for this"?.

Sure. I filed bug 1378972 for the issue, and will leave a comment. Thank you always for caring everything!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fd21ff3bc0c
Return no damage if there is no display property changes. r=heycam
https://hg.mozilla.org/mozilla-central/rev/0fd21ff3bc0c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to Emilio Cobos Álvarez [:emilio] from comment #28)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > > I don't know, that still feels like a hack, honestly. The bug seems to me
> > > that is the fact that we generate a change hint for the animation-only
> > > restyle even on initial styling,
> > 
> > Yes, right. A cumbersome point is some animations (e.g. width) need change
> > hint even on the initial styling.
> 
> Why is that? We haven't even laid out, why would we ever need a change hint?

This went unreplied, and I'm really curious about it. Any insight Hiro?
Flags: needinfo?(hikezoe)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #28)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> > > (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > > > I don't know, that still feels like a hack, honestly. The bug seems to me
> > > > that is the fact that we generate a change hint for the animation-only
> > > > restyle even on initial styling,
> > > 
> > > Yes, right. A cumbersome point is some animations (e.g. width) need change
> > > hint even on the initial styling.
> > 
> > Why is that? We haven't even laid out, why would we ever need a change hint?
> 
> This went unreplied, and I'm really curious about it. Any insight Hiro?

The comment is somewhat misleading. I meant the damage for all animation-only restyling, not only for initial one.
Flags: needinfo?(hikezoe)
Depends on: 1383988
You need to log in before you can comment on or make changes to this bug.