795631 - Valgrind on tbpl detects "Conditional jump or move depends on uninitialised value(s)" with mozilla::FrameLayerBuilder::ClippedDisplayItem::~ClippedDisplayItem on the stack

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Attachment: m-c changeset c09a0c022b2e Valgrind stack with line numbers

Valgrind detects "Conditional jump or move depends on uninitialised value(s)" with mozilla::FrameLayerBuilder::ClippedDisplayItem::~ClippedDisplayItem on the stack on m-c changeset c09a0c022b2e, see attached snippet which comes from:

https://tbpl.mozilla.org/php/getParsedLog.php?id=15660507&tree=Firefox&full=1

Bug 539356 may likely be the cause, because it did not occur on m-c changeset 895f66c4eada at:

https://tbpl.mozilla.org/php/getParsedLog.php?id=15623197&tree=Firefox&full=1

s-s until otherwise shown, some conditional jump errors can be bad.

Comment 1

[Julian Seward [:jseward]]

I also see this, on every startup of Firefox, on x86_64-linux.  It
wasn't happening a week ago, so maybe as Gary says, due to recent
landings of bug 539356.

Here it is with the origin of uninitialised value(s) shown.

Conditional jump or move depends on uninitialised value(s)
   at 0x6299EA9: mozilla::FrameLayerBuilder::ClippedDisplayItem::~ClippedDisplayItem() (layout/base/FrameLayerBuilder.cpp:2436)
   by 0x629A563: nsTHashtable<mozilla::FrameLayerBuilder::ThebesLayerItemsEntry>::s_ClearEntry(PLDHashTable*, PLDHashEntryHdr*) (ff-opt/layout/base/../../dist/include/nsTArray.h:360)
   by 0x6F5CED9: PL_DHashTableFinish (ff-opt/xpcom/build/pldhash.cpp:361)
   by 0x629965F: mozilla::FrameLayerBuilder::~FrameLayerBuilder() (ff-opt/layout/base/../../dist/include/nsTHashtable.h:385)
   by 0x62D888B: nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const (ff-opt/layout/base/../../dist/include/nsAutoPtr.h:71)
   by 0x62D8C12: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const (layout/base/nsDisplayList.cpp:966)
   by 0x62F8919: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (layout/base/nsLayoutUtils.cpp:1743)
   by 0x63118C4: PresShell::Paint(nsIView*, nsRegion const&, nsIPresShell::PaintType, bool) (layout/base/nsPresShell.cpp:5323)
   by 0x66FE679: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (view/src/nsViewManager.cpp:436)
   by 0x66FE7AD: nsViewManager::ProcessPendingUpdates() (view/src/nsViewManager.cpp:1210)
   by 0x6319170: nsRefreshDriver::Notify(nsITimer*) (layout/base/nsRefreshDriver.cpp:431)
   by 0x6F96A9D: nsTimerImpl::Fire() (xpcom/threads/nsTimerImpl.cpp:476)

 Uninitialised value was created by a heap allocation
   at 0x402ADDC: malloc (/home/sewardj/Vg38BRANCH/branch38/coregrind/m_replacemalloc/vg_replace_malloc.c:270)
   by 0x403EFEB: moz_xmalloc (memory/mozalloc/mozalloc.cpp:57)
   by 0x629BB2A: mozilla::FrameLayerBuilder::AddThebesDisplayItem(mozilla::layers::ThebesLayer*, nsDisplayItem*, mozilla::FrameLayerBuilder::Clip const&, nsIFrame*, mozilla::LayerState, nsPoint const&) (ff-opt/layout/base/../../dist/include/mozilla/mozalloc.h:200)
   by 0x629F773: mozilla::(anonymous namespace)::ContainerState::ProcessDisplayItems(nsDisplayList const&, mozilla::FrameLayerBuilder::Clip&, unsigned int) (layout/base/FrameLayerBuilder.cpp:2202)
   by 0x629F445: mozilla::(anonymous namespace)::ContainerState::ProcessDisplayItems(nsDisplayList const&, mozilla::FrameLayerBuilder::Clip&, unsigned int) (layout/base/FrameLayerBuilder.cpp:2063)
   by 0x62A215E: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList const&, mozilla::FrameLayerBuilder::ContainerParameters const&, gfx3DMatrix const*) (layout/base/FrameLayerBuilder.cpp:2868)
   by 0x62D8246: nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const (layout/base/nsDisplayList.cpp:1044)
   by 0x62D8C12: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const (layout/base/nsDisplayList.cpp:966)
   by 0x62F8919: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (layout/base/nsLayoutUtils.cpp:1743)
   by 0x63118C4: PresShell::Paint(nsIView*, nsRegion const&, nsIPresShell::PaintType, bool) (layout/base/nsPresShell.cpp:5323)
   by 0x66FE679: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (view/src/nsViewManager.cpp:436)
   by 0x66FE7AD: nsViewManager::ProcessPendingUpdates() (view/src/nsViewManager.cpp:1210)

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Initialize mPhase variable in opt mode

This was in the original patch (part 2) of bug 539356 - Must have accidentally lost it during rebasing.

I don't think this is really a security bug, calling EndTransaction() every time should be harmless.

Comment 3

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

Opening up as per comment 2.

Comment 4

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/mozilla-central/rev/87346ce19cfc

Comment 5

[Cornel Ionce [:noni] [Hubs QA]]

Is there any way a QA can test this?

Comment 6

[Matt Woodrow (:mattwoodrow)]

I don't think so, without setting up a valgrind install. I don't think we need explicit QA here.

Comment 7

[Cornel Ionce [:noni] [Hubs QA]]

Thanks for the reply