Last Comment Bug 792065 - Uninitialised value use in nsIdleServiceDaily::DailyCallback
: Uninitialised value use in nsIdleServiceDaily::DailyCallback
Status: RESOLVED FIXED
[qa-]
: valgrind
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla18
Assigned To: Gian-Carlo Pascutto [:gcp]
:
Mentors:
Depends on:
Blocks: 792037
  Show dependency treegraph
 
Reported: 2012-09-18 08:36 PDT by Julian Seward [:jseward]
Modified: 2012-10-16 14:54 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed


Attachments
Patch 1. Make sure the timer start time is initialized (1.09 KB, patch)
2012-09-19 13:36 PDT, Gian-Carlo Pascutto [:gcp]
roc: review+
jseward: feedback+
gavin.sharp: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Julian Seward [:jseward] 2012-09-18 08:36:16 PDT
Occurs on all startups of trunk.  From my very hazy memory, has been
going on for at least a month.  I wonder if this is somehow related to
bug 762620.

Conditional jump or move depends on uninitialised value(s)
   at 0x6D996AD: nsIdleServiceDaily::DailyCallback(nsITimer*, void*)
                 (widget/xpwidgets/nsIdleService.cpp:201)
   by 0x6D99AC4: nsIdleServiceDaily::Init()
                 (widget/xpwidgets/nsIdleService.cpp:153)
   by 0x6D7D9B8: nsIdleServiceGTK::nsIdleServiceGTK()
                 (widget/gtk2/nsIdleServiceGTK.cpp:69)
   by 0x6D72028: nsIdleServiceGTKConstructor(nsISupports*, nsID const&,
                 void**) (widget/gtk2/nsIdleServiceGTK.h:37)
   by 0x6FE915C: nsComponentManagerImpl::CreateInstance(nsID const&,
                 nsISupports*, nsID const&, void**)
                 (xpcom/components/nsComponentManager.cpp:944)
   by 0x6FEB798: nsComponentManagerImpl::GetService(nsID const&, nsID const&,
                 void**) (xpcom/components/nsComponentManager.cpp:1237)
   by 0x6A6D8F9: nsJSCID::GetService(JS::Value const&, JSContext*, unsigned
                 char, JS::Value*) (js/xpconnect/src/XPCJSID.cpp:777)
   by 0x700174F: NS_InvokeByIndex_P 
                 (xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164)
   by 0x6A8937A: XPCWrappedNative::CallMethod(XPCCallContext&, 
                 XPCWrappedNative::CallMode)
                 (js/xpconnect/src/XPCWrappedNative.cpp:3105)
   by 0x6A9026F: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)
                 (js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1469)
   by 0x7542428: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct)
                 (js/src/jscntxtinlines.h:372)
   by 0x753C200: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode)
                 (js/src/jsinterp.cpp:2454)

 Uninitialised value was created by a heap allocation
   at 0x402ADDC: malloc (coregrind/m_replacemalloc/vg_replace_malloc.c:270)
   by 0x403EEEB: moz_xmalloc (memory/mozalloc/mozalloc.cpp:57)
   by 0x6D99BF1: nsIdleService::nsIdleService()
                 (ff-opt/widget/xpwidgets/../../dist/include/mozilla/mozalloc.h:200)
   by 0x6D7D9B8: nsIdleServiceGTK::nsIdleServiceGTK()
                 (widget/gtk2/nsIdleServiceGTK.cpp:69)
   by 0x6D72028: nsIdleServiceGTKConstructor(nsISupports*, nsID const&, void**)
                 (widget/gtk2/nsIdleServiceGTK.h:37)
   by 0x6FE915C: nsComponentManagerImpl::CreateInstance(nsID const&,
                 nsISupports*, nsID const&, void**)
                 (xpcom/components/nsComponentManager.cpp:944)
   by 0x6FEB798: nsComponentManagerImpl::GetService(nsID const&, nsID const&,
                 void**) (xpcom/components/nsComponentManager.cpp:1237)
   by 0x6A6D8F9: nsJSCID::GetService(JS::Value const&, JSContext*, unsigned
                 char, JS::Value*) (js/xpconnect/src/XPCJSID.cpp:777)
   by 0x700174F: NS_InvokeByIndex_P
                 (xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164)
   by 0x6A8937A: XPCWrappedNative::CallMethod(XPCCallContext&,
                 XPCWrappedNative::CallMode)
                 (js/xpconnect/src/XPCWrappedNative.cpp:3105)
   by 0x6A9026F: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)
                 (js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1469)
   by 0x7542428: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct)
                 (js/src/jscntxtinlines.h:372)
