1228128 - [Skia canvas] SkLineClipper.cpp:255: failed assertion "is_between_unsorted(r->fY, tmp[0].fY, tmp[1].fY)"

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase

gfx/skia/skia/src/core/SkLineClipper.cpp:255: failed assertion "is_between_unsorted(r->fY, tmp[0].fY, tmp[1].fY)"

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Lee Salzman [:lsalzman]]

I investigated this and it is a case in the math of adding a denormal such that X + Y = X, even when Y is non-zero. This has no significant effects on stability outside of triggering the assertion itself under this testcase, and it would require some rather invasive changes just to get rid of that. I recommend we just WONTFIX this.

Comment 3

[Jesse Ruderman]

Can you remove or weaken the assertion? Assertion failures that can be triggered by content are always bugs.

Comment 4

[Lee Salzman [:lsalzman]]

(In reply to Jesse Ruderman from comment #3)
> Can you remove or weaken the assertion? Assertion failures that can be
> triggered by content are always bugs.

It's only in debug builds that this does anything at all, and removing it seems contrary to the idea of what the assertion was designed to test. I'm not sure there is a fix Skia would consider upstreaming given the headache of making something to ferret out denormals and do something sane with them.

Comment 5

[Jesse Ruderman]

Then it should be downgraded to a warning (cf bug 1080457).

Comment 6

[Jason Orendorff [:jorendorff]]

It seems like content shouldn't be able to crash the browser this easily, even in debug builds. The fuzzers depend on this property. Is it really so much to ask?

(Also, totally beside the point, I'm curious about the floating-point math here. is_between_unsorted() uses `<=` for its comparisons, not `<`... and both paths in sect_with_vertical look like they should satisfy the invariant. I can't tell how it ends up failing. ...But this is just idle curiosity.)

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> It seems like content shouldn't be able to crash the browser this easily,
> even in debug builds. The fuzzers depend on this property. Is it really so
> much to ask?
> 
> (Also, totally beside the point, I'm curious about the floating-point math
> here. is_between_unsorted() uses `<=` for its comparisons, not `<`... and
> both paths in sect_with_vertical look like they should satisfy the
> invariant. I can't tell how it ends up failing. ...But this is just idle
> curiosity.)

The test itself is legit and not really at fault.

The problem is inside sect_with_vertical(), where the result should actually be getting nudged to be within the bounds... assuming unlimited precision. However, since the denormal's effect when added on to the non-denormal can't be represented within the available precision, so it behaves as if adding 0. You'd probably need to add a small margin of error to is_between_unsorted() to account for this slightly out of bounds condition, or make sect_with_vertical() explicitly clamp the value into range of the bounds, so that the assertion is not needed in the first place. But it would be an issue best left to the upstream Skia folks to decide how they wanted to play it.

Comment 8

[Lee Salzman [:lsalzman]]

I'm going to mark this dependent on the Skia update, and I'll revisit the issue once that's safely out of the way.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

FYI, current trunk still hits this assertion with Skia 59.

Comment 10

[Lee Salzman [:lsalzman]]

As per bug 1305151, we have decided that unless the bug in question causes crashes without the assertion in release builds, then it is nothing we plan to fix. Upstream Skia does not really fuzz Skia with debugging on, so neither do we worry about it.