59 bytes, text/x-review-board-request
Any idea what could be going on with this leak, mak?
This shows an asyncClone()d connection not getting cleaned up. Only Places does this. The JS logic seems safe enough, so my guess would be that ConcurrentStatementsHolder's state machine has a race. Specifically: - ConcurrentStatementsHolder::ConcurrentStatementsHolder invoked, triggers AsyncClone - ConcurrentStatementsHolder::Shutdown invoked - ConcurrentStatementsHolder::Complete gets handed the cloned connection. - History seems to be an eternal singleton (it's stored as a raw static History* whose shutdown method doesn't seem to try and delete itself, etc.) and it never drops its mConcurrentStatementsHolder reference, suggesting the ConcurrentStatementsHolder will never be destroyed and accordingly its connection will never be failsafe Close()d by the destructor. Although mozStorageService now MOZ_CRASHes at shutdown, it only does this if gShutdownChecks is SCM_CRASH. Since this is an opt build and not a DEBUG build, and I don't see a MOZ_SHUTDOWN_CHECKS env var, then we should be on SCM_RECORD or SCM_NOTHING, neither of which trigger our crash. Which is why we don't see a crash in this case where we would expect to see one. It's also possible there's an even more complicated and unlikely race going on, but if places wants to change this, the easiest thing would be to have ConcurrentStatementsHolder gain a shutdown boolean flag that its Complete() method checks and calls back into Shutdown(). 1: https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/components/places/History.cpp#2000
I'm working on some Places shutdown refactoring in bug 1275878, so this sounds like coming at the right time. I'll look into Andrew's analysis and make a patch out of it.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Component: Storage → Places
Andrew, as usual your analysis was completely correct. The patch just implements your suggestion, and I think it's indeed the right thing to do. I admit it's basically your patch with my review, but in practice we are doing the opposite :)
Comment on attachment 8889663 [details] Bug 1376676 - Avoid a race condition where the History.cpp cloned connection may not be closed. https://reviewboard.mozilla.org/r/160724/#review166002 Hopefully this makes that other more frequent intermittent go away too!
Attachment #8889663 - Flags: review?(bugmail) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/44260a3e2218 Avoid a race condition where the History.cpp cloned connection may not be closed. r=asuth
You need to log in before you can comment on or make changes to this bug.