1352380 - Crash in OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xrealloc | nsTArray_base<T>::EnsureCapacity<T> | nsTArray_Impl<T>::InsertElementAt<T> | ResolvePremultipliedAlpha

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-8e884bc2-6782-4517-abd4-fcb222170331.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	mozglue.dll 	mozalloc_abort(char const* const) 	memory/mozalloc/mozalloc_abort.cpp:33
1 	mozglue.dll 	mozalloc_handle_oom(unsigned int) 	memory/mozalloc/mozalloc_oom.cpp:46
2 	mozglue.dll 	moz_xrealloc 	memory/mozalloc/mozalloc.cpp:107
3 	xul.dll 	nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned int, unsigned int) 	obj-firefox/dist/include/nsTArray-inl.h:183
4 	xul.dll 	nsTArray_Impl<ColorStop, nsTArrayInfallibleAllocator>::InsertElementAt<ColorStop&, nsTArrayInfallibleAllocator>(unsigned int, ColorStop&) 	obj-firefox/dist/include/nsTArray.h:2172
5 	xul.dll 	ResolvePremultipliedAlpha 	layout/painting/nsCSSRendering.cpp:2694
6 	xul.dll 	nsCSSRendering::PaintGradient(nsPresContext*, gfxContext&, nsStyleGradient*, nsRect const&, nsRect const&, nsRect const&, nsSize const&, mozilla::gfx::IntRectTyped<mozilla::CSSPixel> const&, nsSize const&, float) 	layout/painting/nsCSSRendering.cpp:3117
7 	xul.dll 	nsCSSRendering::PaintGradient(nsPresContext*, gfxContext&, nsStyleGradient*, nsRect const&, nsRect const&, nsRect const&, nsSize const&, mozilla::gfx::IntRectTyped<mozilla::CSSPixel> const&, nsSize const&, float) 	layout/painting/nsCSSRendering.cpp:3117
8 	xul.dll 	gfxContext::SetMatrix(gfxMatrix const&) 	gfx/thebes/gfxContext.cpp:268
9 	xul.dll 	mozilla::gfx::DrawTargetSkia::SetTransform(mozilla::gfx::Matrix const&) 	gfx/2d/DrawTargetSkia.cpp:1966
10 	mozglue.dll 	mozglue.dll@0x2abf 	
11 		@0x3fefffff 	
12 	xul.dll 	mozilla::layers::BasicPaintedLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicPaintedLayer.cpp:129
13 	xul.dll 	nsLayoutUtils::SurfaceFromElementResult::SurfaceFromElementResult(nsLayoutUtils::SurfaceFromElementResult const&) 	
14 	xul.dll 	nsImageRenderer::~nsImageRenderer() 	layout/painting/nsCSSRendering.cpp:5241
15 	xul.dll 	nsCSSRendering::PrepareImageLayer(nsPresContext*, nsIFrame*, unsigned int, nsRect const&, nsRect const&, nsStyleImageLayers::Layer const&, bool*) 	layout/painting/nsCSSRendering.cpp:3883

this crash signature on 32bit versions of the browser various windows versions is spiking up since yesterday (2017-03-30) across various channels, which would indicate that there is some external factor playing into it.
the signature is too new so there aren't any correlations auto-generated for it yet - manually looking into addon/module correlations didn't bring up anything obvious, so maybe it's related to a website change...?

Comment 1

[[:philipp]]

visiting this site reproducibly triggers the crash currently, also on non-win systems: https://job-like.com/company/540/salary

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Yeah, this crash easily reproduces for me too with the link from comment 1. Can you take a look, Markus?

Comment 3

[Markus Stange [:mstange]]

ResolvePremultipliedAlpha is encountering a color stop that has total garbage values. I'm currently spinning up a build on Firefox so that I can use rr to find out where those values are coming from.

Comment 4

[Markus Stange [:mstange]]

We're trying to insert -2147483648 color stops, because that's the number we compute here:

> size_t stepCount = NSToIntFloor(fabsf(leftStop.mColor.a - rightStop.mColor.a) / kAlphaIncrementPerGradientStep);

