Closed
Bug 1393605
Opened 7 years ago
Closed 7 years ago
stylo: panicked at 'determinant should now be 1 or -1'
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
People
(Reporter: truber, Assigned: boris)
References
(Blocks 2 open bugs)
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 1 obsolete file)
The attached testcase causes a panic in m-c rev 20170824-892c8916ba32 with stylo enabled by pref.
thread '<unnamed>' panicked at 'determinant should now be 1 or -1', /home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-4d3f3573eb5f1581/out/properties.rs:90865
stack backtrace:
0: 0x7f5328b423d3 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hcab99e0793da62c7
1: 0x7f5328b3d6f6 - std::sys_common::backtrace::_print::hbfe5b0c7e79c0711
2: 0x7f5328b4fa6a - std::panicking::default_hook::{{closure}}::h9ba2c6973907a2be
3: 0x7f5328b4f66b - std::panicking::default_hook::he4d55e2dd21c3cca
4: 0x7f5328b4feba - std::panicking::rust_panic_with_hook::ha138c05cd33ad44d
5: 0x7f5328738ffa - std::panicking::begin_panic::h3893082380049d75
6: 0x7f532884250e - style::properties::animated_properties::decompose_2d_matrix::hf348b1f062a35f9d
[Parent 29953] WARNING: stylo: No docshell yet, assuming Gecko style system: file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 13446
7: 0x7f532856a6c5 - style::properties::animated_properties::<impl style::values::animated::Animate for style::properties::longhands::transform::computed_value::ComputedMatrix>::animate::hdfafca52f4cd648d
8: 0x7f53283bb94a - Servo_MatrixTransform_Operate
9: 0x7f5326a0daf4 - void nsStyleTransformMatrix::ProcessMatrixOperator<nsStyleTransformMatrix::Interpolate>(mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits>&, nsCSSValue::Array const*, mozilla::GeckoStyleContext*, nsPresContext*, mozilla::RuleNodeCacheConditions&, nsStyleTransformMatrix::TransformReferenceBox&, bool*)
10: 0x7f5326a0d60f - nsStyleTransformMatrix::ReadTransforms(nsCSSValueList const*, nsStyleContext*, nsPresContext*, mozilla::RuleNodeCacheConditions&, nsStyleTransformMatrix::TransformReferenceBox&, float, bool*)
11: 0x7f5326c94aa6 - nsDisplayTransform::GetResultingTransformMatrixInternal(nsDisplayTransform::FrameTransformProperties const&, nsPoint const&, float, unsigned int, nsRect const*)
12: 0x7f5326c94f2e - nsDisplayTransform::GetResultingTransformMatrix(nsIFrame const*, nsPoint const&, float, unsigned int, nsRect const*)
13: 0x7f5326c95408 - nsDisplayTransform::TransformRect(nsRect const&, nsIFrame const*, nsRect const*)
14: 0x7f5326b127ac - nsIFrame::FinishAndStoreOverflow(nsOverflowAreas&, nsSize, nsSize*, nsStyleDisplay const*)
15: 0x7f5326b12a61 - nsIFrame::FinishAndStoreOverflow(mozilla::ReflowOutput*, nsStyleDisplay const*)
16: 0x7f5326b2279f - nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&)
17: 0x7f5326b0e25d - nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*)
18: 0x7f5326b235c5 - nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&)
19: 0x7f5326b0e25d - nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*)
Flags: in-testsuite?
Comment 1•7 years ago
|
||
Caused by https://github.com/servo/servo/pull/18197, probably
Flags: needinfo?(boris.chiou)
Comment 2•7 years ago
|
||
The actual determinant here is 0.00028317794
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Manish Goregaokar [:manishearth] from comment #2)
> The actual determinant here is 0.00028317794
Thanks, Manishearth. I will try to use another way to avoid this case.
Flags: needinfo?(boris.chiou)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → boris.chiou
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•7 years ago
|
||
OK, Gecko also has this assertion, but it's not a fetal assert. In other words, we use NS_ASSERTION(), instead of MOZ_ASSERT(), so that's why we didn't notice this error on Gecko.
This may be a bug in Gecko, I think, so I won't revise the decomposition of 2d matrix. Instead, downgrade the debug_assert().
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8902523 [details]
Bug 1393605 - Return Err(()) if determinant is not 1 or -1 while decomposing the 2d matrix.
https://reviewboard.mozilla.org/r/174114/#review179364
::: servo/components/style/properties/helpers/animated_properties.mako.rs:1715
(Diff revision 1)
> m22 /= scale_y;
> shear_xy /= scale_y;
>
> let determinant = m11 * m22 - m12 * m21;
> - debug_assert!(0.99 < determinant.abs() && determinant.abs() < 1.01,
> - "determinant should now be 1 or -1");
> + if 0.99 > determinant.abs() || determinant.abs() > 1.01 {
> + // determinant should now be 1 or -1".
Nit no need for the " at the end of the comment.
Also, I'd put the comment before the "if" (since it explains what we're checking, not the failure case) and make it start with a capital letter:
// Determinant should now be 1 or -1
Attachment #8902523 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8902523 [details]
Bug 1393605 - Return Err(()) if determinant is not 1 or -1 while decomposing the 2d matrix.
https://reviewboard.mozilla.org/r/174114/#review179364
> Nit no need for the " at the end of the comment.
>
> Also, I'd put the comment before the "if" (since it explains what we're checking, not the failure case) and make it start with a capital letter:
>
> // Determinant should now be 1 or -1
Oh yes. I should update this sentence and put it before "if". Thanks.
Assignee | ||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/00ccf80218b9
NI Boris to land the crashtest.
Flags: needinfo?(boris.chiou)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(boris.chiou)
Keywords: leave-open
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8902523 -
Attachment is obsolete: true
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8903012 [details]
Bug 1393605 - Fix gecko assertion and add one crashtest.
https://reviewboard.mozilla.org/r/174780/#review179848
Attachment #8903012 -
Flags: review?(bbirtles) → review+
Comment 14•7 years ago
|
||
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b53b0454742b
Fix gecko assertion and add one crashtest. r=birtles
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•