Comment 1 Gian-Carlo Pascutto [:gcp] 2012-09-19 00:06:09 PDT
DailyCallback can be called without mDailyTimerStart being initialized, in the case where we force it to fire immediately because our prefs-saved last idle time was more than a day.
Comment 2 Gian-Carlo Pascutto [:gcp] 2012-09-19 00:16:46 PDT
In that case, we might or might not (depending on the random value in memory) allow the idle-daily call to proceed. If we don't, we might set the timer for it to a random large value which will cause our idle-dailies to no longer fire.

Because we will run the "fire immediately" case each time on startup, this looks fairly serious to me in that it would potentially kill idle-daily updates forever. I guess we're generally lucky with RAM here of we'd have noticed an ADU drop in aurora/beta/...

The fix would be to set mDailyTimerStart also in that path, logically to the PREF_LAST_DAILY value adjusted for s/ms/us scale differences.
Comment 3 Gian-Carlo Pascutto [:gcp] 2012-09-19 13:36:07 PDT
Created attachment 662674 [details] [diff] [review]
Patch 1. Make sure the timer start time is initialized

If we're initializing, and we aren't going to set a real timer but instead call the callback immediately (because the last idle-daily run has been more than a day ago), still set the timer start to what it originally would have been. This prevents the code that stops misfiring timers from misfiring (irony much).
Comment 4 Julian Seward [:jseward] 2012-09-19 15:15:50 PDT
Comment on attachment 662674 [details] [diff] [review]
Patch 1. Make sure the timer start time is initialized

This appears to stop valgrind complaining, so, .. LGTM.
Comment 5 Alex Keybl [:akeybl] 2012-09-19 16:59:31 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #2)
> Because we will run the "fire immediately" case each time on startup, this
> looks fairly serious to me in that it would potentially kill idle-daily
> updates forever. I guess we're generally lucky with RAM here of we'd have
> noticed an ADU drop in aurora/beta/...

We have questions.

1) Is bug 762620 definitely the regressing bug? If so, this is a FF15 regression.
2) Is this limited to telemetry? https://bugzilla.mozilla.org/show_bug.cgi?id=762620#c12 suggests that AUS/blocklist are not on the daily timer.
Comment 6 Gian-Carlo Pascutto [:gcp] 2012-09-20 02:35:08 PDT
1) Yes, bug 762620 causes it.
2) Seems to be: telemetry, places categories and form history expiration, sqlite vacuuming

If we didn't notice a drop in Telemetry submissions from Firefox 15, I guess we're generally "lucky" in the field.
Comment 7 Gian-Carlo Pascutto [:gcp] 2012-09-20 02:39:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f69ef21a70
Comment 8 Alex Keybl [:akeybl] 2012-09-20 10:15:02 PDT
Please nominate for uplift with a risk evaluation this week. Thanks!
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-09-20 18:35:00 PDT
https://hg.mozilla.org/mozilla-central/rev/22f69ef21a70
Comment 10 Gian-Carlo Pascutto [:gcp] 2012-09-24 08:03:48 PDT
Comment on attachment 662674 [details] [diff] [review]
Patch 1. Make sure the timer start time is initialized

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 762620
User impact if declined: Randomly not working telemetry, places categories and form history expiration, sqlite vacuuming 
Testing completed (on m-c, etc.): landed on m-i a few days ago
Risk to taking this patch (and alternatives if risky): Entirely breaking above services. More people looked at the code now, and we'd notice on Nightly if things are completely broken, so the risk is small.
Comment 11 Alex Keybl [:akeybl] 2012-09-24 12:38:44 PDT
Comment on attachment 662674 [details] [diff] [review]
Patch 1. Make sure the timer start time is initialized

[Triage Comment]
Approving for Beta 16 -  we need confidence in our metrics data, we've got 2 more betas, and this doesn't affect our most critical timer for AUS (risk is manageable).

gcp - I'm putting you down for checking for inconsistencies in the Aurora data, and the Beta telemetry data after FF16 beta 5 ships this week :). We'll want to quickly backout for release if we see any regressions.
Comment 12 Gian-Carlo Pascutto [:gcp] 2012-09-24 14:05:32 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/d54dcbfd09ee
Comment 13 Gian-Carlo Pascutto [:gcp] 2012-09-24 14:09:50 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/54efd2cc7c85

>gcp - I'm putting you down for checking for inconsistencies in the Aurora data, 
>and the Beta telemetry data after FF16 beta 5 ships this week :)

Uh, OK. I'll keep an eye out for Telemetry people with pitchforks near my office, too.
Comment 14 Gian-Carlo Pascutto [:gcp] 2012-10-01 07:11:36 PDT
>We'll want to quickly backout for release if we see any regressions.

There's a 40-60% increase in Telemetry submissions after this fix landed. Pretty sad we didn't notice the initial regression...

Note You need to log in before you can comment on or make changes to this bug.