399096 - nsDOMCacheUpdateService leaks

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

Attachment: Possible patch

nsDOMCacheUpdateService::GetInstance returns an addrefed pointer.  I see lots of places in the code, however, that assume it doesn't.  We therefore leak the single nsDOMCacheUpdateService when I run the tests in dom/tests/mochitest/ajax/offline.

Ideally we could get away without all the addrefs/releases as was clearly the original intent of the code, but I don't know the ownership model well enough to make that work.  When I changed those places to just use the global pointer directly, I found that in at least two cases (Init and ScheduleOnDocumentStop) the pointer was null, suggesting that nothing else was keeping it alive.  Maybe someone needs to create the service via do_GetService in order for there to be a long-term owning reference to it?  I didn't investigate that much, so please make suggestions for better options if you see them.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 284050 [details] [diff] [review]
Possible patch

This is failing weirdly, probably due to that no-long-living-owning-pointer problem I mentioned in comment 0.  This isn't ready yet...

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Better patch

This passes Mochitests now, and now we're in the same spot we were in before in that the owning reference in the service manager will keep the service alive, so notifications will get fired even after method scopes go away.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 284107 [details] [diff] [review]
Better patch

Fixes one of the few refcounted-object leaks that shows up in Mochitests, which will reduce the noise keeping other leaks from being diagnosed...

Comment 4

[Reed Loden [:reed]]

Comment on attachment 284107 [details] [diff] [review]
Better patch

Blocker now. No need for approval (for check-in once the tree reopens). :)

Comment 5

[Jeff Walden [:Waldo]]

I'm just waiting for the checkin queue to reach a point of sanity before committing.  My time's available irregularly enough that I don't think it's worth my time to put this on that list or to be the holdup when someone decides I'm next up but I don't have the time at that moment to commit it.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Putting this back on reeds queue. I'd rather just have this landed than wait for the right time. The earlier we can get stuff check off the fixed-list the better.

Jeff, of course, if you do have time to land, please do so and mark as fixed

Comment 7

[Jeff Walden [:Waldo]]

Fixt.  Onward!