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

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

({intermittent-failure, memory-leak})

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
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
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 6

2 years ago
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)
Assignee

Updated

2 years ago
Blocks: 1377029
Assignee

Comment 8

2 years ago
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 9

2 years ago
mozreview-review
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+
Assignee

Updated

2 years ago
Whiteboard: [fxsearch]

Comment 10

2 years ago
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
Assignee

Updated

2 years ago
Priority: -- → P1

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44260a3e2218
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment hidden (Intermittent Failures Robot)
You need to log in before you can comment on or make changes to this bug.