Closed Bug 1401801 Opened 7 years ago Closed 7 years ago

stylo: thread '<unnamed>' panicked at 'Display animation should only happen for SMIL', servo/components/style/matching.rs:227

Categories

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

57 Branch
defect

Tracking

()

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

People

(Reporter: bc, Assigned: hiro)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

Attached file Windows Debug Log
[Tracking Requested - why for this release]:

1. https://www.vix.com/es/ciencia/188599/la-disolucion-de-cadaveres-y-el-fin-de-los-cementerios?utm_term=Autofeed&utm_campaign=Echobox&utm_medium=VixExploraES&utm_source=Facebook#link_time=1505829743

2. thread '<unnamed>' panicked at 'Display animation should only happen for SMIL', Z:/build/build/src/servo/components/style/matching.rs:227

3  xul.dll!std::panicking::begin_panic<&str> [panicking.rs:0ade339411587887bf01bcfa2e9ae4414c8900d4 : 511 + 0x12]
    rbx = 0x0000000000000035   rbp = 0x00000003d99f9fc0
    rsp = 0x00000003d99fa1b0   r12 = 0x00000003d99fae50
    r13 = 0x00000003d99fae40   r14 = 0x0000000000000001
    r15 = 0x0000015983502128   rip = 0x00007ffcc45d8069
    Found by: call frame info
 4  xul.dll!style::matching::PrivateMatchMethods::handle_display_change_for_smil_if_needed<style::gecko::wrapper::GeckoElement> [<panic macros> : 3 + 0x18]
    rbx = 0x0000000000000035   rbp = 0x00000003d99f9fc0
    rsp = 0x00000003d99fa200   r12 = 0x00000003d99fae50
    r13 = 0x00000003d99fae40   r14 = 0x0000000000000001
    r15 = 0x0000015983502128   rip = 0x00007ffcc46a0e7d
    Found by: call frame info
 5  xul.dll!style::matching::MatchMethods::finish_restyle<style::gecko::wrapper::GeckoElement> [matching.rs:a0eb21bf55e1 : 542 + 0x1b]
    rbx = 0x0000000000000035   rbp = 0x00000003d99f9fc0
    rsp = 0x00000003d99fa290   r12 = 0x00000003d99fae50
    r13 = 0x00000003d99fae40   r14 = 0x0000000000000001
    r15 = 0x0000015983502128   rip = 0x00007ffcc469fa85
    Found by: call frame info
 6  xul.dll!style::traversal::compute_style<style::gecko::wrapper::GeckoElement> [traversal.rs:a0eb21bf55e1 : 754 + 0x1d]
    rbx = 0x0000000000000035   rbp = 0x00000003d99f9fc0
    rsp = 0x00000003d99fa440   r12 = 0x00000003d99fae50
    r13 = 0x00000003d99fae40   r14 = 0x0000000000000001
    r15 = 0x0000015983502128   rip = 0x00007ffcc45f9068
    Found by: call frame info
