1949356 - Race condition when Purge and MayPurgeStep run concurrently

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect, P1)

Description

[Paul Bone [:pbone]]

This is incredibly unlikely but the problem is that if two Purge()s run concurrently, one of them from MayPurgeStep() then:

Thread A: Calls Purge(), makes a chunk "busy" for purging.
Thread B: Calls MayPurgeStep() and finds the same arena for purging.
Thread B: Can't find any not-busy chunks for purging. returns false WITHOUT CLEARING mIsDeferredPurgePending
Thread B: Either removes the arena from the list or (since Bug 1920451) does not insert it.

Now the mIsDeferredPurgePending flag is set while the arena is not in the list. this is an invalid state and can lead to leaking memory. Depending on Thread A it may recover since Thread A may keep purging and clear that flag.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1949356 - Clear mIsDeferredPurgePending when Purge() returns false r=glandium

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1903758

Comment 3

[Jens Stutte [:jstutte]]

(In reply to Paul Bone [:pbone] from comment #0)

This is incredibly unlikely but the problem is that if two Purge()s run concurrently, one of them from MayPurgeStep() then:

Thread A: Calls Purge(), makes a chunk "busy" for purging.

Thread A is always the main thread in our current setting, except for MayPurgeAll, so yes, very unlikely.

Thread B: Calls MayPurgeStep() and finds the same arena for purging.

Thread B is always the main thread.

Thread B: Can't find any not-busy chunks for purging. returns false WITHOUT CLEARING mIsDeferredPurgePending
Thread B: Either removes the arena from the list or (since Bug 1920451) does not insert it.

Now the mIsDeferredPurgePending flag is set while the arena is not in the list. this is an invalid state and can lead to leaking memory. Depending on Thread A it may recover since Thread A may keep purging and clear that flag.

So yes, for correctness we should fix this, but it is really very unlikely.

BTW, I'd not say we are really leaking memory in that case, it would still be available for re-use. But we might bloat the overhead, though.

Comment 4

[Paul Bone [:pbone]]

(In reply to Jens Stutte [:jstutte] from comment #3)

So yes, for correctness we should fix this, but it is really very unlikely.

I prefer impossible over unlikely. The fix is so trivial (1 line) that it doesn't really matter what the risk to not-landing it is.

BTW, I'd not say we are really leaking memory in that case, it would still be available for re-use. But we might bloat the overhead, though.

True, I shouldn't have said "leak". However it's not generally reusable, it can only be reused by that specific firefox process and mozjemalloc arena. Other parts of Firefox and other programs can't access it.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce66913f92d4
Clear mIsDeferredPurgePending when Purge() returns false r=glandium

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ce66913f92d4