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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

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.

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 404682 [details] [diff] [review]
patch rev1

If you prefer I can create a mock timer manager instead.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Robert Strong (they/them - no direct email)]

(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."?

Comment 6

[Marco Bonardo [:mak]]

(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.

Comment 7

[Marco Bonardo [:mak]]

(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

Comment 8

[Robert Strong (they/them - no direct email)]

You're right and I'll take care of this in app update

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

I've verified it fixes the leak but could you verify this fixes the leak for you as well?

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[Robert Strong (they/them - no direct email)]

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.

Comment 12

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/088b50af32dc

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 404741 [details] [diff] [review]
patch rev2

Drivers, I'd like to get this for 1.9.2. Thanks

Comment 14

[Johnathan Nightingale [:johnath]]

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.

Comment 15

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d3c5ebaad147