Closed
Bug 1376676
Opened 7 years ago
Closed 7 years ago
Intermittent LeakSanitizer | leak at sqlite3MemMalloc, sqlite3Malloc, sqlite3MallocZero, pcache1Create
Categories
(Toolkit :: Places, defect, P1)
Toolkit
Places
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)
Filed by: philringnalda [at] gmail.com https://treeherder.mozilla.org/logviewer.html#?job_id=110039399&repo=mozilla-central https://queue.taskcluster.net/v1/task/Wr2iU70XQq6TlPTlLfEzww/runs/0/artifacts/public/logs/live_backing.log
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 4•7 years ago
|
||
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•7 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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 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 :)
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Keywords: mlk
Comment 9•7 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•7 years ago
|
Whiteboard: [fxsearch]
Comment 10•7 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•7 years ago
|
Priority: -- → P1
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44260a3e2218
Status: ASSIGNED → RESOLVED
Closed: 7 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.
Description
•