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)
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)
[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
Assignee | ||
Comment 1•7 years ago
|
||
Hmm, I don't see the panic on m-c (f8dd3f21e434).
Comment 2•7 years ago
|
||
I can't repro on latest Nightly on Windows either.
Comment 3•7 years ago
|
||
Bob, can you confirm that this reproduces on the latest Nightly?
Flags: needinfo?(bob)
Assignee | ||
Comment 4•7 years ago
|
||
Ni? to me since I suspect this might happen. I will try to write a test case.
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Updated•7 years ago
|
Priority: -- → P2
Reporter | ||
Comment 6•7 years ago
|
||
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.
Comment 7•7 years ago
|
||
Requesting tracking on all outstanding p2 stylo bugs.
Assignee | ||
Comment 8•7 years ago
|
||
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.
Updated•7 years ago
|
status-firefox58:
--- → affected
Assignee | ||
Comment 9•7 years ago
|
||
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
Assignee | ||
Comment 10•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
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 16•7 years ago
|
||
mozreview-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 17•7 years ago
|
||
mozreview-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+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8911648 [details] Bug 1401801 - Crash test. https://reviewboard.mozilla.org/r/183048/#review188224
Attachment #8911648 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 19•7 years ago
|
||
Thanks for the review!
Attachment #8911044 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8911645 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8911646 -
Attachment is obsolete: true
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/edcb9bd4cdd0 https://hg.mozilla.org/mozilla-central/rev/8e33cb1c0b78
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 23•7 years ago
|
||
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?
Updated•7 years ago
|
Comment 24•7 years ago
|
||
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+
Comment 25•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9458811cd8ba https://hg.mozilla.org/releases/mozilla-beta/rev/0da6b667a44d https://hg.mozilla.org/releases/mozilla-beta/rev/4320c131a141
tracking-firefox57:
? → ---
Comment 26•7 years ago
|
||
(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.
Description
•