Closed Bug 1376676 Opened 3 years ago Closed 3 years ago

Intermittent LeakSanitizer | leak at sqlite3MemMalloc, sqlite3Malloc, sqlite3MallocZero, pcache1Create

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mak)

References

Details

(Keywords: intermittent-failure, memory-leak, Whiteboard: [fxsearch])

Attachments

(1 file)

Any idea what could be going on with this leak, mak?
Flags: needinfo?(mak77)
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[1]'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
Flags: needinfo?(mak77)
Blocks: 1377029
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+
Whiteboard: [fxsearch]
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/44260a3e2218
Avoid a race condition where the History.cpp cloned connection may not be closed. r=asuth
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/44260a3e2218
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.