Closed Bug 1352380 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, critical)

53 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 + wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: philipp, Assigned: mstange)

References

(Depends on 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

[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...?
visiting this site reproducibly triggers the crash currently, also on non-win systems: https://job-like.com/company/540/salary
OS: Windows → All
Hardware: x86 → All
Yeah, this crash easily reproduces for me too with the link from comment 1. Can you take a look, Markus?
Flags: needinfo?(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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(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 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!).
Attachment #8853526 - Flags: review?(dbaron) → review+
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, ...));
We could still potentially uplift this to 53 once it lands, if you think it's a good idea.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/70940262ce18
Make sure gradient stops have ordered mPosition variables. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/70940262ce18
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Flags: needinfo?(mstange)
Flags: in-testsuite+
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
Flags: needinfo?(mstange)
Attachment #8853526 - Flags: approval-mozilla-esr52?
Attachment #8853526 - Flags: approval-mozilla-beta?
Attachment #8853526 - Flags: approval-mozilla-aurora?
Attached patch branch patchSplinter Review
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.
Attachment #8853526 - Flags: approval-mozilla-beta?
Attachment #8853526 - Flags: approval-mozilla-beta-
Attachment #8853526 - Flags: approval-mozilla-aurora?
Attachment #8853526 - Flags: approval-mozilla-aurora+
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
Attachment #8853526 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
Depends on: 1355130
You need to log in before you can comment on or make changes to this bug.