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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(2 files)
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
Assignee | ||
Comment 1•7 years ago
|
||
A with WIP patches, not work well yet.. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6557e57be904986f8858fe2310efbefbbb53c7d1
Comment 2•7 years ago
|
||
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?).
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
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!
Assignee | ||
Comment 5•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment 6•7 years ago
|
||
(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)
Assignee | ||
Comment 7•7 years ago
|
||
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?
Assignee | ||
Comment 8•7 years ago
|
||
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
Assignee | ||
Comment 9•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
(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.
Assignee | ||
Comment 12•7 years ago
|
||
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
Assignee | ||
Comment 13•7 years ago
|
||
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).
Assignee | ||
Comment 14•7 years ago
|
||
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
Assignee | ||
Comment 15•7 years ago
|
||
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
Assignee | ||
Comment 16•7 years ago
|
||
(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!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
Moved checking the flag point into the if block for NAC elements. Try looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d7941085a19d3eddcb2fcb40698c3b76340ced7
Assignee | ||
Comment 20•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
mozreview-review |
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+
Comment 24•7 years ago
|
||
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?
Assignee | ||
Comment 25•7 years ago
|
||
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)
Comment 26•7 years ago
|
||
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)
Assignee | ||
Comment 27•7 years ago
|
||
(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.
Comment 28•7 years ago
|
||
(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"?.
Assignee | ||
Comment 29•7 years ago
|
||
(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!
Assignee | ||
Comment 30•7 years ago
|
||
A final try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=b883f09ba29f81cf3b7d8ea5d635b44ecda1f5b0
Comment hidden (mozreview-request) |
Comment 32•7 years ago
|
||
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
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0fd21ff3bc0c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 34•7 years ago
|
||
(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)
Assignee | ||
Comment 36•7 years ago
|
||
(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)
You need to log in
before you can comment on or make changes to this bug.
Description
•