Closed Bug 949310 Opened 6 years ago Closed 6 years ago

Undefined value error in nsDisplayOwnLayer::BuildLayer()

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- unaffected
firefox28 --- fixed
firefox29 --- fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed

People

(Reporter: njn, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

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==
CC'ing some folks who might know what's going on. (if not, please continue to cc!)
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
But as an educated guess, I'd suggest bug 814435 is at fault.
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.
Where is mIsScrollbar initialized?
Blocks: 814435
(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.
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.
Assignee: nobody → bugmail.mozilla
Attachment #8346491 - Flags: review?(tnikkel)
Are there any security implications you can think of for those variables being used uninitialized?
> 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.
How about this?
Attachment #8346491 - Attachment is obsolete: true
Attachment #8346507 - Attachment is obsolete: true
Attachment #8346491 - Flags: review?(tnikkel)
Attachment #8346612 - Flags: review?(bgirard)
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 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?
Attachment #8346612 - Flags: review?(bgirard) → review+
Yeah that does seem like a better solution, thanks! I was too blinded from copying the mStickyPositionData pattern.
Attachment #8346612 - Attachment is obsolete: true
Attachment #8346666 - Flags: review?(bgirard)
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.
Attachment #8346666 - Flags: review?(bgirard) → review+
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
So is this not really a security issue?  Can I unhide the bug?
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
Thanks.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/0487c6a2c712
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
blocking-b2g: --- → 1.3?
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
Attachment #8346666 - Flags: approval-mozilla-aurora?
blocking-b2g: 1.3? → ---
Attachment #8346666 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.