Closed Bug 1088731 Opened 8 years ago Closed 8 years ago

undefined behavior in TableTicker

Categories

(Core :: Gecko Profiler, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: tromey, Assigned: tromey)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Attachment #8511103 - Flags: review?(bgirard)
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.
Attachment #8511103 - Flags: review?(bgirard) → review+
I went back and forth over whether to include that line.
Removed now.
Attachment #8511103 - Attachment is obsolete: true
Keywords: checkin-needed
Hi Tom, could you provide a try link for this changes. Thanks!
Assignee: nobody → ttromey
Flags: needinfo?(ttromey)
Keywords: checkin-needed
Sorry about that Carsten.
The try build is queued.
Trying again to clear the needinfo...
Haha, I misread the checkbox.  Silly me.
Flags: needinfo?(ttromey)
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).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/79eace8a9153
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.