Fix for GC hole when using software write watch

Issue:
- When using software write watch, getting dirty pages is synchronized with growing the write watch table using gc_lock, as those cannot happen concurrently
- It turns out that a foreground GC can happen at that point, pausing the background GC while it tries to acquire gc_lock
- In this issue, the last object in the previously revisited page is a free object whose range spans one or more pages
- With the intent of skipping the free range, revisit_written_page() records the next object to scan immediately following the free range
- Since only 100 dirty pages are retrieved at a time, the next set of up to 100 dirty pages needs to be requested. Before that, a foreground GC begins, and the background GC pauses trying to acquire gc_lock.
- The foreground GC promotes some objects, and in the process allocates from the free range above, and completes
- One of the moved objects in that previously free range gets a reference to a new object that is not referenced from anywhere else
- The background GC resumes, determines that the moved object's page is dirty, but skips revisiting the object because the previously recorded next object from which to start scanning, already took the free range size into account before it was allocated
- The background GC does not mark through the moved object, so the newly referenced object appears to be free, and is sweeped
- Later, either verify_heap fails or the app crashes, when trying to access that referenced object

Fix:
- Similarly to what is done for the large object heap, when revisiting a page and using software write watch, don't advance last_object when the current object is a free object that spans beyond the current page or high_address
- On the next revisit, it would start scanning from the previously scanned free object (which now may be allocated or will have a smaller size) until it reaches the requested page to scan

Fixes (but not yet closing) #5194

1 files changed, 15 insertions, 1 deletions

diff --git a/src/gc/gc.cpp b/src/gc/gc.cpp
index 7158285320..585f1f4093 100644
--- a/src/gc/gc.cpp
+++ b/src/gc/gc.cpp
@@ -26320,7 +26320,13 @@ void gc_heap::revisit_written_page (uint8_t* page,
                                     background_mark_object (oo THREAD_NUMBER_ARG);
                                 );
             }
-            else if (concurrent_p && large_objects_p && ((CObjectHeader*)o)->IsFree() && (next_o > min (high_address, page + OS_PAGE_SIZE)))
+            else if (
+                concurrent_p &&
+#ifndef FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP // see comment below
+                large_objects_p &&
+#endif // !FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP
+                ((CObjectHeader*)o)->IsFree() &&
+                (next_o > min (high_address, page + OS_PAGE_SIZE)))
             {
                 // We need to not skip the object here because of this corner scenario:
                 // A large object was being allocated during BGC mark so we first made it 
@@ -26330,6 +26336,14 @@ void gc_heap::revisit_written_page (uint8_t* page,
                 // been made into a valid object and some of its memory was changed. We need
                 // to be sure to process those written pages so we can't skip the object just
                 // yet.
+                //
+                // Similarly, when using software write watch, don't advance last_object when
+                // the current object is a free object that spans beyond the current page or
+                // high_address. Software write watch acquires gc_lock before the concurrent
+                // GetWriteWatch() call during revisit_written_pages(). A foreground GC may
+                // happen at that point and allocate from this free region, so when
+                // revisit_written_pages() continues, it cannot skip now-valid objects in this
+                // region.
                 no_more_loop_p = TRUE;
                 goto end_limit;                
             }