Closed Bug 520526 Opened 11 years ago Closed 11 years ago

Changes to TimerManager cause test_preventive_maintenance.js to leak due to PlacesDBUtils using TimerManager

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: mak, Assigned: robert.strong.bugs)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

This test  (and i think PlacesDBUtils at this point) started leaking recently, looking at the changelogs i think the culprit is bug 471219.

Unfortunatly DBUtils is not a component, we did not want to expense for component registration since this starts later and has no reason to exist at startup, so i can't use the new "update-timer" component registration.

Btw, using registerTimer should not leak.
Attached patch patch rev1 (obsolete) — Splinter Review
I suspect the removal of the following from head_bookmarks.js though that isn't clear since that file is under browser and not toolkit.
var updateSvc = Cc["@mozilla.org/updates/update-service;1"].
                getService(Ci.nsISupports);

Since this code initializes nsIUpdateTimerManager it should notify it for profile-after-change and xpcom-shutdown so it clean up any registered timers in order to prevent leaks.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #404682 - Flags: review?(mak77)
Comment on attachment 404682 [details] [diff] [review]
patch rev1

If you prefer I can create a mock timer manager instead.
i don't mind having a simple commented service instantiation or a mock implementation, this patch would be fine.
Actually i have a couple thoughts/concerns though:
- a change in browser header cannot influence toolkit tests
- you should also update test_preventive_maintenance_checkAndFixDatabase.js
- xpcshell already fires xpcom-shutdown afaik, so why forcing it on test finish?

i have not yet tried the patch though, i'll do that soon to ensure it solves the leak here too.
So, this works, as i suspected the xpcom-shutdown call is useless.

but actually, i'm not sure why nsUpdateService.js.in adds the xpcom-shutdown observer in profile-after-change, xpcshell tests won't call that and this is in toolkit.

probably the observer should be added in start, and the first call to registerTimer should ensure _start has been called or call it.
(In reply to comment #3)
> i don't mind having a simple commented service instantiation or a mock
> implementation, this patch would be fine.
> Actually i have a couple thoughts/concerns though:
> - a change in browser header cannot influence toolkit tests
I looked at it a bit more and believe it is due to the refactoring of the timer manager.

> - you should also update test_preventive_maintenance_checkAndFixDatabase.js
Will do. How about just adding the mock implementation in the head file?

> - xpcshell already fires xpcom-shutdown afaik, so why forcing it on test
> finish?
It didn't when I looked a while back.

(In reply to comment #4)
> So, this works, as i suspected the xpcom-shutdown call is useless.
I just added a dump statement to AUS_observe and TM_observe and they only show up once when the test calls the components observe method passing xpcom-shutdown. I also had a test a while back where a bug wasn't caught because the component didn't get the call to xpcom-shutdown. How did you verify this?

> but actually, i'm not sure why nsUpdateService.js.in adds the xpcom-shutdown
> observer in profile-after-change, xpcshell tests won't call that and this is in
> toolkit.
Seemed reasonable to add it there since the underlying timer is created in profile-after-change. I don't think it is appropriate to setup everything in the constructor since profile prefs are used and a component calling regsiterTimer prior to having a profile will cause the timer manager to not respect the prefs. Not sure what you mean by "and this is in toolkit."?
(In reply to comment #5)
> > So, this works, as i suspected the xpcom-shutdown call is useless.
> I just added a dump statement to AUS_observe and TM_observe and they only show
> up once when the test calls the components observe method passing
> xpcom-shutdown. I also had a test a while back where a bug wasn't caught
> because the component didn't get the call to xpcom-shutdown. How did you verify
> this?

I am working on an patch that fixes places shutdown, moving work to xpcom-shutdown. Actually we call xpcom-shutdown in tail_bookmarks.js (so the above call would still be redundant due to that), but in that patch i completely get rid of the tail file and the cleanup did happen correctly on xpcom-shutdown. iirc some time ago i asked and ted confirmed me xpcshell calls it, but i could recall badly about this. Maybe you can't see any dump because it is called after the test exits from scope (after do_test_finished() to be clear) and xpcshell takes back the control and moves to next test.

> > but actually, i'm not sure why nsUpdateService.js.in adds the xpcom-shutdown
> > observer in profile-after-change, xpcshell tests won't call that and this is in
> > toolkit.
> Seemed reasonable to add it there since the underlying timer is created in
> profile-after-change. I don't think it is appropriate to setup everything in
> the constructor since profile prefs are used and a component calling
> regsiterTimer prior to having a profile will cause the timer manager to not
> respect the prefs. Not sure what you mean by "and this is in toolkit."?

well not in the constructor but in the internal _start() method.
So if i don't call "profile-after-change" but i just call registerTimer, internal _start method is never called. I was just guessing if registerTimer should ensureStarted() and call internal method _start in such a case.

i just mean being in toolkit most unit tests won't bother creating an app that will force that call, and they'll have to force the call manually... i'm trying to move Places cleanup to xpcom-shutdown exactly to avoid forcing anyone using a Places service to manually cleanup in tests.

the mock service in head would be fine as well, but i just ask you if there is no way to fix this in the service without having tests to handle the cleanup. that would be really better.

Thank you for looking into this.
(In reply to comment #6)
 Actually we call xpcom-shutdown in tail_bookmarks.js (so the
> above call would still be redundant due to that)

nevermind this, i was bad recalling, but this enforces my belief
You're right and I'll take care of this in app update
Assignee: robert.bugzilla → nobody
Component: Places → Application Update
QA Contact: places → application.update
Summary: test_preventive_maintenance.js leaks due to PlacesDBUtils using TimerManager → Changes to TimerManager cause test_preventive_maintenance.js to leak due to PlacesDBUtils using TimerManager
Attached patch patch rev2Splinter Review
I've verified it fixes the leak but could you verify this fixes the leak for you as well?
Assignee: nobody → robert.bugzilla
Attachment #404682 - Attachment is obsolete: true
Attachment #404741 - Flags: review?(mak77)
Attachment #404682 - Flags: review?(mak77)
Attachment #404741 - Flags: review?(dtownsend)
Comment on attachment 404741 [details] [diff] [review]
patch rev2

yes, the leak is fixed with that, great work.

Last thing: this way _start() is still only called on profile-after-change. Now i don't know if this really matters in real situations (apps) or it's just a unit test problem, but supposing i don't call profile-after-change, the internal timer will never start, so my registerTimer call is completely useless. That's the main reason i thought to a ensureStarted() in registerTimer. Could be in a real situation profile-after-change can never be missing, so this would not matter, and patch is just good.

i leave to you and Mossop decision if that matters or not, from my point of view (aka solving the leak adding the xpcom-shutdown observer even if profile-after-change is not called) the patch looks good.
Attachment #404741 - Flags: review?(mak77) → review+
The current unit tests for the timer manager explicitly call profile-after-change and any new tests relying on there being a timer would need to do the same. I prefer keeping it this way since it protects against the timer starting before we have profile prefs.
Attachment #404741 - Flags: review?(dtownsend) → review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/088b50af32dc
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #404741 - Flags: approval1.9.2?
Comment on attachment 404741 [details] [diff] [review]
patch rev2

Drivers, I'd like to get this for 1.9.2. Thanks
Comment on attachment 404741 [details] [diff] [review]
patch rev2

Leak fixes are good; tiny, easy-to-understand leak fixes are even better. a=me, but please hold off until after beta 1 code freeze - it'll give this thing a chance to bake, but more importantly, it will help keep the tree quiet while we get the blockers landed.
Attachment #404741 - Flags: approval1.9.2? → approval1.9.2+
You need to log in before you can comment on or make changes to this bug.