Closed Bug 1340597 Opened 5 years ago Closed 5 years ago

js::DumpHeap() shouldn't call UnmarkGray on the cells it is iterating over


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- affected
firefox54 --- fixed


(Reporter: mccr8, Assigned: jonco)




(1 file)

I figured out why I was able to reproduce the leak in bug 1336811 so easily: when you take a GC log using DumpHeap, it calls unmark gray on everything! That is going to make it hard to investigate any leaks.

This is a regression from a patch landed for bug 1322420, but I don't see that commit linked in the bug, so I'm not sure which branches it is on.
I can back that out locally, but somebody should figure out what the right thing to do about it is, and also what the right bug number is for the patch.
Flags: needinfo?(jcoppeard)
It's the fix for bug 1322971, which I mistakenly landed with the wrong bug number.

I guess for DumpHeap we really don't want unmark everything.  I'll take a look.
The IterateZone(s)CompartmentsArenasCells functions are only used for DumpHeap and collecting memory statistics.  I made these use an iterator that doesn't trigger a read barrier and appended 'Unbarriered' to their names.

(I considered renaming them to IterateZoneContentsUnbarriered or something because it's getting a bit ridiculous -- let me know if you think that's a good idea.)
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8839184 - Flags: review?(sphink)
Comment on attachment 8839184 [details] [diff] [review]

Review of attachment 8839184 [details] [diff] [review]:

::: js/src/jsgc.h
@@ +1019,5 @@
>   * every compartment, |arenaCallback| on every in-use arena, and |cellCallback|
>   * on every in-use cell in the GC heap.
> + *
> + * Note that no read barrier is triggered on the cells passed to cellCallback,
> + * so no these pointers must not escape the callback.


@@ +1024,3 @@
>   */
>  extern void
> +IterateZonesCompartmentsArenasCellsUnbarriered(JSContext* cx, void* data,

Or should this be IterateZoneGroupsZonesCompartmentsArenasCellsUnbarriered? :-)

Actually no, because it'd probably be within one zone group.

Do we really need the intervening layers? I'm thinking IterateCellsUnbarriered or IterateAllCellsUnbarriered or perhaps (now or later) IterateZoneGroupCellsUnbarriered. Although I can see your point that you're not just iterating over the cells, you're also doing callbacks for zones and compartments and arenas. So yes, I think I'll go for either your IterateZoneContentsUnbarriered or IterateHeapUnbarriered.
Attachment #8839184 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #4)
Good idea, I'm going with IterateHeapUnbarriered and IterateHeapUnbarrieredForZone.
Pushed by
Avoid triggering read barrier in DumpHeap and when collecting stats r=sfink
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
If you could uplift this to Aurora, that would be good, in the unlikely chance we have to investigate a leak there. (Beta is almost out the door, so it isn't needed there.)
You need to log in before you can comment on or make changes to this bug.