1395974 - Assertion failure: aScale >= 0.0f (negative scaling factors must be handled manually)

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes an assertion in m-c rev 20170901-a3585c77e2b1

Assertion failure: aScale >= 0.0f (negative scaling factors must be handled manually), at /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCoord.h:128
#0: NSCoordSaturatingNonnegativeMultiply, at gfx/src/nsCoord.h:127
#1: mozilla::nsImageRenderer::ComputeConstrainedSize, at layout/painting/nsImageRenderer.cpp:378
#2: nsCSSRendering::PrepareImageLayer, at layout/painting/nsCSSRendering.cpp:2978
#3: nsDisplayMask::nsDisplayMask, at layout/painting/nsDisplayList.cpp:8741
#4: nsIFrame::BuildDisplayListForStackingContext, at layout/generic/nsFrame.cpp:2713
#5: nsIFrame::BuildDisplayListForChild, at layout/generic/nsFrame.cpp:3191
#6: nsCanvasFrame::BuildDisplayList, at layout/generic/nsCanvasFrame.cpp:599
#7: nsIFrame::BuildDisplayListForChild, at layout/generic/nsFrame.cpp:3242
#8: mozilla::ScrollFrameHelper::BuildDisplayList, at layout/generic/nsGfxScrollFrame.cpp:3539
#9: nsIFrame::BuildDisplayListForChild, at layout/generic/nsFrame.cpp:3242
#10: mozilla::ViewportFrame::BuildDisplayList, at layout/generic/ViewportFrame.cpp:65
#11: nsIFrame::BuildDisplayListForStackingContext, at layout/generic/nsFrame.cpp:2591
#12: nsLayoutUtils::PaintFrame, at layout/base/nsLayoutUtils.cpp:3639
#13: mozilla::PresShell::Paint, at layout/base/PresShell.cpp:6467

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I will take a look at this.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The test case caused an integer overflow at NSCoordSaturatingAdd in nsBlockFrame::ComputeFinalSize[1]. 

[1] https://hg.mozilla.org/mozilla-central/file/b4c1ad9565ee/layout/generic/nsBlockFrame.cpp#l1599

Comment 3

[Hiroyuki Ikezoe (:hiro)]

The real overflow (not integer over flow, it's nscoord overflow) happens at nsMargin::LeftRight() and nsMargin.TopBottom() before doing the NSCoordSaturatingAdd there.  The parent struct of nsMargin, BaseMargin, implements the LeftRight() and TopBottom() methods that just do 'left + right' or 'top + bottom'.  So if both of left and right are nscoord_MAX, the result is greater than nscoord_MAX.

Daniel, how should we handle these cases?  I thought template specialization for BaseMargin<nscoord, nsMargin>::LeftRight() and other methods, just like this try;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3682f28fb9fd322928a4895eccb687911c38515c

But the try result noticed me that there are some NS_PRECONDITION(xx != NS_AUTOHEIGHT) check, which seems to me that NS_AUTOHEIGHT (= nscoord_MAX) has special meaning that it's 'auto' value, right? If so, should we return nscood_MAX - 1 from NSCoordSaturatingAdd()?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Daniel, ^

Comment 5

[Daniel Holbert [:dholbert]]

Yeah -- there are all sorts of layout additions that can produce values greater than nscoord_MAX.  Pretty much any "size + size" addition.  We cannot & should not use something like NSCoordSaturatingAdd for every single one of these cases.

NSCoordSaturatingAdd was originally meant for things like table layout, where we e.g. sum up of all the cell preferred-widths, and then we need to allocate a share of the actual table-width to each cell according to its desired share.  In that case, if the cells have huge widths (clamped to nscoord_MAX), the point is to make the summation "stick" once it hits nscoord_MAX (and then we have a special case for how we hand out space if we do hit that threshold -- giving all the free space to the largest cell(s), or something like that, as if those cells all had "infinitely-great desire" as compared to the others).

In this case, though, I think we have to just let the arithmetic overflow, and we need our assertions to be able to handle the possibility that we might have overflowed.  Depending on the circumstances, that might mean adding a bail-out if some size is less than 0, or it might mean just relaxing the asserted condition.

Comment 6

[Daniel Holbert [:dholbert]]

The idea is that generally, layout code should be *safe* in the face of nscoord overflow -- we'll just position a div way off to the left of the screen, and we might trip a few assertions for matching NS_AUTOHEIGHT if input values are specified just right, but we shouldn't crash or hang.

In cases where overflow could cause hanging / crashing / serious corruption, that may be a case for NSCoordSaturatingAdd or std::max(0, valWhichMustNotBeNegative).  But otherwise, we tend to just let stuff overflow.

Regarding NS_PRECONDITION(xx != NS_AUTOHEIGHT) -- it's known that fuzzer-generated testcases can trip such assertions, I think (though we sometimes take that as a signal that the assertion should be softened to a warning).  I'm pretty sure there's code that depends on NSCoordSaturdatingAdd returning nscoord_MAX (and not nscoord_MAX - 1), so I don't think that's a viable change, at least not unless it was part of a bigger rethinking of how we do this sort of arithmetic.  (Ideally it'd be nice to have a different sentinel value for NS_AUTOHEIGHT / NS_INTRINSICSCIZE / etc. that is completely distinct from our "saturating addition" target, but that's probably a broader change that's beyond the scope of this one fuzzer-bug.)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Thanks for the explanations! That's great to know.

