Closed Bug 1406438 Opened 2 years ago Closed 2 years ago

GCRuntime::allNonEmptyChunks should be called under the GC lock

Categories

(Core :: JavaScript: GC, enhancement, P2)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch bug1406438-chunk-pool-lock (obsolete) — Splinter Review
Patch to make this method take the lock reference and add locking in callers.
Attachment #8916049 - Flags: review?(sphink)
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_.
Attachment #8916049 - Flags: review?(sphink)
(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 #8916049 - Attachment is obsolete: true
Attachment #8916681 - Flags: review?(sphink)
Attachment #8916681 - Flags: review?(sphink) → review+
Priority: -- → P2
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ab7e9b2f2f
Ensure GC lock held when iterating all non-empty chunks r=sfink
https://hg.mozilla.org/mozilla-central/rev/36ab7e9b2f2f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.