1074697 - nsDisplaymtdBorder::GetBounds does not set out parameter *aSnap

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Julian Seward [:jseward]]

This eventually leads to branches on uninitialised data, as shown in the
next comment.  Test case is layout/mathml/tests/test_bug330964.html.  I
inspected all other implementations of GetBounds(nsDisplayListBuilder*, bool*)
that I could find, and they all write to *aSnap.

Possibly related to bug 330964.

Comment 1

[Julian Seward [:jseward]]

Conditional jump or move depends on uninitialised value(s)
  at 0x81EA903: ScaleToOutsidePixels (layout/base/FrameLayerBuilder.cpp:651)
  by 0x81EA903: mozilla::ThebesLayerData::Accumulate(mozilla::ContainerState*, nsDisplayItem*, nsIntRegion const&, nsIntRect const&, nsIntRect const&, mozilla::DisplayItemClip const&) (layout/base/FrameLayerBuilder.cpp:2339)
  by 0x81F2466: mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) (layout/base/FrameLayerBuilder.cpp:3052)
  by 0x81F3AF8: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4 const*, unsigned int) (layout/base/FrameLayerBuilder.cpp:3999)
  by 0x8215680: nsDisplayOwnLayer::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (layout/base/nsDisplayList.cpp:3672)
  by 0x821B0D6: nsDisplaySubDocument::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) (layout/base/nsDisplayList.cpp:3714)
  by 0x81F1D2A: mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) (layout/base/FrameLayerBuilder.cpp:2892)
  by 0x81F3AF8: mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4 const*, unsigned int) (layout/base/FrameLayerBuilder.cpp:3999)
  by 0x824E640: nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) (layout/base/nsDisplayList.cpp:1317)
  by 0x824F19A: nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) (layout/base/nsDisplayList.cpp:1235)
  by 0x824FEB9: nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (layout/base/nsLayoutUtils.cpp:3064)
  by 0x81C9290: PresShell::Paint(nsView*, nsRegion const&, unsigned int) (layout/base/nsPresShell.cpp:6231)
  by 0x7D280DB: nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) (view/nsViewManager.cpp:443)
  by 0x7D284E1: nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) (view/nsViewManager.cpp:384)
  by 0x7D28651: nsViewManager::ProcessPendingUpdates() (view/nsViewManager.cpp:1075)
  by 0x81D48E2: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (layout/base/nsRefreshDriver.cpp:1341)
  by 0x81D650D: TickDriver (layout/base/nsRefreshDriver.cpp:173)
  by 0x81D650D: Tick (layout/base/nsRefreshDriver.cpp:164)
  by 0x81D650D: mozilla::RefreshDriverTimer::TimerTick(nsITimer*, void*) (layout/base/nsRefreshDriver.cpp:190)

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1074697-1.diff

The obvious fix -- set *aSnap to false.  Note that I have no
idea whether 'false' is the correct value here.  It does stop
Valgrind complaining, though.

Comment 3

[James Kitchener (:jkitch)]

Stupid, stupid me.

The parent class uses true, but false looks to be acceptable if in doubt.  Do not take my word for it however.

https://hg.mozilla.org/mozilla-central/annotate/7c24470b6b3a/layout/base/nsDisplayList.h#l921

Comment 4

[Julian Seward [:jseward]]

(In reply to James Kitchener from comment #3)
> The parent class uses true, but false looks to be acceptable if in doubt. 
> Do not take my word for it however.

Who would be able to tell us for sure?

Comment 5

[James Kitchener (:jkitch)]

My first guess would be Robert O'Callahan ([e-mail])

Comment 6

[Julian Seward [:jseward]]

Robert, can we get your opinion on this?  In short:
nsDisplaymtdBorder::GetBounds needs to set its out-param *aSnap to
something, but it is unclear whether it should be to false or true.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8497368 [details] [diff] [review]
bug1074697-1.diff

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

False is always safe.

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +274,5 @@
>    }
>  
>    virtual nsRect GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap) MOZ_OVERRIDE
>    {
> +    *aSnap = false;

Make it true; nsCSSRendering::PaintBorderWithStyleBorder snaps.

Comment 8

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85ad6786d38

Comment 9

[Carsten Book [:Tomcat]]

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