(In reply to Daniel Holbert [:dholbert] from comment #6)
> In cases where overflow could cause hanging / crashing / serious corruption,
> that may be a case for NSCoordSaturatingAdd or std::max(0,
> valWhichMustNotBeNegative).  But otherwise, we tend to just let stuff
> overflow.

Especially this,

> I'm pretty sure there's code that depends on
> NSCoordSaturdatingAdd returning nscoord_MAX (and not nscoord_MAX - 1),

and this. :)

I will do a bail-out from where the assertion happens.
Thanks!

Comment 8

[Daniel Holbert [:dholbert]]

setting this as "fix-optional" for 57 (though unless we've got a fix in the next couple days, it should probably really be "wontfix" for 57, because it's nearly 57beta and I'm not sure the risk-to-reward of a patch here would merit squeezing it in at the end of the cycle.)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 6bdef7ba8b4108a996b9f61ef9f81c5ea6c93017
INFO: First bad revision: 944f98dcb83b740910d547b97181258ed0890afe
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6bdef7ba8b4108a996b9f61ef9f81c5ea6c93017&tochange=944f98dcb83b740910d547b97181258ed0890afe

Comment 10

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 11

[Tyson Smith [:tsmith]]

This issue is no longer reported by the fuzzers and the attached testcase is no longer reproducing the issue.

dholbert: Is the testcase worth landing or can we close this issue?

Comment 12

[Daniel Holbert [:dholbert]]

Sure, we might as well land the testcase.

(The testcase used a large animation-duration, 1855 i.e. nearly 2 seconds, and in practice we'll probably move to the next test before that time passes, so we'll never see the animation finish in a crashtest. Not sure if that was needed to trigger the assertion in the original testcase or not. In any case, I'll add a second variant of the testcase with a shorter duration -- maybe 5ms -- in case it was important to get a chance to sample the end of the animation.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Bug 1395974: Add crashtests for this bug. r?#layout

I'm adding the crashtest from the bug, as well as a variant with a much shorter
animation duration, in case the bad-outcome depended on reaching the end of the
animation.

(We could use add reftest-animate etc. and wait for the animation to be done,
but I'm not sure if that was necessary; it's likely that the version with the
shortened duration will complete at least some of the time in CI, giving us a
chance to sample the animation end-state.)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/88fd6b388e14
https://hg.mozilla.org/integration/autoland/rev/7ba156cad885
Add crashtests for this bug. r=layout-reviewers,emilio

Comment 15

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7ba156cad885