1088731 - undefined behavior in TableTicker

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Tom Tromey :tromey]

Running the test case from bug 1047124 under valgrind shows some
undefined behavior in TableTicker:

==11141== Conditional jump or move depends on uninitialised value(s)
==11141==    at 0xA4AE2F3: TableTicker::InplaceTick(TickSample*) (TableTicker.cpp:827)
==11141==    by 0xA4ADFA7: TableTicker::Tick(TickSample*) (TableTicker.cpp:743)
==11141==    by 0xA4AE4E4: TableTicker::GetBacktrace() (TableTicker.cpp:884)
==11141==    by 0xA4C6C40: mozilla_sampler_get_backtrace() (platform.cpp:1016)
==11141==    by 0x9E1E0E2: profiler_get_backtrace() (GeckoProfilerImpl.h:111)
==11141==    by 0x9E4C29F: nsRefreshDriver::AddLayoutFlushObserver(nsIPresShell*) (nsRefreshDriver.h:177)
==11141==    by 0x9E41C31: PresShell::ScheduleReflow() (nsPresShell.cpp:8778)
==11141==    by 0x9E41AF3: PresShell::MaybeScheduleReflow() (nsPresShell.cpp:8766)
==11141==    by 0x9E2E55F: PresShell::FrameNeedsReflow(nsIFrame*, nsIPresShell::IntrinsicDirty, nsFrameState) (nsPresShell.cpp:2919)
==11141==    by 0xA011ADE: nsBoxFrame::InsertFrames(mozilla::layout::FrameChildListID, nsIFrame*, nsFrameList&) (nsBoxFrame.cpp:1064)
==11141==    by 0x9DE703D: nsFrameManager::InsertFrames(nsContainerFrame*, mozilla::layout::FrameChildListID, nsIFrame*, nsFrameList&) (nsFrameManager.cpp:381)
==11141==    by 0x9D97F46: nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (nsCSSFrameConstructor.cpp:7650)


The problem here is that the code does:

  if (sample && sample->ussMemory != 0) {

but sample was initialized in GetBacktrace like:

  TickSample sample;

and the TickSample constructor doesn't initialize ussMemory.


I'll attach a patch that fixes this by making sure all the
members are initialized.

Comment 1

[Tom Tromey :tromey]

Attachment: initialize all members of TickSample

This patch initializes all the members of TickSample.
This avoids undefined behavior as noted in the bug.
I chose to explicitly initialize timestamp for completeness.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8511103 [details] [diff] [review]
initialize all members of TickSample

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

::: tools/profiler/platform.h
@@ +265,5 @@
>  #endif
>          context(NULL),
> +        isSamplingCurrentThread(false),
> +        threadProfile(nullptr),
> +        timestamp(),

This one isn't needed, it's done by default.

Comment 3

[Tom Tromey :tromey]

Attachment: initialize all members of TickSample

I went back and forth over whether to include that line.
Removed now.

Comment 4

[Carsten Book [:Tomcat]]

Hi Tom, could you provide a try link for this changes. Thanks!

Comment 5

[Tom Tromey :tromey]

https://tbpl.mozilla.org/?tree=Try&rev=ae9f2014587d

Comment 6

[Tom Tromey :tromey]

Sorry about that Carsten.
The try build is queued.

Comment 7

[Tom Tromey :tromey]

Trying again to clear the needinfo...

Comment 8

[Tom Tromey :tromey]

Haha, I misread the checkbox.  Silly me.

Comment 9

[Tom Tromey :tromey]

The results seem ok to me.  There are two oranges, one is a known
failure and one is a mercurial timeout which seems common (I've seen
it before).

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79eace8a9153

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79eace8a9153