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

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: Treeherder Bug Filer, Assigned: mak)

Tracking

({intermittent-failure, memory-leak})

unspecified
mozilla56
intermittent-failure, memory-leak
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment 1

10 months ago
1 failures in 718 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-central: 1

Platform breakdown:
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1376676&startday=2017-06-26&endday=2017-07-02&tree=all

Comment 2

9 months ago
1 failures in 720 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 1

Platform breakdown:
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1376676&startday=2017-07-10&endday=2017-07-16&tree=all
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 5

9 months ago
7 failures in 822 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 4
* mozilla-inbound: 3

Platform breakdown:
* linux64: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1376676&startday=2017-07-17&endday=2017-07-23&tree=all
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)
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 :)
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
Keywords: mlk

Comment 9

9 months 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+
Whiteboard: [fxsearch]

Comment 10

9 months 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
Priority: -- → P1

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44260a3e2218
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 12

9 months ago
3 failures in 1008 pushes (0.003 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 2
* mozilla-inbound: 1

Platform breakdown:
* linux64: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1376676&startday=2017-07-24&endday=2017-07-30&tree=all
You need to log in before you can comment on or make changes to this bug.