Closed Bug 1228128 Opened 8 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
critical

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fix-optional

People

(Reporter: jruderman, Unassigned)

References

Details

(Keywords: assertion, testcase, Whiteboard: gfx-noted)

Attachments

(2 files)

Attached file testcase
gfx/skia/skia/src/core/SkLineClipper.cpp:255: failed assertion "is_between_unsorted(r->fY, tmp[0].fY, tmp[1].fY)"
Attached file stack
Flags: needinfo?(lsalzman)
Whiteboard: gfx-noted
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.
Flags: needinfo?(lsalzman) → needinfo?(milan)
Can you remove or weaken the assertion? Assertion failures that can be triggered by content are always bugs.
(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.
Then it should be downgraded to a warning (cf bug 1080457).
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.)
(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.
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.
Depends on: 1082598
Flags: needinfo?(milan)
FYI, current trunk still hits this assertion with Skia 59.
Has Regression Range: --- → irrelevant
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Depends on: 1305151
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.