1394405 - Assertion failure: n2 >= 0

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 20170828-d10c97627b51.

Assertion failure: n2 >= 0, at /home/worker/workspace/build/src/gfx/2d/BezierUtils.cpp:333
#01: mozilla::DottedCornerFinder::FindNext at layout/painting/DottedCornerFinder.cpp:299
#02: mozilla::DottedCornerFinder::Next at layout/painting/DottedCornerFinder.cpp:200
#03: nsCSSBorderRenderer::DrawDottedCornerSlow at layout/painting/nsCSSRenderingBorders.cpp:2479
#04: nsCSSBorderRenderer::DrawDashedOrDottedCorner at layout/painting/nsCSSRenderingBorders.cpp:2395
#05: nsCSSBorderRenderer::DrawBorderSides at layout/painting/nsCSSRenderingBorders.cpp:1379
#06: nsCSSBorderRenderer::DrawBorders at layout/painting/nsCSSRenderingBorders.cpp:3519
#07: nsCSSRendering::PaintBorderWithStyleBorder at layout/painting/nsCSSRendering.cpp:881
#08: nsCSSRendering::PaintBorder at layout/painting/nsCSSRendering.cpp:649
#09: nsDisplayBorder::Paint at layout/painting/nsDisplayList.cpp:5126
#10: mozilla::FrameLayerBuilder::PaintItems at layout/painting/FrameLayerBuilder.cpp:6050
#11: mozilla::FrameLayerBuilder::DrawPaintedLayer at layout/painting/FrameLayerBuilder.cpp:6226
#12: mozilla::layers::ClientPaintedLayer::PaintThebes at gfx/layers/client/ClientPaintedLayer.cpp:174
#13: mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback at gfx/src/nsRegion.h:75
#14: mozilla::layers::ClientContainerLayer::RenderLayer at gfx/layers/client/ClientContainerLayer.h:59
#15: mozilla::layers::ClientContainerLayer::RenderLayer at gfx/layers/client/ClientContainerLayer.h:59
#16: mozilla::layers::ClientLayerManager::EndTransactionInternal at gfx/layers/client/ClientLayerManager.cpp:380
#17: mozilla::layers::ClientLayerManager::EndTransaction at gfx/layers/client/ClientLayerManager.cpp:439
#18: nsDisplayList::PaintRoot at layout/painting/nsDisplayList.cpp:2344
#19: nsLayoutUtils::PaintFrame at mfbt/RefPtr.h:130
#20: mozilla::PresShell::Paint at layout/base/PresShell.cpp:6457

Comment 1

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1394405 - Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem;

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

Comment 2

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8904185 [details]
Bug 1394405 - Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem;

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

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8904185 [details]
Bug 1394405 - Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem;

https://reviewboard.mozilla.org/r/175952/#review181822

This assertion is suspicious because it does accepts that the result of some non-trivial floating point computation can be equal to zero, but with non-trivial float arithmetic, anything that can be strictly equal to a number can usually also be sightly greater or slightly smaller than that number, and that's a fundamental issue with floating point computation and its finite precision.
My experience with dealing with this type of errors is that replacing some 32 bits floats with doubles adds precision but doesn't usually solve the problem, it mostly makes it harder to reproduce.

I think that in this case the loss of precision is acceptable and below the range of visible things, and the problem is that the assertion should accept that the value of n1 and n2 can fall slightly below zero and should be adjusted with a very small threshold like ```MOZ_ASSERT(n2 > -epsilon)```. With some value of epsilon chosen aribtrarily like 0.00001.
And if returning a negative value from that function can be a source of error, also return ```max(n1 < n2 ? n1 : n2, 0.0);```

Comment 4

[Kevin Chen[:kechen] (UTC + 8)]

> I think that in this case the loss of precision is acceptable and below the
> range of visible things, and the problem is that the assertion should accept
> that the value of n1 and n2 can fall slightly below zero and should be
> adjusted with a very small threshold like ```MOZ_ASSERT(n2 > -epsilon)```.
> With some value of epsilon chosen aribtrarily like 0.00001.
> And if returning a negative value from that function can be a source of
> error, also return ```max(n1 < n2 ? n1 : n2, 0.0);```

The value of epsilon must be bigger than 0.5 to pass the case provided by this bug; this threshold might be bigger in other cases.
Perhaps the precision error is accumulated from somewhere before this method.

Comment 5

[Nicolas Silva [:nical]]

Ouch, yeah this is because A is very small and as we divide by it the error is scaled up. This could be re-written like this:

>  Float n1 = - B + S;
>  Float n2 = - B - S;
>
>  Float epsilon = 0.0001;
>  MOZ_ASSERT(n1 >= -epsilon);
>  MOZ_ASSERT(n2 >= -epsilon);
> 
>  return max((n1 < n2 ? n1 : n2) / A, 0.0);

Bonus point because we save a division (I doubt it matters in practice).

I toyed with this a bit and the root of the issue is in the specific invocation of GetBezierPoint here: https://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/layout/painting/DottedCornerFinder.cpp#266

With a bezier curve that is sufficiently far down the y axis, we can for some points an error that actually adds up to about one pixel (so much for my earlier comment about being within the range of invisible errors).

This can be fixed by translating mCenterBezier by mInnerCurveOrigin to work in a more reasonable range and be careful about translating the result back at some point. But honestly I don't think it is worth the effort. If we just relax the assertion and accept that very very far down the page with an unrealistically high rounded rectangle a 1px error can crop up, thing work out well. We do need to clamp negative values up to something positive becasue otherwise we hit another assertion somewhere else.

How do you feel about this?

Comment 6

[Kevin Chen[:kechen] (UTC + 8)]

Hello nical, thank you for the feedback.

I've tried to deduce the center bezier point from the inner curve and outer curve.
There would still be a precision losing problem when using inner curve while using outer curve could fix the problem in this bug, but it would cause some reftest failures[1].

Since the bug is a very rare case and calculating center bezier point from inner curve(outer curve) can still have a precision losing problem, it might be advisable to relax the assertion and deal with the precision error manually.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=92b04d35180065d9d3d53f1c16f210fa04412aa6

Comment 7

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8904185 [details]
Bug 1394405 - Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem;

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

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 8904185 [details]
Bug 1394405 - Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem;

https://reviewboard.mozilla.org/r/175952/#review193556

Thanks!

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8904185 [details]
Bug 1394405 - Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175952/diff/3-4/

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8904185 [details]
Bug 1394405 - Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175952/diff/4-5/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6796e53fa9e3
Relax the assertion in CalculateDistanceToEllipticArc to deal with the floating point precision problem; r=nical

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/6796e53fa9e3