Closed Bug 960603 Opened 11 years ago Closed 11 years ago

Uninitialised value use relating to nsJSContext::EndCycleCollectionCallback


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox29 --- fixed
firefox30 --- fixed


(Reporter: jseward, Assigned: jseward)


(Whiteboard: [qa-])


(1 file)

When running m-c either 32- or 64-bit linux, I get errors as follows

Conditional jump or move depends on uninitialised value(s)
   at 0x57AFE1B: dosprintf(SprintfStateStr*, char16_t const*, __va_list_tag*) (nsTextFormatter.cpp:294)
   by 0x57B1A48: nsTextFormatter::vsmprintf(char16_t const*, __va_list_tag*) (nsTextFormatter.cpp:1249)
   by 0x57B1AFC: nsTextFormatter::smprintf(char16_t const*, ...) (nsTextFormatter.cpp:1210)
   by 0x6558D0F: nsJSContext::EndCycleCollectionCallback(mozilla::CycleCollectorResults&) (nsJSEnvironment.cpp:2332)
   by 0x6486797: XPCJSRuntime::EndCycleCollectionCallback(mozilla::CycleCollectorResults&) (XPCJSRuntime.cpp:721)
   by 0x57CC75E: nsCycleCollector::CleanupAfterCollection() (nsCycleCollector.cpp:2955)
   by 0x57CDF80: nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) (nsCycleCollector.cpp:3027)
   by 0x57CE0C6: nsCycleCollector_collectSlice(long) (nsCycleCollector.cpp:3566)
   by 0x655790A: nsJSContext::RunCycleCollectorSlice(long) (nsJSEnvironment.cpp:2151)
   by 0x65584F6: CCTimerFired(nsITimer*, void*) (nsJSEnvironment.cpp:2449)
   by 0x580724A: nsTimerImpl::Fire() (nsTimerImpl.cpp:551)
   by 0x5807311: nsTimerEvent::Run() (nsTimerImpl.cpp:635)
 Uninitialised value was created by a stack allocation
   at 0x6558785: nsJSContext::EndCycleCollectionCallback(mozilla::CycleCollectorResults&) (nsJSEnvironment.cpp:2213)

I don't know if this remotely relevant, but this is a build with
"ac_add_options --enable-profiling" and started like this:

vTRUNK --track-origins=yes --smc-check=all-non-file --fair-sched=yes \
  ./ff-opt-linux/dist/bin/firefox-bin -P dev -no-remote

This clearly relates to the second call to nsTextFormatter::smprintf.
At first I thought some of the passed values might be uninitialised, so
I added this


etc (one for each passed value) just before the call, but none of those
produced any output, which suggests that they are all properly defined.

I was a bit mystified therefore, until I compared the 32-bit and 64-bit
error reports.  The 32-bit ones start like this

Conditional jump or move depends on uninitialised value(s)
   at 0x8BD11CF: dosprintf(SprintfStateStr*, char16_t const*, char*) (nsTextFormatter.cpp:252)

whereas the 64-bit ones are like this

Conditional jump or move depends on uninitialised value(s)
   at 0x57AFE1B: dosprintf(SprintfStateStr*, char16_t const*, __va_list_tag*) (nsTextFormatter.cpp:294)

that is, they are taking different paths in nsTextFormatter.cpp.  Line 252 is inside
cvt_l() whereas line 294 is inside cvt_ll().


One theory is that there is a format string mismatch for the call.  The various
args are passed in memory on the stack, possibly with some alignment holes, and because
the string doesn't match the args, in some cases the holes get picked up by dosprintf().

I notice nsTextFormatter.h doesn't mark smprintf() as __attribute__((printf)) or
whatever magic is needed to get printf-style format string checking.
I compared the format types and actual parameter types for the
offending call to nsTextFormatter::smprintf, with the following

 %llu  TimeStamp(uint64_t?)  endCCTime
 %llu  uint32_t  ccNowDuration
 %llu  uint32_t  gCCStats.mMaxSliceTime
 %llu  uint32_t  gCCStats.mTotalSliceTime
 %llu  uint32_t  gCCStats.mMaxGCDuration
 %llu  uint32_t  gCCStats.mMaxSkippableDuration
 %lu   uint32_t  gCCStats.mSuspected
 %lu   uint32_t  aResults.mVisitedRefCounted
 %lu   uint32_t  aResults.mVisitedGCed
 %lu   uint32_t  aResults.mFreedRefCounted
 %lu   uint32_t  aResults.mFreedGCed
 %lu   uint32_t  sCCollectedWaitingForGC
 %lu   uint32_t  sLikelyShortLivingObjectsNeedingGC
 %d    bool      aResults.mForcedGC
 %lu   uint32_t  sForgetSkippableBeforeCC
 %lu   uint32_t  minForgetSkippableTime / PR_USEC_PER_MSEC
 %lu   uint32_t  sMaxForgetSkippableTime / PR_USEC_PER_MSEC
 %lu   uint32_t  (sTotalForgetSkippableTime / cleanups) / PR_USEC_PER_MSEC
 %lu   uint32_t  sTotalForgetSkippableTime / PR_USEC_PER_MSEC
 %lu   uint32_t  sRemovedPurples

It seems to me the 2nd through 6th %llus should be %lu, to be
consistent with the rest of it.  IMO that still isn't really right:
%lu can be interpreted to mean "unsigned machine word", which matches
uint32_t on 32 bit targets but not on 64 bit targets.  Really I think
we should use plain %u for uint32_t.
Attached patch a fixSplinter Review
Per the comments above, I don't believe this is really right,
but it at least stops V complaining on both 32- and 64-bit x86.
I didn't check whether the output of the smprintf is still
as expected -- don't know how to do that.
Attachment #8361607 - Flags: review?(continuation)
Comment on attachment 8361607 [details] [diff] [review]
a fix

Attachment #8361607 - Flags: review?(continuation) → review+
Assignee: nobody → jseward
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8361607 [details] [diff] [review]
a fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): maybe bug 948554
User impact if declined: breaks some addons on 32-bit builds, like MemChaser
Testing completed (on m-c, etc.): it has been on m-c for a while
Risk to taking this patch (and alternatives if risky): very low, and it should only affect these addons
String or IDL/UUID changes made by this patch: none
Attachment #8361607 - Flags: approval-mozilla-aurora?
Attachment #8361607 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.