Hmm, I don't see the panic on m-c (f8dd3f21e434).
I can't repro on latest Nightly on Windows either.
Bob, can you confirm that this reproduces on the latest Nightly?
Flags: needinfo?(bob)
Ni? to me since I suspect this might happen. I will try to write a test case.
Flags: needinfo?(hikezoe)
Though I have no idea how to solve this panic, I take this.
It seems to me that the panic is not a problem on release build.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
Flags: needinfo?(bob)
Priority: -- → P2
Reproduced with local debug builds from this morning on Fedora for Nightly/58 and Beta/57. Reproduced in Bughunter for both 57 and 58 on Windows and Linux.  I haven't seen any opt crashes yet.
Requesting tracking on all outstanding p2 stylo bugs.
Attached file Crash test for this bug (obsolete) —
Here is a crash test for this bug. In this test case, I used Element.animate() but as far as I checked the site in comment 0 does not use it at all. So there must be a case that causes this bug with CSS animations or transitions, but as far as I can tell it's pretty hard to make this panic possible since any CSS animations and transitions stop working if the target element is display: none. So changing display property from 'none' during running CSS animations or transitions is hard to reproduce.
I have been thinking/checking that there are other cases that display property change happens during animation-only restyle. I think there is no  case other than SMIL and CSSOM. So we can just skip handle_display_change_for_smil_if_needed (i.e. setting eRestyle_Subtree for descendant element in the display:none subtree) since the descendant elements will be traversed in a subsequent normal traversal.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c349dbba30cfa3f83b4bb4f44b6e746fd6d2c7f
Do'h! There was a horrible mistake that the patch makes process_animations() not early returning in case of animation-only restyle.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=005b8c18d9deaaff60f8e8f254f9ff204e1cd4bd
Comment on attachment 8911645 [details]
Bug 1401801 - Use 4-spaces instead of 2-spaced.

https://reviewboard.mozilla.org/r/183042/#review188218
Attachment #8911645 - Flags: review?(bbirtles) → review+
Comment on attachment 8911646 [details]
Bug 1401801 - Handle display property change from 'none' only if there is restyle hint for SMIL.

https://reviewboard.mozilla.org/r/183044/#review188220

::: commit-message-c8f65:3
(Diff revision 1)
> +When display property is changed from 'none' by CSSOM during animation-only
> +resyle, all descendant elements in the display none subtree will be traversed

(Nit: This would be a little more clear with a comma after CSSOM. i.e. "We only need to handle changes when the display property is changed to 'none' when we have a restyle hint for SMIL. The only other case where we expect to see changes to the display property during an animation are from using the CSSOM. However, when the display property is changed from 'none' by the CSSOM, during the animation-only restyle we can skip all descendants since they will be traversed in the subsequent normal traversal...")

::: commit-message-c8f65:7
(Diff revision 1)
> +
> +When display property is changed from 'none' by CSSOM during animation-only
> +resyle, all descendant elements in the display none subtree will be traversed
> +in a subsequent normal traversal because at that time we flush style sheets and
> +traverse all elements in the document. So we don't need to care about the
> +descendants during animaiton-only restyle.

nit: animation
Attachment #8911646 - Flags: review?(bbirtles) → review+
Comment on attachment 8911647 [details]
Bug 1401801 - A reftest for a child element in animating element that the display property is changed from none by CSSOM.

https://reviewboard.mozilla.org/r/183046/#review188222
Attachment #8911647 - Flags: review?(bbirtles) → review+
Attached file Servo PR
Thanks for the review!
Attachment #8911044 - Attachment is obsolete: true
Attachment #8911645 - Attachment is obsolete: true
Attachment #8911646 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edcb9bd4cdd0
A reftest for a child element in animating element that the display property is changed from none by CSSOM. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8e33cb1c0b78
Crash test. r=birtles
Request uplift?
Flags: needinfo?(hikezoe)
Comment on attachment 8911658 [details] [review]
Servo PR

Approval Request Comment
[Feature/Bug causing the regression]: Nothing particular, it starts with stylo
[User impact if declined]: User should not see any visual differences without this patch
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Both automation tests in this bug
[Is the change risky?]: Not likely
[Why is the change risky/not risky?]: This patch does just avoid calling an unnecessary function that caused an assertion on a certain condition on debug builds, the condition hardly happens.
[String changes made/needed]: None

Note that corresponding commit for this PR is https://hg.mozilla.org/mozilla-central/rev/0354d1f5073e .
Flags: needinfo?(hikezoe)
Attachment #8911658 - Flags: approval-mozilla-beta?
Comment on attachment 8911658 [details] [review]
Servo PR

We don't want to have thread panicking, taking it.
Should be in 57b4, gtb tomorrow Thursday
Attachment #8911658 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Hiroyuki Ikezoe's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: