949310 - Undefined value error in nsDisplayOwnLayer::BuildLayer()

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicholas Nethercote [inactive]]

Two very similar warnings from Valgrind.  They were introduced in the last two days or so.  I don't have a regression range, sorry.

> ==1342== Conditional jump or move depends on uninitialised value(s)
> ==1342==    at 0x92D4662: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (Layers.h:981)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92D45F5: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (nsDisplayList.cpp:3282)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92EB743: nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const (nsDisplayList.cpp:1146)
> ==1342==    by 0x92EC297: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const (nsDisplayList.cpp:1066)
> ==1342==    by 0x92F23D0: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (nsLayoutUtils.cpp:2321)
> ==1342==    by 0x92769B1: PresShell::Paint(nsView*, nsRegion const&, unsigned int) (nsPresShell.cpp:5845)
> ==1342==    by 0x8DB87C4: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (nsViewManager.cpp:421)
> ==1342==    by 0x8DB88A0: nsViewManager::ProcessPendingUpdates() (nsViewManager.cpp:1053)
> ==1342==    by 0x927F458: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:1208)
> ==1342==    by 0x92802AD: mozilla::RefreshDriverTimer::TimerTick(nsITimer*, void*) (nsRefreshDriver.cpp:168)
> ==1342==    by 0x818D8CE: nsTimerImpl::Fire() (nsTimerImpl.cpp:551)
> ==1342==    by 0x818D999: nsTimerEvent::Run() (nsTimerImpl.cpp:635)
> ==1342==    by 0x818A3E1: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:634)
> ==1342==  Uninitialised value was created by a heap allocation
> ==1342==    at 0x4C28A49: malloc (vg_replace_malloc.c:270)
> ==1342==    by 0x7614457: moz_xmalloc (mozalloc.cpp:52)
> ==1342==    by 0x860B875: mozilla::layers::BasicLayerManager::CreateContainerLayer() (mozalloc.h:201)
> ==1342==    by 0x92ACC7E: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:3003)
> ==1342==    by 0x92D45F5: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (nsDisplayList.cpp:3282)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92D45F5: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (nsDisplayList.cpp:3282)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92EB743: nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const (nsDisplayList.cpp:1146)
> ==1342==    by 0x92EC297: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const (nsDisplayList.cpp:1066)
> ==1342==    by 0x92F23D0: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (nsLayoutUtils.cpp:2321)
> ==1342==    by 0x92769B1: PresShell::Paint(nsView*, nsRegion const&, unsigned int) (nsPresShell.cpp:5845)
> ==1342==    by 0x8DB87C4: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (nsViewManager.cpp:421)
> ==1342==    by 0x8DB88A0: nsViewManager::ProcessPendingUpdates() (nsViewManager.cpp:1053)
> ==1342==    by 0x927F458: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:1208)
> ==1342==
> ==1342== Conditional jump or move depends on uninitialised value(s)
> ==1342==    at 0x92D46A3: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (Layers.h:981)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92D45F5: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (nsDisplayList.cpp:3282)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92EB743: nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const (nsDisplayList.cpp:1146)
> ==1342==    by 0x92EC297: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const (nsDisplayList.cpp:1066)
> ==1342==    by 0x92F23D0: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (nsLayoutUtils.cpp:2321)
> ==1342==    by 0x92769B1: PresShell::Paint(nsView*, nsRegion const&, unsigned int) (nsPresShell.cpp:5845)
> ==1342==    by 0x8DB87C4: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (nsViewManager.cpp:421)
> ==1342==    by 0x8DB88A0: nsViewManager::ProcessPendingUpdates() (nsViewManager.cpp:1053)
> ==1342==    by 0x927F458: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:1208)
> ==1342==    by 0x92802AD: mozilla::RefreshDriverTimer::TimerTick(nsITimer*, void*) (nsRefreshDriver.cpp:168)
> ==1342==    by 0x818D8CE: nsTimerImpl::Fire() (nsTimerImpl.cpp:551)
> ==1342==    by 0x818D999: nsTimerEvent::Run() (nsTimerImpl.cpp:635)
> ==1342==    by 0x818A3E1: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:634)
> ==1342==  Uninitialised value was created by a heap allocation
> ==1342==    at 0x4C28A49: malloc (vg_replace_malloc.c:270)
> ==1342==    by 0x7614457: moz_xmalloc (mozalloc.cpp:52)
> ==1342==    by 0x860B875: mozilla::layers::BasicLayerManager::CreateContainerLayer() (mozalloc.h:201)
> ==1342==    by 0x92ACC7E: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:3003)
> ==1342==    by 0x92D45F5: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (nsDisplayList.cpp:3282)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92D45F5: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (nsDisplayList.cpp:3282)
> ==1342==    by 0x92AD8EB: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::ContainerLayerParameters const&, gfx3DMatrix const*, unsigned int) (FrameLayerBuilder.cpp:2268)
> ==1342==    by 0x92EB743: nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const (nsDisplayList.cpp:1146)
> ==1342==    by 0x92EC297: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const (nsDisplayList.cpp:1066)
> ==1342==    by 0x92F23D0: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (nsLayoutUtils.cpp:2321)
> ==1342==    by 0x92769B1: PresShell::Paint(nsView*, nsRegion const&, unsigned int) (nsPresShell.cpp:5845)
> ==1342==    by 0x8DB87C4: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (nsViewManager.cpp:421)
> ==1342==    by 0x8DB88A0: nsViewManager::ProcessPendingUpdates() (nsViewManager.cpp:1053)
> ==1342==    by 0x927F458: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:1208)
> ==1342==

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