And that's because rightStop.mColor.a is NaN.

That color component is NaN because it was computed like this:

> gfx::Float alpha = color1.a + multiplier * (color2.a - color1.a);

with multiplier being NaN.

> float multiplier = powf(relativeOffset, logf(.5f) / logf(midpoint))

midpoint is -0.0169491358

> float midpoint = (offset - offset1) / (offset2 - offset1);

offset is 0.699999988
offset1 is 0.704999983
offset2 is 1

These come from:

> float offset1 = stops[x-1].mPosition;
> float offset2 = stops[x+1].mPosition;
> float offset = stops[x].mPosition;

So our stops aren't correctly sorted by mPosition.

And that's because this adjustment in nsCSSGradientRenderer::Create:

> position = std::max(position, stops[i - 1].mPosition);

was ineffective because stops[i - 1] is a stop that did not have a position assigned (its mPosition is zero, and firstUnsetPosition == i - 1).

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1352380 - Make sure gradient stops have ordered mPosition variables.

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8853526 [details]
Bug 1352380 - Make sure gradient stops have ordered mPosition variables.

https://reviewboard.mozilla.org/r/125582/#review128760

r=dbaron, although I'd somewhat prefer the following change:

::: layout/painting/nsCSSRenderingGradients.cpp:632
(Diff revision 1)
> -      position = std::max(position, stops[i - 1].mPosition);
> +      double previousPosition = stops[i - 1].mPosition;
> +      if (firstUnsetPosition > 0) {
> +        previousPosition = stops[firstUnsetPosition - 1].mPosition;
> +      }

It seems a little cleaner to do:
  uint32_t previousPositionIndex = (firstUnsetPosition > 0) ? firstUnsetPosition - 1 : i - 1;
  position = std::max(position, stops[previousPositionIndex].mPosition);
given that otherwise you are putting an uninitialized value in a double (which in some cases can even crash!).

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8853526 [details]
Bug 1352380 - Make sure gradient stops have ordered mPosition variables.

https://reviewboard.mozilla.org/r/125582/#review128760

> It seems a little cleaner to do:
>   uint32_t previousPositionIndex = (firstUnsetPosition > 0) ? firstUnsetPosition - 1 : i - 1;
>   position = std::max(position, stops[previousPositionIndex].mPosition);
> given that otherwise you are putting an uninitialized value in a double (which in some cases can even crash!).

I agree that the ternary expression is preferable. The reason I shied away from it was that the expression didn't fit into one line and I was unsure where to add the line breaks... I'll just add them in front of the question mark and in front of the colon.

I don't think the value is actually uninitialized, though - we initialize it with zero in stops.AppendElement(ColorStop(0, ...));

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

We could still potentially uplift this to 53 once it lands, if you think it's a good idea.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8853526 [details]
Bug 1352380 - Make sure gradient stops have ordered mPosition variables.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70940262ce18
Make sure gradient stops have ordered mPosition variables. r=dbaron

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70940262ce18

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Thanks for including a test with this :). Please request Aurora/Beta/ESR52 approval on this when you get a chance. Doesn't seem worth ESR45 uplift IMO.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8853526 [details]
Bug 1352380 - Make sure gradient stops have ordered mPosition variables.

Approval Request Comment
[Feature/Bug causing the regression]: Gradient midpoints, bug 1074056
[User impact if declined]: crash
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This patch only affects a very small code flow path, whose handling was clearly wrong before.
[String changes made/needed]: none

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's a crash, can be triggered unintentionally by a CSS gradient.
User impact if declined: Crashes on random websites
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): extremely low
String or UUID changes made by this patch: none

Comment 14

[Markus Stange [:mstange]]

Attachment: branch patch

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Looking back at the crash reports now I would rather hold off on uplift. We are in the last week before release and there are fewer than 100 crashes on 52.0.2 release. We could still take this for aurora.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a0c50ce045d

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8853526 [details]
Bug 1352380 - Make sure gradient stops have ordered mPosition variables.

I don't see any instances of this crash on ESR52. Given that, we can just wontfix this for esr52