1393605 - stylo: panicked at 'determinant should now be 1 or -1'

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

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*)

Comment 1

[Manish Goregaokar [:manishearth]]

Caused by https://github.com/servo/servo/pull/18197, probably

Comment 2

[Manish Goregaokar [:manishearth]]

The actual determinant here is 0.00028317794

Comment 3

[Boris Chiou [:boris]]

(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.

Comment 4

[Boris Chiou [:boris]]

Mark P2 because "panic"

Comment 5

[Boris Chiou [:boris]]

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 6

[Boris Chiou [:boris]]

Attachment: Bug 1393605 - Return Err(()) if determinant is not 1 or -1 while decomposing the 2d matrix.

This may happen in some cases, and we shouldn't panic in debug mode,
so let's return Err(()) for it to fall back to discrete animation.

Review commit: https://reviewboard.mozilla.org/r/174114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174114/

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 8

[Boris Chiou [:boris]]

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.

Comment 9

[Boris Chiou [:boris]]

Attachment: Servo PR, #18305

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/autoland/rev/00ccf80218b9

NI Boris to land the crashtest.

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1393605 - Fix gecko assertion and add one crashtest.

Review commit: https://reviewboard.mozilla.org/r/174780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174780/

Comment 12

[Boris Chiou [:boris]]

Comment on attachment 8902523 [details]
Bug 1393605 - Return Err(()) if determinant is not 1 or -1 while decomposing the 2d matrix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174114/diff/1-2/

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8903012 [details]
Bug 1393605 - Fix gecko assertion and add one crashtest.

https://reviewboard.mozilla.org/r/174780/#review179848

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b53b0454742b
Fix gecko assertion and add one crashtest. r=birtles

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b53b0454742b