CC'ing some folks who might know what's going on. (if not, please continue to cc!)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nick, it should be possible to construct a regression range using the changeset hashes of those 2 days:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=<first day hash>&tochange=<last day hash>

e.g.:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=547d6f20ecb2&tochange=4c431a919737

Comment 3

[Nicholas Nethercote [inactive]]

But as an educated guess, I'd suggest bug 814435 is at fault.

Comment 4

[Nicholas Nethercote [inactive]]

Yeah, I had some changesets written down but putting them into the log thingy gave an empty range.  I wasn't confident that the range endpoints were correct, anyway, because of all the other crap (hangs and crashes) that have been occurring in the Valgrind-on-TBPL runs.

So let's start with bug 814435.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Where is mIsScrollbar initialized?

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Where is mIsScrollbar initialized?

mIsScrollbar is initialized in the Layer constructor, but wasn't in the patch in the bug. The other two fields added by that patch do not look to be initialized though.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Oops, my bad. I did actually leave the other two uninitialized on purpose because they should only never be used when mIsScrollbar is false, and they are always initialized when mIsScrollbar is set to true. However there is a bug at Layers.h:981, where the mIsScrollbar check should be a !mIsScrollbar check instead, and that is causing the uninitialized value access reported by Valgrind. I'll fix that; should I also initialize the other two variables or can I leave that out? In this case leaving them uninitialized uncovered a logic error which I probably wouldn't have noticed otherwise.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix logic error allowing uninitialized value access

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: (optional) also initialize the variables on construction

Comment 10

[Andrew McCreight [:mccr8]]

Are there any security implications you can think of for those variables being used uninitialized?

Comment 11

[Nicholas Nethercote [inactive]]

> should I also initialize the other two variables
> or can I leave that out?

At the least I would document the relationship and circumstances in which the members are uninitialized and why it's safe.  And if you can some code (getter functions with assertions, maybe?) to catch any future mistakes, even better.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix logic error and document uninitialized state

How about this?

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8346612 [details] [diff] [review]
Fix logic error and document uninitialized state

Review of attachment 8346612 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely!

::: gfx/layers/Layers.h
@@ +1014,5 @@
>    const LayerRect& GetStickyScrollRangeOuter() { return mStickyPositionData->mOuter; }
>    const LayerRect& GetStickyScrollRangeInner() { return mStickyPositionData->mInner; }
>    bool GetIsScrollbar() { return mIsScrollbar; }
> +  FrameMetrics::ViewID GetScrollbarTargetContainerId() {
> +    // mScrollbarTargetId may be uninitialized unless mIsScrollbar is true

Nit: trailing period?

@@ +1021,5 @@
> +  }
> +  ScrollDirection GetScrollbarDirection() {
> +    // mScrollbarDirection may be uninitialized unless mIsScrollbar is true
> +    MOZ_ASSERT(mIsScrollbar);
> +    return mScrollbarDirection;

Ditto.

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 8346612 [details] [diff] [review]
Fix logic error and document uninitialized state

Review of attachment 8346612 [details] [diff] [review]:
-----------------------------------------------------------------

This change is fine, but let me convince you of:

::: gfx/layers/Layers.h
@@ +1401,1 @@
>    bool mIsScrollbar;

IMO this is needlessly complicated. Let's remove mIsScrollBar, initialize the viewid to be zero and introduce and initialize msCrollbarDirection to none. What do you think?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix logic error and get rid of mIsScrollbar

Yeah that does seem like a better solution, thanks! I was too blinded from copying the mStickyPositionData pattern.

Comment 16

[Benoit Girard (:BenWa)]

Comment on attachment 8346666 [details] [diff] [review]
Fix logic error and get rid of mIsScrollbar

Review of attachment 8346666 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +1268,5 @@
>    }
>    if (GetContentFlags() & CONTENT_COMPONENT_ALPHA) {
>      aTo += " [componentAlpha]";
>    }
> +  switch (GetScrollbarDirection()) {

I'm not sure why you used a switch statement here. IMO I prefers ifs. If you want to use it please add a case for no scroll that does nothing. Some flow analysis tools check this.

::: gfx/layers/Layers.h
@@ -1012,5 @@
>    const LayerMargin& GetFixedPositionMargins() { return mMargins; }
>    FrameMetrics::ViewID GetStickyScrollContainerId() { return mStickyPositionData->mScrollId; }
>    const LayerRect& GetStickyScrollRangeOuter() { return mStickyPositionData->mOuter; }
>    const LayerRect& GetStickyScrollRangeInner() { return mStickyPositionData->mInner; }
> -  bool GetIsScrollbar() { return mIsScrollbar; }

You could of kept this as != NONE but this is fine too.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

I usually default to switch statements for enums but I don't care either way. Replaced the switch with ifs and landed:

https://hg.mozilla.org/integration/b2g-inbound/rev/0487c6a2c712

Comment 18

[Andrew McCreight [:mccr8]]

So is this not really a security issue?  Can I unhide the bug?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Yeah, I think so. I don't think the uninitialized value usage would allow arbitrary code execution or anything like that. At worst it would trigger incorrect but safe code paths.

Comment 20

[Andrew McCreight [:mccr8]]

Thanks.

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/0487c6a2c712

Comment 22

[Kartikaya Gupta (email:[e-mail])]

This fixes a regression in bug 814435 and should be uplifted to Fx28 and B2G1.3. Requesting blocking status on 1.3 so RyanVM's can uplift it.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8346666 [details] [diff] [review]
Fix logic error and get rid of mIsScrollbar

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 814435
User impact if declined: Scrollbars might not animate properly
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): pretty low risk, it's baked on m-c for almost a week now
String or IDL/UUID changes made by this patch: none

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/509350da3b66