1271931 - Conditional jump or move depends on uninitialised value(s) mozilla::PaintedLayerData::Accumulate (FrameLayerBuilder.cpp:3320)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

While running valgrind over some mochitests I found this warning:

Conditional jump or move depends on uninitialised value(s)
   at 0x12ECD6CE: mozilla::PaintedLayerData::Accumulate(mozilla::ContainerState*, nsDisplayItem*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::DisplayItemClip const&, mozilla::LayerState) (FrameLayerBuilder.cpp:3320)
   by 0x12ED1465: mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) (FrameLayerBuilder.cpp:4113)
   by 0x12ED79D9: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> const*, unsigned int) (FrameLayerBuilder.cpp:5177)
   by 0x12F57FEA: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) (nsDisplayList.cpp:1712)
   by 0x12F91701: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) (nsLayoutUtils.cpp:3572)
   by 0x12FC0404: PresShell::Paint(nsView*, nsRegion const&, unsigned int) (nsPresShell.cpp:6366)
   by 0x12B7BC5E: nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) (nsViewManager.cpp:467)
   by 0x12B7B925: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (nsViewManager.cpp:398)
   by 0x12EABD80: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:1900)
   by 0x12EB1BA3: mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) (nsRefreshDriver.cpp:246)
   by 0x12EB1A34: mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:265)
   by 0x12EB1D7F: applyImpl<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), StoreCopyPassByValue<mozilla::TimeStamp> , 0> (nsThreadUtils.h:675)
   by 0x12EB1D7F: apply<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)> (nsThreadUtils.h:681)
   by 0x12EB1D7F: nsRunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, mozilla::TimeStamp>::Run() (nsThreadUtils.h:709)

Probably caused by unclean backout. Bug 1195004 seems related.

Comment 1

[Timothy Nikkel (:tnikkel)]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #0)
> Conditional jump or move depends on uninitialised value(s)
>    at 0x12ECD6CE:
> mozilla::PaintedLayerData::Accumulate(mozilla::ContainerState*,
> nsDisplayItem*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>
> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&,
> mozilla::DisplayItemClip const&, mozilla::LayerState)
> (FrameLayerBuilder.cpp:3320)

FrameLayerBuilder.cpp:3320 is a blank line here http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#3320 so I don't know what value might be un-initailized.

> Probably caused by unclean backout. Bug 1195004 seems related.

Unclean backout? Of what? Can you elaborate?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

https://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=d6eac6cc4588#3409

At first I thought |uniformColor| was used uninitialized but look like all IsUniform writes to it when |isUniform| is true. Probably a false alarm; I'll try again today.

I think it's still a good idea to initialize uniformColor to a default value.

Comment 3

[Timothy Nikkel (:tnikkel)]

We've definitely had a bug on a warning about the uniform color not getting initialized before (I don't have the bug number handy). That bug concluded it was an incorrect warning.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

So it's gcc figured out that it's safe to optimized out the |!isUniform| check and always compare uniformColor since uniformColor is never used later if isUniform is false. Which sometimes checks against uninitialized value and triggered a valgrind false alarm.

Relevant disassembly:
   0x00007fbadb0bc7d4 : callq  *0x68(%rax)
   0x00007fbadb0bc7d7 : mov    %al,%bl                  /* save isUniform to BL */
=> 0x00007fbadb0bc7d9 : cmpl   $0xffffff,-0xa0(%rbp)    /* compare if NS_GET_A(uniformColor) > 0 */
   0x00007fbadb0bc7e3 : ja     0x7fbadb0bc7ef
   0x00007fbadb0bc7e5 : mov    %bl,%al                  /* if NS_GET_A(uniformColor) < 0 */
   0x00007fbadb0bc7e7 : xor    $0x1,%al                 /* check if isUniform is true */
   0x00007fbadb0bc7e9 : je     0x7fbadb0bc9a0           /* skip the entire if block */

:tnikkel, I'll let you decide if you want to fix this warning.

Comment 5

[Timothy Nikkel (:tnikkel)]

Fix it if the false positives are causing problems or getting in the way of fixing real bugs.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: MozReview Request: Bug 1271931 - Make DisplayItem::IsUniform to return Maybe<nscolor>. r?tnikkel

Maybe<nscolor> ensures that correct value is returned and also makes the
error checking clearer than using a out param.

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

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8754254 [details]
MozReview Request: Bug 1271931 - Make DisplayItem::IsUniform to return Maybe<nscolor>. r?tnikkel

https://reviewboard.mozilla.org/r/53868/#review50598

Good use of Maybe.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34eda1b91ca3

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34eda1b91ca3