Closed Bug 1406438 Opened 2 years ago Closed 2 years ago
Non Empty Chunks should be called under the GC lock
allNonEmptyChunks() iterates available chunks and then full chunks. The accessors to get the respective chunk pools take a AutoLockGC reference to ensure they're only called with the GC lock held. It seems like this method should too.
Patch to make this method take the lock reference and add locking in callers.
Comment on attachment 8916049 [details] [diff] [review] bug1406438-chunk-pool-lock Review of attachment 8916049 [details] [diff] [review]: ----------------------------------------------------------------- Huh. This seems like a big omission. Is it actually unsafe? But given that everything seems to be locking now, I guess I don't follow why fullChunks_ and availableChunks_ are UnprotectedData instead of GCLockData like emptyChunks_.
(In reply to Steve Fink [:sfink] [:s:] from comment #2) > Huh. This seems like a big omission. Is it actually unsafe? We can allocate GC memory off-main-thread, which can allocate chunks. So probably. > But given that everything seems to be locking now, I guess I don't follow > why fullChunks_ and availableChunks_ are UnprotectedData instead of > GCLockData like emptyChunks_. GCRuntime::pickChunk() can take an empty chunk and make it an available chunk, so it seems like they should be GCLockData too.
I guess this affects 57, and could affect stability. But we're not aware of any specific problems it causes. Jon / Release engineering people, is something like this worth uplifing? what happens normally?
Updated patch which also makes availableChunks_ and fullChunks_ GCLockData.
Attachment #8916681 - Flags: review?(sphink) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/36ab7e9b2f2f Ensure GC lock held when iterating all non-empty chunks r=sfink
I'm going to call this wontfix for 57. Please reset status to affected and request uplift if you disagree.
You need to log in before you can comment on or make changes to this bug.