The default bug view has changed. See this FAQ.

Uninitialised value use in nsIdleServiceDaily::DailyCallback

RESOLVED FIXED in Firefox 16

Status

()

Core
General
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jseward, Assigned: gcp)

Tracking

({valgrind})

Trunk
mozilla18
x86_64
Linux
valgrind
Points:
---

Firefox Tracking Flags

(firefox15 wontfix, firefox16+ fixed, firefox17+ fixed, firefox18+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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)
(Assignee)

Updated

5 years ago
Assignee: nobody → gpascutto
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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.
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
(Assignee)

Updated

5 years ago
Severity: normal → critical
(Assignee)

Comment 3

5 years ago
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).
Attachment #662674 - Flags: review?(roc)
Attachment #662674 - Flags: feedback?(jseward)
(Reporter)

Comment 4

5 years ago
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.
Attachment #662674 - Flags: feedback?(jseward) → feedback+
Attachment #662674 - Flags: review?(roc) → review+

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f69ef21a70

Comment 8

5 years ago
Please nominate for uplift with a risk evaluation this week. Thanks!
status-firefox15: --- → wontfix
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox16: ? → +
tracking-firefox17: ? → +
tracking-firefox18: ? → +

Updated

5 years ago
Blocks: 792037
https://hg.mozilla.org/mozilla-central/rev/22f69ef21a70
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox18: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 10

5 years ago
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.
Attachment #662674 - Flags: approval-mozilla-beta?
Attachment #662674 - Flags: approval-mozilla-aurora?
Attachment #662674 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #662674 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d54dcbfd09ee
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Updated

5 years ago
status-firefox16: affected → fixed
status-firefox17: affected → fixed
(Assignee)

Comment 14

5 years ago
>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...

Updated

5 years ago
Keywords: valgrind
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.