1341149 - Use of uninitialized value in SetupImageLayerClip

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

This was found using a firefox-valgrind-opt build from task cluster:
https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-central.latest.firefox/gecko.v2.mozilla-central.latest.firefox.linux64-valgrind-opt

Valgrind was launched using the following args:
valgrind -q --smc-check=all-non-file --show-mismatched-frees=no --show-possibly-lost=no --read-inline-info=yes --track-origins=yes --vex-iropt-register-updates=allregs-at-mem-access ...

Conditional jump or move depends on uninitialised value(s)
   at 0x12A04E99: SetupImageLayerClip (nsCSSRendering.cpp:2147)
   by 0x12A04E99: nsCSSRendering::PaintStyleImageLayerWithSC(nsCSSRendering::PaintBGParams const&, nsStyleContext*, nsStyleBorder const&) (nsCSSRendering.cpp:3425)
   by 0x1297704A: PaintMaskSurface(nsSVGIntegrationUtils::PaintFramesParams const&, mozilla::gfx::DrawTarget*, float, nsStyleContext*, nsTArray<nsSVGMaskFrame*> const&, gfxMatrix const&, nsPoint const&) (nsSVGIntegrationUtils.cpp:529)
   by 0x12977A61: CreateAndPaintMaskSurface (nsSVGIntegrationUtils.cpp:611)
   by 0x12977A61: nsSVGIntegrationUtils::PaintMaskAndClipPath(nsSVGIntegrationUtils::PaintFramesParams const&) (nsSVGIntegrationUtils.cpp:972)
   by 0x129F4A17: nsDisplayMask::PaintAsLayer(nsDisplayListBuilder*, nsRenderingContext*, mozilla::layers::LayerManager*) (nsDisplayList.cpp:7711)
   by 0x12A0F19D: PaintInactiveLayer (FrameLayerBuilder.cpp:3706)
   by 0x12A0F19D: mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) (FrameLayerBuilder.cpp:6017)
    by 0x12A0F8D1: mozilla::FrameLayerBuilder::DrawPaintedLayer(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*) (FrameLayerBuilder.cpp:6209)
    by 0x11A8EDE9: mozilla::layers::ClientPaintedLayer::PaintThebes(nsTArray<mozilla::layers::ReadbackProcessor::Update>*) (ClientPaintedLayer.cpp:91)
    by 0x11A92568: mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) (ClientPaintedLayer.cpp:139)
    by 0x11A9078D: mozilla::layers::ClientContainerLayer::RenderLayer() (ClientContainerLayer.h:57)
    by 0x11A9078D: mozilla::layers::ClientContainerLayer::RenderLayer() (ClientContainerLayer.h:57)
    by 0x11A8D8CC: mozilla::layers::ClientLayerManager::EndTransactionInternal(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*, mozilla::layers::LayerManager::EndTransactionFlags) (ClientLayerManager.cpp:358)
    by 0x11A98B53: mozilla::layers::ClientLayerManager::EndTransaction(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*, mozilla::layers::LayerManager::EndTransactionFlags) (ClientLayerManager.cpp:411)
  Uninitialised value was created by a stack allocation
    at 0x12A049D5: nsCSSRendering::PaintStyleImageLayerWithSC(nsCSSRendering::PaintBGParams const&, nsStyleContext*, nsStyleBorder const&) (nsCSSRendering.cpp:3255)

Comment 1

[Markus Stange [:mstange]]

Looks like some of the early exits in nsCSSRendering::GetImageLayerClip don't set aClipState->mHasRoundedCorners.

Comment 2

[Andrew McCreight [:mccr8]]

This also looks suspiciously related to bug 1338767, which is a crash in DisplayItemClip::GetCommonRoundedRectCount.

Comment 3

[u459114]

Let me check

Comment 4

[u459114]

Attachment: Bug 1341149 - Implement ImageLayerClipState::ctor and ImageLayerClipState::IsValid.

Comment 5

[u459114]

Attachment: Bug 1341149 - Implement ImageLayerClipState::ctor.

Comment 6

[u459114]

Attachment: Bug 1341149 - Implement ImageLayerClipState::ctor.

Comment 7

[u459114]

Since this is a sec bug, let's keep the solution as simple as possible. Implement default ctor of ImageLayerClipState should be enough to fix this bug.
I will do more fine tune in the follow up (bug 1342302)

Comment 8

[u459114]

Attachment: Bug 1341149 - Implement ImageLayerClipState::ctor.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/ea1dda72c26d

Comment 10

[Andrew McCreight [:mccr8]]

Please nominate this for uplift to Aurora when you get a chance, in case it fixes bug 1338767. (The only crashes on Nightly are from 2-22, so it is hard to draw conclusion one way or the other.)

Comment 11

[u459114]

Comment on attachment 8840770 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1311270
[User impact if declined]: We need this fix to identify whether the root cause of bug 1338767 is the same with this one. 
[Is this code covered by automated tests?]:no
[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]: NA
[Is the change risky?]: no.
[Why is the change risky/not risky?]: I implemented the default constructor of ImageLayerClipState to give default value of its data member, which should be very safe change.
[String changes made/needed]: NA

Comment 12

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

Comment on attachment 8840770 [details] [diff] [review]
Bug 1341149 - Implement ImageLayerClipState::ctor.

Diagnostic patch, let's take it for 53 so we can follow up on related issues.

Comment 13

[Carsten Book [:Tomcat]]

uplifted by ryan in https://hg.mozilla.org/releases/mozilla-aurora/rev/3df2a420389b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request ESR52 approval on this when you get a chance since we have strong evidence that this indeed fixes bug 1338767.

Comment 15

[Daniel Veditz [:dveditz]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #11)
> [Feature/Bug causing the regression]: bug 1311270

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> ESR52 approval [...] we have strong evidence that this indeed fixes bug 1338767.

How can both of these things be true? bug 1311270 was landed in Firefox 53 and almost all of the crashes for bug 1338767 were in versions before that.

Comment 16

[Markus Stange [:mstange]]

That's a very good point.

Comment 17

[u459114]

Since bug 1311270 was landed in 53 and the patch here fix regression of it, uplift it to 52 does not make sense to me